From owner-svn-src-all@freebsd.org Wed Jan 17 05:30:39 2018 Return-Path: Delivered-To: svn-src-all@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 CDCD4E7C477; Wed, 17 Jan 2018 05:30:39 +0000 (UTC) (envelope-from nwhitehorn@freebsd.org) Received: from c.mail.sonic.net (c.mail.sonic.net [64.142.111.80]) (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 9C18A7D418; Wed, 17 Jan 2018 05:30:39 +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 c.mail.sonic.net (8.15.1/8.15.1) with ESMTPSA id w0H5UUGY005788 (version=TLSv1.2 cipher=DHE-RSA-AES128-SHA bits=128 verify=NOT); Tue, 16 Jan 2018 21:30:31 -0800 Subject: Re: svn commit: r327950 - in head/sys/powerpc: aim include powerpc ps3 To: Marius Strobl Cc: Konstantin Belousov , svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org References: <20180114175211.GD1684@kib.kiev.ua> <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> From: Nathan Whitehorn Message-ID: <11a7fdd6-cfd6-26c1-ae3c-7d8a63924d5a@freebsd.org> Date: Tue, 16 Jan 2018 21:30:29 -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: <20180116193208.GA12364@alchemy.franken.de> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US X-Sonic-CAuth: UmFuZG9tSVaVwOwKMg9/LXT+QiLNI7NNYESoK1LTULNj/VKZfJGgF5gZoiOLLRewYwzGpawXoNrUhZ83yHmN/uvixbLK076gLCY8sXjW6Hw= X-Sonic-ID: C;Mj0xiEf75xGX4tzi2dYaJA== M;OuKhiEf75xGX4tzi2dYaJA== X-Spam-Flag: No X-Sonic-Spam-Details: 0.0/5.0 by cerberusd X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 17 Jan 2018 05:30:39 -0000 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. -Nathan