On 01/18/2018 09:35, Konstantin Belousov wrote: > On Thu, Jan 18, 2018 at 07:24:11AM -0800, Nathan Whitehorn wrote: >> >> On 01/17/18 01:44, Konstantin Belousov wrote: >>> On Tue, Jan 16, 2018 at 09:30:29PM -0800, Nathan Whitehorn wrote: >>>> On 01/16/18 11:32, Marius Strobl wrote: >>>>> On Mon, Jan 15, 2018 at 03:20:49PM -0800, Nathan Whitehorn wrote: >>>>>> On 01/15/18 09:53, Konstantin Belousov wrote: >>>>>>> On Mon, Jan 15, 2018 at 09:32:56AM -0800, Nathan Whitehorn wrote: >>>>>>>> That seems fine to me. I don't think a less-clumsy way that does not >>>>>>>> involve extra indirection is possible. The PHYS_TO_DMAP() returning >>>>>>>> NULL >>>>>>>> is about the best thing I can come up with from a clumsiness standpoint >>>>>>>> since plenty of code checks for null pointers already, but doesn't >>>>>>>> cleanly handle the rarer case where you want to test for the existence >>>>>>>> of direct maps in general without testing some potemkin address. >>>>>>>> >>>>>>>> My one reservation about PMAP_HAS_DMAP or the like as a selector is >>>>>>>> that >>>>>>>> it does not encode the full shape of the problem: one could imagine >>>>>>>> having a direct map that only covers a limited range of RAM (I am not >>>>>>>> sure whether the existence of dmaplimit on amd64 implies this can >>>>>>>> happen >>>>>>>> with non-device memory in real life), for example. These cases are >>>>>>>> currently covered by an assert() in PHYS_TO_DMAP(), whereas having >>>>>>>> PHYS_TO_DMAP() return NULL allows a more flexible signalling and the >>>>>>>> potential for the calling code to do something reasonable to handle the >>>>>>>> error. A single global flag can't convey information at this kind of >>>>>>>> granularity. Is this a reasonable concern? Or am I overthinking things? >>>>>>> IMO it is overreaction. amd64 assumes that all normal memory is covered >>>>>>> by DMAP. It must never fail. See, for instance, the implementation >>>>>>> of the sf bufs for it. >>>>>>> >>>>>>> If device memory not covered by DMAP can exists, it is the driver >>>>>>> problem. >>>>>>> For instance, for NVDIMMs I wrote specific mapping code which >>>>>>> establishes >>>>>>> kernel mapping for it, when not covered by EFI memory map and >>>>>>> correspondingly >>>>>>> not included into DMAP. >>>>>>> >>>>>> Fair enough. Here's a patch with a new flag (DIRECT_MAP_AVAILABLE). I've >>>>>> also retooled the sfbuf code to use this rather than its own flags that >>>>>> mean the same things. The sparc64 part of the patch is untested. >>>>>> -Nathan >>>>>> Index: sparc64/include/vmparam.h >>>>>> =================================================================== >>>>>> --- sparc64/include/vmparam.h (revision 328006) >>>>>> +++ sparc64/include/vmparam.h (working copy) >>>>>> @@ -240,10 +240,12 @@ >>>>>> */ >>>>>> #define ZERO_REGION_SIZE PAGE_SIZE >>>>>> >>>>>> +#include <machine/tlb.h> >>>>>> + >>>>>> #define SFBUF >>>>>> #define SFBUF_MAP >>>>>> -#define SFBUF_OPTIONAL_DIRECT_MAP dcache_color_ignore >>>>>> -#include <machine/tlb.h> >>>>>> -#define SFBUF_PHYS_DMAP(x) TLB_PHYS_TO_DIRECT(x) >>>>>> >>>>>> +#define DIRECT_MAP_AVAILABLE dcache_color_ignore >>>>>> +#define PHYS_TO_DMAP(x) (DIRECT_MAP_AVAILABLE ? (TLB_PHYS_TO_DIRECT(x) >>>>>> : 0) >>>>> What dcache_color_ignore actually indicates is the presence of >>>>> hardware unaliasing support, in other words the ability to enter >>>>> duplicate cacheable mappings into the MMU. While a direct map is >>>>> available and used by MD code on all supported CPUs down to US-I, >>>>> the former feature is only implemented in the line of Fujitsu SPARC64 >>>>> processors. IIRC, the sfbuf(9) code can't guarantee that there isn't >>>>> already a cacheable mapping from a different VA to the same PA, >>>>> which is why it employs dcache_color_ignore. Is that a general >>>>> constraint of all MI PHYS_TO_DMAP users or are there consumers >>>>> which can guarantee that they are the only users of a mapping >>>>> to the same PA? >>>>> >>>>> Marius >>>>> >>>> With the patch, there are four uses of this in the kernel: the sfbuf >>>> code, a diagnostic check on page zeroing, part of the EFI runtime code, >>>> and part of the Linux KBI compat. The second looks safe from this >>>> perspective and at least some of the others (EFI runtime) are irrelevant >>>> on sparc64. But I really have no idea what was intended for the >>>> semantics of this API -- I didn't even know it *was* an MI API until >>>> this commit. Maybe kib can comment? If this is outside the semantics of >>>> PHYS_TO_DMAP, then we need to keep the existing sfbuf code. >>> sfbufs cannot guarantee that there is no other mapping of the page when >>> the sfbuf is created. For instance, one of the use of sfbufs is to map >>> the image page 0 to read ELF headers when doing the image activation. >>> The image might be mapped by other processes, and we do not control the >>> address at which it mapped. >>> >>> So the direct map accesses must work regardless of the presence of other >>> page mappings, and the check for dcache_color_ignore is needed to allow >>> MI code to take advantage of DMAP. >>> >> So: what do you want to happen with PHYS_TO_DMAP()? Do we want to claim >> to MI that a direct map is "available" in such circumstances, or >> "unavailable"? Should sfbuf retain a separate API? I have no preferences >> here and just want to close out this issue. > Perhaps DMAP should be conditionally available to the MI layer, same as > on powerpc ? I.e. your patch cited above looks right to me, unless I > misunderstand the Marius' response. > Yes, it should. Only the sparc64 machines where dcache_color_ignore is true should attempt to use the direct map in MI code. The machine-dependent uiomove_fromphys() on sparc64 shows the hoops one has to jump through in order to correctly use the direct map on sparc64.
_______________________________________________ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
