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: amd64/include/vmparam.h
===================================================================
--- amd64/include/vmparam.h (revision 328006)
+++ amd64/include/vmparam.h (working copy)
@@ -190,6 +190,7 @@
* because the result is not actually accessed until later, but the early
* vt fb startup needs to be reworked.
*/
+#define DIRECT_MAP_AVAILABLE 1
#define PHYS_TO_DMAP(x) ({ \
KASSERT(dmaplimit == 0 || (x) < dmaplimit, \
("physical address %#jx not covered by the DMAP", \
Index: arm64/include/vmparam.h
===================================================================
--- arm64/include/vmparam.h (revision 328006)
+++ arm64/include/vmparam.h (working copy)
@@ -176,6 +176,7 @@
#define VIRT_IN_DMAP(va) ((va) >= DMAP_MIN_ADDRESS && \
(va) < (dmap_max_addr))
+#define DIRECT_MAP_AVAILABLE
#define PHYS_TO_DMAP(pa) \
({ \
KASSERT(PHYS_IN_DMAP(pa), \
Index: dev/efidev/efirt.c
===================================================================
--- dev/efidev/efirt.c (revision 328006)
+++ 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)
Index: kern/subr_sfbuf.c
===================================================================
--- kern/subr_sfbuf.c (revision 328006)
+++ kern/subr_sfbuf.c (working copy)
@@ -88,8 +88,8 @@
vm_offset_t sf_base;
int i;
-#ifdef SFBUF_OPTIONAL_DIRECT_MAP
- if (SFBUF_OPTIONAL_DIRECT_MAP)
+#ifdef DIRECT_MAP_AVAILABLE
+ if (DIRECT_MAP_AVAILABLE)
return;
#endif
@@ -119,8 +119,8 @@
struct sf_buf *sf;
int error;
-#ifdef SFBUF_OPTIONAL_DIRECT_MAP
- if (SFBUF_OPTIONAL_DIRECT_MAP)
+#ifdef DIRECT_MAP_AVAILABLE
+ if (DIRECT_MAP_AVAILABLE)
return ((struct sf_buf *)m);
#endif
@@ -181,8 +181,8 @@
sf_buf_free(struct sf_buf *sf)
{
-#ifdef SFBUF_OPTIONAL_DIRECT_MAP
- if (SFBUF_OPTIONAL_DIRECT_MAP)
+#ifdef DIRECT_MAP_AVAILABLE
+ if (DIRECT_MAP_AVAILABLE)
return;
#endif
@@ -205,8 +205,8 @@
sf_buf_ref(struct sf_buf *sf)
{
-#ifdef SFBUF_OPTIONAL_DIRECT_MAP
- if (SFBUF_OPTIONAL_DIRECT_MAP)
+#ifdef DIRECT_MAP_AVAILABLE
+ if (DIRECT_MAP_AVAILABLE)
return;
#endif
Index: powerpc/include/vmparam.h
===================================================================
--- powerpc/include/vmparam.h (revision 328006)
+++ powerpc/include/vmparam.h (working copy)
@@ -37,6 +37,10 @@
#ifndef _MACHINE_VMPARAM_H_
#define _MACHINE_VMPARAM_H_
+#ifndef LOCORE
+#include <machine/md_var.h>
+#endif
+
#define USRSTACK SHAREDPAGE
#ifndef MAXTSIZ
@@ -236,17 +240,21 @@
*/
#define SFBUF
#define SFBUF_NOMD
-#define SFBUF_OPTIONAL_DIRECT_MAP hw_direct_map
-#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
-
+#ifdef __powerpc64__
+#define DMAP_ADDRESS 0x0000000000000000UL
+#else
+#define DMAP_ADDRESS 0x00000000UL
+#endif
+
+#define DIRECT_MAP_AVAILABLE (hw_direct_map)
+#define PHYS_TO_DMAP(x) (hw_direct_map ? (x | DMAP_ADDRESS) : 0)
+#define DMAP_TO_PHYS(x) (hw_direct_map ? (x & ~DMAP_ADDRESS) : 0)
+
#endif /* _MACHINE_VMPARAM_H_ */
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)
+
#endif /* !_MACHINE_VMPARAM_H_ */
Index: sys/sf_buf.h
===================================================================
--- sys/sf_buf.h (revision 328006)
+++ sys/sf_buf.h (working copy)
@@ -77,9 +77,6 @@
* that do no invalidate cache on the rest of CPUs.
* SFBUF_NOMD This machine doesn't have machine/sf_buf.h
*
- * SFBUF_OPTIONAL_DIRECT_MAP Value of this define is used as boolean
- * variable that tells whether machine is
- * capable of direct map or not at runtime.
* SFBUF_MAP This machine provides its own sf_buf_map() and
* sf_buf_unmap().
* SFBUF_PROCESS_PAGE This machine provides sf_buf_process_page()
@@ -109,9 +106,6 @@
#ifndef SFBUF_NOMD
#include <machine/sf_buf.h>
#endif
-#ifdef SFBUF_OPTIONAL_DIRECT_MAP
-#include <machine/md_var.h>
-#endif
#ifdef SFBUF
struct sf_buf *sf_buf_alloc(struct vm_page *, int);
@@ -121,9 +115,9 @@
static inline vm_offset_t
sf_buf_kva(struct sf_buf *sf)
{
-#ifdef SFBUF_OPTIONAL_DIRECT_MAP
- if (SFBUF_OPTIONAL_DIRECT_MAP)
- return (SFBUF_PHYS_DMAP(VM_PAGE_TO_PHYS((vm_page_t)sf)));
+#ifdef DIRECT_MAP_AVAILABLE
+ if (DIRECT_MAP_AVAILABLE)
+ return (PHYS_TO_DMAP(VM_PAGE_TO_PHYS((vm_page_t)sf)));
#endif
return (sf->kva);
@@ -132,8 +126,8 @@
static inline vm_page_t
sf_buf_page(struct sf_buf *sf)
{
-#ifdef SFBUF_OPTIONAL_DIRECT_MAP
- if (SFBUF_OPTIONAL_DIRECT_MAP)
+#ifdef DIRECT_MAP_AVAILABLE
+ if (DIRECT_MAP_AVAILABLE)
return ((vm_page_t)sf);
#endif
Index: vm/vm_page.c
===================================================================
--- vm/vm_page.c (revision 328006)
+++ 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));
_______________________________________________
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
