On 01/15/18 03:18, Konstantin Belousov wrote:
On Sun, Jan 14, 2018 at 03:46:38PM -0800, Nathan Whitehorn wrote:
On 01/14/18 15:42, Nathan Whitehorn wrote:
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
I think that this is fine from the PoV of code complexity.
We now require MI (but not amd64 and arm64 MD) code to check for
PHYS_TO_DMAP() return value, which is redundand for a*64. I am not sure
if this is good choice from the PoV of possible microoptimizations.
You promised something which is trivially detectable by compiler as
an excess code.
Fair enough -- the logic was that a lot of code already checks for NULL
pointers (the linux_page.c for instance required no changes to do the
right thing). If we want it to be fully compiler-transparent, we could
also add a flag, but that would add more code complexity. Do you have a
preference? I would be happy to draft that too.
Sorry, this is the patch I meant to send.
Do you plan to convert sf buf code on powerpc ?
Yes, once this is finalized.
-Nathan
-Nathan
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 ? (x) : 0)
+#define DMAP_TO_PHYS(x) (hw_direct_map ? (x) : 0)
#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)
_______________________________________________
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"
