Date: Sun, 14 Jan 2018 15:42:56 -0800 From: Nathan Whitehorn <nwhitehorn@freebsd.org> To: Konstantin Belousov <kostikbel@gmail.com> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r327950 - in head/sys/powerpc: aim include powerpc ps3 Message-ID: <f4b44b69-7b06-6b5a-c17c-31bd46ca1af0@freebsd.org> In-Reply-To: <b2b1bf30-177b-af30-54ce-f484224bb2ad@freebsd.org> References: <201801132314.w0DNEra5002692@repo.freebsd.org> <20180113232441.GV1684@kib.kiev.ua> <010d0153-8931-a3c2-db21-dfcbaf848fc0@freebsd.org> <f33e9b1a-28bd-e6cf-4bdb-ec0097c0787d@freebsd.org> <20180114083036.GX1684@kib.kiev.ua> <ede06fc6-7c34-100c-8a7a-6346cd8cd363@freebsd.org> <20180114170502.GB1684@kib.kiev.ua> <184ba3ee-a9f7-01ed-bb02-1bcba9acc041@freebsd.org> <20180114175211.GD1684@kib.kiev.ua> <b2b1bf30-177b-af30-54ce-f484224bb2ad@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
This is a multi-part message in MIME format.
--------------03AE6774B8597D0FB912A356
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
On 01/14/18 09:57, Nathan Whitehorn wrote:
>
>
> On 01/14/18 09:52, Konstantin Belousov wrote:
>> On Sun, Jan 14, 2018 at 09:30:53AM -0800, Nathan Whitehorn wrote:
>>> The immediate consequence of that is that no MI code that knows about
>>> direct maps can possibly take advantage of the direct map on this
>>> platform. Do we really want that to save some conditional logic that
>>> would get optimized out on amd64 and arm64 anyway? I really do not see
>>> the benefit here.
>> It is not clear what do you mean. Are you saying that there is no
>> benefit
>> of providing the conditional logic, or that it is not benefit of
>> exclusing
>> powerpc ?
>
> Sorry, that was poorly stated. Let me try again:
>
> If we make a PPC_PHYS_TO_DMAP(), but there is an MI PHYS_TO_DMAP()
> API, consumer code in the MI parts of the kernel won't be able to
> benefit from the PPC direct map, which seems unfortunate. The cost
> from a code perspective of having an if (direct_map_available) seems
> low, since on systems where direct_map_available is defined to be 1,
> the compiler will optimize it to the same code as if gated by #ifdef.
> It might be more cumbersome to write the code, however.
>
>> I do not object against adding the conditional, but it should not be
>> too clumsy to use.
>>
>
> OK. Let me try to draft something in the next couple days and see how
> much of a pain it is in practice.
> -Nathan
>
How about the attached? It makes PHYS_TO_DMAP() return 0 if no mapping
exists. This is straightforward, does not introduce extra macros, and
can pretty easily replace SFBUF_OPTIONAL_DIRECT_MAP on the assumption
that PHYS_TO_DMAP() is cheap. I've modified the other MI-ish consumers
in the tree accordingly; compat/linuxkpi/common/src/linux_page.c already
does the right thing and needed no modifications.
-Nathan
--------------03AE6774B8597D0FB912A356
Content-Type: text/x-patch;
name="PHYS_TO_DMAP.diff"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename="PHYS_TO_DMAP.diff"
Index: powerpc/include/vmparam.h
===================================================================
--- powerpc/include/vmparam.h (revision 327952)
+++ powerpc/include/vmparam.h (working copy)
@@ -240,13 +240,12 @@
#define SFBUF_PHYS_DMAP(x) (x)
/*
- * We (usually) have a direct map of all physical memory. All
- * uses of this macro must be gated by a check on hw_direct_map!
- * The location of the direct map may not be 1:1 in future, so use
- * of the macro is recommended; it may also grow an assert that hw_direct_map
- * is set.
+ * We (usually) have a direct map of all physical memory, so provide
+ * a macro to use to get the kernel VA address for a given PA. Returns
+ * 0 if the direct map is unavailable. The location of the direct map
+ * may not be 1:1 in future, so use of the macro is recommended.
*/
-#define PHYS_TO_DMAP(x) x
-#define DMAP_TO_PHYS(x) x
+#define PHYS_TO_DMAP(x) (hw_direct_map ? 0 : (x))
+#define DMAP_TO_PHYS(x) (hw_direct_map ? 0 : (x))
#endif /* _MACHINE_VMPARAM_H_ */
Index: vm/vm_page.c
===================================================================
--- vm/vm_page.c (revision 327952)
+++ vm/vm_page.c (working copy)
@@ -2937,7 +2937,8 @@
{
#if defined(DIAGNOSTIC) && defined(PHYS_TO_DMAP)
- if ((m->flags & PG_ZERO) != 0) {
+ if ((m->flags & PG_ZERO) != 0 &&
+ PHYS_TO_DMAP(VM_PAGE_TO_PHYS(m)) != 0) {
uint64_t *p;
int i;
p = (uint64_t *)PHYS_TO_DMAP(VM_PAGE_TO_PHYS(m));
Index: dev/efidev/efirt.c
===================================================================
--- dev/efidev/efirt.c (revision 327952)
+++ dev/efidev/efirt.c (working copy)
@@ -115,6 +115,11 @@
return (0);
}
efi_systbl = (struct efi_systbl *)PHYS_TO_DMAP(efi_systbl_phys);
+ if (efi_systbl == NULL) {
+ if (bootverbose)
+ printf("EFI systbl not mapped in kernel VA\n");
+ return (0);
+ }
if (efi_systbl->st_hdr.th_sig != EFI_SYSTBL_SIG) {
efi_systbl = NULL;
if (bootverbose)
--------------03AE6774B8597D0FB912A356--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?f4b44b69-7b06-6b5a-c17c-31bd46ca1af0>