From owner-svn-src-head@freebsd.org Thu Jan 18 15:24:20 2018 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id DC418EB23A7; Thu, 18 Jan 2018 15:24:20 +0000 (UTC) (envelope-from nwhitehorn@freebsd.org) Received: from d.mail.sonic.net (d.mail.sonic.net [64.142.111.50]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id C441372E6D; Thu, 18 Jan 2018 15:24:20 +0000 (UTC) (envelope-from nwhitehorn@freebsd.org) Received: from comporellon.tachypleus.net (cpe-75-82-218-62.socal.res.rr.com [75.82.218.62]) (authenticated bits=0) by d.mail.sonic.net (8.15.1/8.15.1) with ESMTPSA id w0IFOCxg017296 (version=TLSv1.2 cipher=DHE-RSA-AES128-SHA bits=128 verify=NOT); Thu, 18 Jan 2018 07:24:13 -0800 Subject: Re: svn commit: r327950 - in head/sys/powerpc: aim include powerpc ps3 To: Konstantin Belousov Cc: Marius Strobl , svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org References: <20180115111812.GF1684@kib.kiev.ua> <20180115170603.GJ1684@kib.kiev.ua> <9e5554d7-6a0c-5910-8cb6-74f98259536f@freebsd.org> <20180115175335.GK1684@kib.kiev.ua> <20180116193208.GA12364@alchemy.franken.de> <11a7fdd6-cfd6-26c1-ae3c-7d8a63924d5a@freebsd.org> <20180117094413.GF55707@kib.kiev.ua> From: Nathan Whitehorn Message-ID: <57f837ce-1209-1e9a-158f-7eac5ae6d59a@freebsd.org> Date: Thu, 18 Jan 2018 07:24:11 -0800 User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: <20180117094413.GF55707@kib.kiev.ua> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US X-Sonic-CAuth: UmFuZG9tSVbXSo0cUGAtRqnJ1uG5RlJvcuocztENOOtcBg9IxkZpt0E1QiNynlJNPxHB6FDRulVT2go42WWMFBFrkga7a9YtoRD/8p/GkzM= X-Sonic-ID: C;UhDjomP85xGFyeBQ9eIpJw== M;rDWpo2P85xGFyeBQ9eIpJw== X-Spam-Flag: No X-Sonic-Spam-Details: 0.0/5.0 by cerberusd X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 18 Jan 2018 15:24:21 -0000 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 >>>> + >>>> #define SFBUF >>>> #define SFBUF_MAP >>>> -#define SFBUF_OPTIONAL_DIRECT_MAP dcache_color_ignore >>>> -#include >>>> -#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. -Nathan