[v4,2/9] x86: introduce a new set of APIs to manage Xen page tables
diff mbox series

Message ID fba262641f8233b4b9856cffeeb7a3ad0bad086a.1575477921.git.hongyxia@amazon.com
State New, archived
Headers show
Series
  • Add alternative API for Xen PTEs
Related show

Commit Message

Xia, Hongyan Dec. 4, 2019, 5:10 p.m. UTC
From: Wei Liu <wei.liu2@citrix.com>

We are going to switch to using domheap page for page tables.
A new set of APIs is introduced to allocate, map, unmap and free pages
for page tables.

The allocation and deallocation work on mfn_t but not page_info,
because they are required to work even before frame table is set up.

Implement the old functions with the new ones. We will rewrite, site
by site, other mm functions that manipulate page tables to use the new
APIs.

Note these new APIs still use xenheap page underneath and no actual
map and unmap is done so that we don't break xen half way. They will
be switched to use domheap and dynamic mappings when usage of old APIs
is eliminated.

No functional change intended in this patch.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Signed-off-by: Hongyan Xia <hongyxia@amazon.com>

---
Changed since v3:
- const qualify unmap_xen_pagetable_new().
- remove redundant parentheses.
---
 xen/arch/x86/mm.c        | 39 ++++++++++++++++++++++++++++++++++-----
 xen/include/asm-x86/mm.h | 11 +++++++++++
 2 files changed, 45 insertions(+), 5 deletions(-)

Comments

Xia, Hongyan Dec. 4, 2019, 5:54 p.m. UTC | #1
> There's no need for the map/unmap functions to have a _new
> suffix, is there?

I thought this was weird at first also, but what I find really useful
is that we can just change all call sites to the new API in small steps
without breaking. Otherwise we have to merge a huge batch of
changes (around 40 patches) at once.

Hongyan
Jan Beulich Dec. 5, 2019, 7:20 a.m. UTC | #2
On 04.12.2019 18:54, Xia, Hongyan wrote:
>> There's no need for the map/unmap functions to have a _new
>> suffix, is there?
> 
> I thought this was weird at first also, but what I find really useful
> is that we can just change all call sites to the new API in small steps
> without breaking. Otherwise we have to merge a huge batch of
> changes (around 40 patches) at once.

But my comment was on functions _not_ having an "old"
equivalent. Having the suffix there initially makes for more
code churn than necessary when finally dropping the suffixes.

Jan
Julien Grall Dec. 11, 2019, 4:33 p.m. UTC | #3
Hi,

On 04/12/2019 17:10, Hongyan Xia wrote:
> From: Wei Liu <wei.liu2@citrix.com>
> 
> We are going to switch to using domheap page for page tables.
> A new set of APIs is introduced to allocate, map, unmap and free pages
> for page tables.
> 
> The allocation and deallocation work on mfn_t but not page_info,
> because they are required to work even before frame table is set up.
> 
> Implement the old functions with the new ones. We will rewrite, site
> by site, other mm functions that manipulate page tables to use the new
> APIs.
> 
> Note these new APIs still use xenheap page underneath and no actual
> map and unmap is done so that we don't break xen half way. They will
> be switched to use domheap and dynamic mappings when usage of old APIs
> is eliminated.
> 
> No functional change intended in this patch.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Signed-off-by: Hongyan Xia <hongyxia@amazon.com>
> 
> ---
> Changed since v3:
> - const qualify unmap_xen_pagetable_new().
> - remove redundant parentheses.
> ---
>   xen/arch/x86/mm.c        | 39 ++++++++++++++++++++++++++++++++++-----
>   xen/include/asm-x86/mm.h | 11 +++++++++++
>   2 files changed, 45 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 7d4dd80a85..ca362ad638 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -119,6 +119,7 @@
>   #include <xen/efi.h>
>   #include <xen/grant_table.h>
>   #include <xen/hypercall.h>
> +#include <xen/mm.h>
>   #include <asm/paging.h>
>   #include <asm/shadow.h>
>   #include <asm/page.h>
> @@ -5020,22 +5021,50 @@ int mmcfg_intercept_write(
>   }
>   
>   void *alloc_xen_pagetable(void)
> +{
> +    mfn_t mfn;
> +
> +    mfn = alloc_xen_pagetable_new();
> +    ASSERT(!mfn_eq(mfn, INVALID_MFN));

The current version of alloc_xen_pagetable() may return NULL. So I can't 
see why this would not happen with the new implementation (see more below).

Furthermore, AFAIK, mfn_to_virt() is not able to deal with INVALID_MFN. 
While the ASSERT will catch such error in debug build, non-debug build 
will end up to undefined behavior (allocation failure is a real 
possibility).

Regardless the discussion on whether the ASSERT() is warrant, I think 
you want to have:

if ( mfn_eq(mfn, INVALID_MFN) )
    return NULL;

I am half tempted to suggest to put that in map_xen_pagetable_new() for 
hardening.

> +
> +    return map_xen_pagetable_new(mfn);
> +}
> +
> +void free_xen_pagetable(void *v)
> +{
> +    if ( system_state != SYS_STATE_early_boot )
> +        free_xen_pagetable_new(virt_to_mfn(v));
> +}
> +
> +mfn_t alloc_xen_pagetable_new(void)
>   {
>       if ( system_state != SYS_STATE_early_boot )
>       {
>           void *ptr = alloc_xenheap_page();
>   
>           BUG_ON(!hardware_domain && !ptr);

This BUG_ON() only catch memory exhaustion before the Hardware Domain 
(aka Dom0) is created. Afterwards, the pointer may be NULL. As ...

> -        return ptr;
> +        return virt_to_mfn(ptr);

virt_to_mfn() requires a valid MFN, you will end up to undefined behavior.

>       }
>   
> -    return mfn_to_virt(mfn_x(alloc_boot_pages(1, 1)));
> +    return alloc_boot_pages(1, 1);
>   }
>   
> -void free_xen_pagetable(void *v)
> +void *map_xen_pagetable_new(mfn_t mfn)
>   {
> -    if ( system_state != SYS_STATE_early_boot )
> -        free_xenheap_page(v);
> +    return mfn_to_virt(mfn_x(mfn));
> +}
> +
> +/* v can point to an entry within a table or be NULL */
> +void unmap_xen_pagetable_new(const void *v)
> +{
> +    /* XXX still using xenheap page, no need to do anything.  */
> +}
> +
> +/* mfn can be INVALID_MFN */
> +void free_xen_pagetable_new(mfn_t mfn)
> +{
> +    if ( system_state != SYS_STATE_early_boot && !mfn_eq(mfn, INVALID_MFN) )
> +        free_xenheap_page(mfn_to_virt(mfn_x(mfn)));
>   }
>   
>   static DEFINE_SPINLOCK(map_pgdir_lock);
> diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
> index 9d2b833579..76593fe9e7 100644
> --- a/xen/include/asm-x86/mm.h
> +++ b/xen/include/asm-x86/mm.h
> @@ -582,6 +582,17 @@ void *do_page_walk(struct vcpu *v, unsigned long addr);
>   /* Allocator functions for Xen pagetables. */
>   void *alloc_xen_pagetable(void);
>   void free_xen_pagetable(void *v);
> +mfn_t alloc_xen_pagetable_new(void);
> +void *map_xen_pagetable_new(mfn_t mfn);
> +void unmap_xen_pagetable_new(const void *v);
> +void free_xen_pagetable_new(mfn_t mfn);
> +
> +#define UNMAP_XEN_PAGETABLE_NEW(ptr)    \
> +    do {                                \
> +        unmap_xen_pagetable_new(ptr);   \
> +        (ptr) = NULL;                   \
> +    } while (0)
> +
>   l1_pgentry_t *virt_to_xen_l1e(unsigned long v);
>   
>   int __sync_local_execstate(void);
>

Patch
diff mbox series

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 7d4dd80a85..ca362ad638 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -119,6 +119,7 @@ 
 #include <xen/efi.h>
 #include <xen/grant_table.h>
 #include <xen/hypercall.h>
+#include <xen/mm.h>
 #include <asm/paging.h>
 #include <asm/shadow.h>
 #include <asm/page.h>
@@ -5020,22 +5021,50 @@  int mmcfg_intercept_write(
 }
 
 void *alloc_xen_pagetable(void)
+{
+    mfn_t mfn;
+
+    mfn = alloc_xen_pagetable_new();
+    ASSERT(!mfn_eq(mfn, INVALID_MFN));
+
+    return map_xen_pagetable_new(mfn);
+}
+
+void free_xen_pagetable(void *v)
+{
+    if ( system_state != SYS_STATE_early_boot )
+        free_xen_pagetable_new(virt_to_mfn(v));
+}
+
+mfn_t alloc_xen_pagetable_new(void)
 {
     if ( system_state != SYS_STATE_early_boot )
     {
         void *ptr = alloc_xenheap_page();
 
         BUG_ON(!hardware_domain && !ptr);
-        return ptr;
+        return virt_to_mfn(ptr);
     }
 
-    return mfn_to_virt(mfn_x(alloc_boot_pages(1, 1)));
+    return alloc_boot_pages(1, 1);
 }
 
-void free_xen_pagetable(void *v)
+void *map_xen_pagetable_new(mfn_t mfn)
 {
-    if ( system_state != SYS_STATE_early_boot )
-        free_xenheap_page(v);
+    return mfn_to_virt(mfn_x(mfn));
+}
+
+/* v can point to an entry within a table or be NULL */
+void unmap_xen_pagetable_new(const void *v)
+{
+    /* XXX still using xenheap page, no need to do anything.  */
+}
+
+/* mfn can be INVALID_MFN */
+void free_xen_pagetable_new(mfn_t mfn)
+{
+    if ( system_state != SYS_STATE_early_boot && !mfn_eq(mfn, INVALID_MFN) )
+        free_xenheap_page(mfn_to_virt(mfn_x(mfn)));
 }
 
 static DEFINE_SPINLOCK(map_pgdir_lock);
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index 9d2b833579..76593fe9e7 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -582,6 +582,17 @@  void *do_page_walk(struct vcpu *v, unsigned long addr);
 /* Allocator functions for Xen pagetables. */
 void *alloc_xen_pagetable(void);
 void free_xen_pagetable(void *v);
+mfn_t alloc_xen_pagetable_new(void);
+void *map_xen_pagetable_new(mfn_t mfn);
+void unmap_xen_pagetable_new(const void *v);
+void free_xen_pagetable_new(mfn_t mfn);
+
+#define UNMAP_XEN_PAGETABLE_NEW(ptr)    \
+    do {                                \
+        unmap_xen_pagetable_new(ptr);   \
+        (ptr) = NULL;                   \
+    } while (0)
+
 l1_pgentry_t *virt_to_xen_l1e(unsigned long v);
 
 int __sync_local_execstate(void);