svn commit: r327950 - in head/sys/powerpc: aim include powerpc ps3

[Konstantin Belousov]

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.