diff mbox series

[v6,07/15] x86_64/mm: switch to new APIs in paging_init

Message ID 0655cc2d3dc27141ef102076c4ad390a37191b37.1587735799.git.hongyxia@amazon.com (mailing list archive)
State Superseded
Headers show
Series switch to domheap for Xen page tables | expand

Commit Message

Hongyan Xia April 24, 2020, 2:08 p.m. UTC 
From: Wei Liu <wei.liu2@citrix.com>

Map and unmap pages instead of relying on the direct map.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Signed-off-by: Hongyan Xia <hongyxia@amazon.com>
---
 xen/arch/x86/x86_64/mm.c | 43 ++++++++++++++++++++++++++++------------
 1 file changed, 30 insertions(+), 13 deletions(-)

Comments

Jan Beulich May 20, 2020, 9:46 a.m. UTC | #1 
On 24.04.2020 16:08, Hongyan Xia wrote:
> @@ -493,22 +494,28 @@ void __init paging_init(void)
>          if ( !(l4e_get_flags(idle_pg_table[l4_table_offset(va)]) &
>                _PAGE_PRESENT) )
>          {
> -            l3_pgentry_t *pl3t = alloc_xen_pagetable();
> +            l3_pgentry_t *pl3t;
> +            mfn_t l3mfn = alloc_xen_pagetable_new();
>  
> -            if ( !pl3t )
> +            if ( mfn_eq(l3mfn, INVALID_MFN) )
>                  goto nomem;
> +
> +            pl3t = map_domain_page(l3mfn);
>              clear_page(pl3t);
>              l4e_write(&idle_pg_table[l4_table_offset(va)],
> -                      l4e_from_paddr(__pa(pl3t), __PAGE_HYPERVISOR_RW));
> +                      l4e_from_mfn(l3mfn, __PAGE_HYPERVISOR_RW));
> +            unmap_domain_page(pl3t);

This can be moved up, and once it is you'll notice that you're
open-coding clear_domain_page(). I wonder whether I didn't spot
the same in other patches of this series.

Besides the previously raised point of possibly having an
allocation function that returns a mapping of the page right
away (not needed here) - are there many cases where allocation
of a new page table isn't accompanied by clearing the page? If
not, should the function perhaps do so (and then, once it has
a mapping anyway, it would be even more so natural to return
it for users wanting a mapping anyway)?

> @@ -662,6 +677,8 @@ void __init paging_init(void)
>      return;
>  
>   nomem:
> +    UNMAP_DOMAIN_PAGE(l2_ro_mpt);
> +    UNMAP_DOMAIN_PAGE(l3_ro_mpt);
>      panic("Not enough memory for m2p table\n");
>  }

I don't think this is a very useful addition.

Jan
Hongyan Xia May 27, 2020, 3:01 p.m. UTC | #2 
On Wed, 2020-05-20 at 11:46 +0200, Jan Beulich wrote:
> On 24.04.2020 16:08, Hongyan Xia wrote:
> > @@ -493,22 +494,28 @@ void __init paging_init(void)
> >          if ( !(l4e_get_flags(idle_pg_table[l4_table_offset(va)]) &
> >                _PAGE_PRESENT) )
> >          {
> > -            l3_pgentry_t *pl3t = alloc_xen_pagetable();
> > +            l3_pgentry_t *pl3t;
> > +            mfn_t l3mfn = alloc_xen_pagetable_new();
> >  
> > -            if ( !pl3t )
> > +            if ( mfn_eq(l3mfn, INVALID_MFN) )
> >                  goto nomem;
> > +
> > +            pl3t = map_domain_page(l3mfn);
> >              clear_page(pl3t);
> >              l4e_write(&idle_pg_table[l4_table_offset(va)],
> > -                      l4e_from_paddr(__pa(pl3t),
> > __PAGE_HYPERVISOR_RW));
> > +                      l4e_from_mfn(l3mfn, __PAGE_HYPERVISOR_RW));
> > +            unmap_domain_page(pl3t);
> 
> This can be moved up, and once it is you'll notice that you're
> open-coding clear_domain_page(). I wonder whether I didn't spot
> the same in other patches of this series.
> 
> Besides the previously raised point of possibly having an
> allocation function that returns a mapping of the page right
> away (not needed here) - are there many cases where allocation
> of a new page table isn't accompanied by clearing the page? If
> not, should the function perhaps do so (and then, once it has
> a mapping anyway, it would be even more so natural to return
> it for users wanting a mapping anyway)?

I grepped through all alloc_xen_pagetable(). Except the page shattering
logic in x86/mm.c where the whole page table page is written
immediately, all other call sites clear the page right away, so it is
useful to have a helper that clears it for you. I also looked at the
use of VA and MFN from the call. MFN is almost always needed while VA
is not, and if we bundle clearing into the alloc() itself, a lot of
call sites don't even need the VA.

Similar to what you suggested before, we can do:
void* alloc_map_clear_xen_pagetable(mfn_t* mfn)
which needs to be paired with an unmap call, of course.

> > @@ -662,6 +677,8 @@ void __init paging_init(void)
> >      return;
> >  
> >   nomem:
> > +    UNMAP_DOMAIN_PAGE(l2_ro_mpt);
> > +    UNMAP_DOMAIN_PAGE(l3_ro_mpt);
> >      panic("Not enough memory for m2p table\n");
> >  }
> 
> I don't think this is a very useful addition.

I was trying to avoid further mapping leaks in the panic path, but it
does not look like panic() does mappings, so these can be removed.

Hongyan
diff mbox series

Patch

diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index 35f9de4ad4..3cf699d27b 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -478,9 +478,10 @@  void __init paging_init(void)
 {
     unsigned long i, mpt_size, va;
     unsigned int n, memflags;
-    l3_pgentry_t *l3_ro_mpt;
+    l3_pgentry_t *l3_ro_mpt = NULL;
     l2_pgentry_t *pl2e = NULL, *l2_ro_mpt = NULL;
     struct page_info *l1_pg;
+    mfn_t l3_ro_mpt_mfn, l2_ro_mpt_mfn;
 
     /*
      * We setup the L3s for 1:1 mapping if host support memory hotplug
@@ -493,22 +494,28 @@  void __init paging_init(void)
         if ( !(l4e_get_flags(idle_pg_table[l4_table_offset(va)]) &
               _PAGE_PRESENT) )
         {
-            l3_pgentry_t *pl3t = alloc_xen_pagetable();
+            l3_pgentry_t *pl3t;
+            mfn_t l3mfn = alloc_xen_pagetable_new();
 
-            if ( !pl3t )
+            if ( mfn_eq(l3mfn, INVALID_MFN) )
                 goto nomem;
+
+            pl3t = map_domain_page(l3mfn);
             clear_page(pl3t);
             l4e_write(&idle_pg_table[l4_table_offset(va)],
-                      l4e_from_paddr(__pa(pl3t), __PAGE_HYPERVISOR_RW));
+                      l4e_from_mfn(l3mfn, __PAGE_HYPERVISOR_RW));
+            unmap_domain_page(pl3t);
         }
     }
 
     /* Create user-accessible L2 directory to map the MPT for guests. */
-    if ( (l3_ro_mpt = alloc_xen_pagetable()) == NULL )
+    l3_ro_mpt_mfn = alloc_xen_pagetable_new();
+    if ( mfn_eq(l3_ro_mpt_mfn, INVALID_MFN) )
         goto nomem;
+    l3_ro_mpt = map_domain_page(l3_ro_mpt_mfn);
     clear_page(l3_ro_mpt);
     l4e_write(&idle_pg_table[l4_table_offset(RO_MPT_VIRT_START)],
-              l4e_from_paddr(__pa(l3_ro_mpt), __PAGE_HYPERVISOR_RO | _PAGE_USER));
+              l4e_from_mfn(l3_ro_mpt_mfn, __PAGE_HYPERVISOR_RO | _PAGE_USER));
 
     /*
      * Allocate and map the machine-to-phys table.
@@ -591,12 +598,17 @@  void __init paging_init(void)
         }
         if ( !((unsigned long)pl2e & ~PAGE_MASK) )
         {
-            if ( (l2_ro_mpt = alloc_xen_pagetable()) == NULL )
+            UNMAP_DOMAIN_PAGE(l2_ro_mpt);
+
+            l2_ro_mpt_mfn = alloc_xen_pagetable_new();
+            if ( mfn_eq(l2_ro_mpt_mfn, INVALID_MFN) )
                 goto nomem;
+
+            l2_ro_mpt = map_domain_page(l2_ro_mpt_mfn);
             clear_page(l2_ro_mpt);
             l3e_write(&l3_ro_mpt[l3_table_offset(va)],
-                      l3e_from_paddr(__pa(l2_ro_mpt),
-                                     __PAGE_HYPERVISOR_RO | _PAGE_USER));
+                      l3e_from_mfn(l2_ro_mpt_mfn,
+                                   __PAGE_HYPERVISOR_RO | _PAGE_USER));
             pl2e = l2_ro_mpt;
             ASSERT(!l2_table_offset(va));
         }
@@ -608,13 +620,16 @@  void __init paging_init(void)
     }
 #undef CNT
 #undef MFN
+    UNMAP_DOMAIN_PAGE(l2_ro_mpt);
+    UNMAP_DOMAIN_PAGE(l3_ro_mpt);
 
     /* Create user-accessible L2 directory to map the MPT for compat guests. */
-    if ( (l2_ro_mpt = alloc_xen_pagetable()) == NULL )
+    l2_ro_mpt_mfn = alloc_xen_pagetable_new();
+    if ( mfn_eq(l2_ro_mpt_mfn, INVALID_MFN) )
         goto nomem;
-    compat_idle_pg_table_l2 = l2_ro_mpt;
-    clear_page(l2_ro_mpt);
-    pl2e = l2_ro_mpt;
+    compat_idle_pg_table_l2 = map_domain_page_global(l2_ro_mpt_mfn);
+    clear_page(compat_idle_pg_table_l2);
+    pl2e = compat_idle_pg_table_l2;
     /* Allocate and map the compatibility mode machine-to-phys table. */
     mpt_size = (mpt_size >> 1) + (1UL << (L2_PAGETABLE_SHIFT - 1));
     if ( mpt_size > RDWR_COMPAT_MPT_VIRT_END - RDWR_COMPAT_MPT_VIRT_START )
@@ -662,6 +677,8 @@  void __init paging_init(void)
     return;
 
  nomem:
+    UNMAP_DOMAIN_PAGE(l2_ro_mpt);
+    UNMAP_DOMAIN_PAGE(l3_ro_mpt);
     panic("Not enough memory for m2p table\n");
 }