Date: Mon, 15 Jan 2018 15:20:49 -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: <bb27ba01-8383-6b85-8b2b-65227ff46efc@freebsd.org> In-Reply-To: <20180115175335.GK1684@kib.kiev.ua> References: <20180114170502.GB1684@kib.kiev.ua> <184ba3ee-a9f7-01ed-bb02-1bcba9acc041@freebsd.org> <20180114175211.GD1684@kib.kiev.ua> <b2b1bf30-177b-af30-54ce-f484224bb2ad@freebsd.org> <f4b44b69-7b06-6b5a-c17c-31bd46ca1af0@freebsd.org> <e04bc7a6-fa77-9ca0-2aff-dc29c543c9a1@freebsd.org> <20180115111812.GF1684@kib.kiev.ua> <f6350c61-55d1-9bf7-c4b3-e10fb329a42a@freebsd.org> <20180115170603.GJ1684@kib.kiev.ua> <9e5554d7-6a0c-5910-8cb6-74f98259536f@freebsd.org> <20180115175335.GK1684@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
This is a multi-part message in MIME format.
--------------082F835BD7FFED1A7DEBEEC2
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
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
--------------082F835BD7FFED1A7DEBEEC2
Content-Type: text/x-patch;
name="dmap_api.diff"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename="dmap_api.diff"
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));
--------------082F835BD7FFED1A7DEBEEC2--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?bb27ba01-8383-6b85-8b2b-65227ff46efc>