diff mbox series

[5/5] x86_64/mm: map and unmap page tables in destroy_m2p_mapping

Message ID 7143c2a1e0c7ca46b3ace329d7dcab85e0b5c87c.1584955616.git.hongyxia@amazon.com (mailing list archive)
State Superseded
Headers show
Series use new API for Xen page tables | expand

Commit Message

Hongyan Xia March 23, 2020, 9:41 a.m. UTC 
From: Wei Liu <wei.liu2@citrix.com>

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

Comments

Jan Beulich April 1, 2020, 12:40 p.m. UTC | #1 
On 23.03.2020 10:41, Hongyan Xia wrote:
> @@ -297,26 +298,33 @@ static void destroy_m2p_mapping(struct mem_hotadd_info *info)
>              continue;
>          }
>  
> -        l2_ro_mpt = l3e_to_l2e(l3_ro_mpt[l3_table_offset(va)]);
> +        l2_ro_mpt = map_l2t_from_l3e(l3_ro_mpt[l3_table_offset(va)]);
>          if (!(l2e_get_flags(l2_ro_mpt[l2_table_offset(va)]) & _PAGE_PRESENT))
>          {
>              i = ( i & ~((1UL << (L2_PAGETABLE_SHIFT - 3)) - 1)) +
>                      (1UL << (L2_PAGETABLE_SHIFT - 3)) ;
> +            UNMAP_DOMAIN_PAGE(l2_ro_mpt);
>              continue;
>          }
>  
>          pt_pfn = l2e_get_pfn(l2_ro_mpt[l2_table_offset(va)]);
>          if ( hotadd_mem_valid(pt_pfn, info) )
>          {
> +            l2_pgentry_t *l2t;
> +
>              destroy_xen_mappings(rwva, rwva + (1UL << L2_PAGETABLE_SHIFT));
>  
> -            l2_ro_mpt = l3e_to_l2e(l3_ro_mpt[l3_table_offset(va)]);
> -            l2e_write(&l2_ro_mpt[l2_table_offset(va)], l2e_empty());
> +            l2t = map_l2t_from_l3e(l3_ro_mpt[l3_table_offset(va)]);

Why a 2nd mapping of the same L3 entry that you've already mapped
into l2_ro_mpt?

> +            l2e_write(&l2t[l2_table_offset(va)], l2e_empty());
> +            UNMAP_DOMAIN_PAGE(l2t);

If this then weren't to go away, it should again be the lowercase
variant imo, as the variable's scope ends here.

Jan
Hongyan Xia April 7, 2020, 4:23 p.m. UTC | #2 
On Wed, 2020-04-01 at 14:40 +0200, Jan Beulich wrote:
> On 23.03.2020 10:41, Hongyan Xia wrote:
> > @@ -297,26 +298,33 @@ static void destroy_m2p_mapping(struct
> > mem_hotadd_info *info)
> >              continue;
> >          }
> >  
> > -        l2_ro_mpt = l3e_to_l2e(l3_ro_mpt[l3_table_offset(va)]);
> > +        l2_ro_mpt =
> > map_l2t_from_l3e(l3_ro_mpt[l3_table_offset(va)]);
> >          if (!(l2e_get_flags(l2_ro_mpt[l2_table_offset(va)]) &
> > _PAGE_PRESENT))
> >          {
> >              i = ( i & ~((1UL << (L2_PAGETABLE_SHIFT - 3)) - 1)) +
> >                      (1UL << (L2_PAGETABLE_SHIFT - 3)) ;
> > +            UNMAP_DOMAIN_PAGE(l2_ro_mpt);
> >              continue;
> >          }
> >  
> >          pt_pfn = l2e_get_pfn(l2_ro_mpt[l2_table_offset(va)]);
> >          if ( hotadd_mem_valid(pt_pfn, info) )
> >          {
> > +            l2_pgentry_t *l2t;
> > +
> >              destroy_xen_mappings(rwva, rwva + (1UL <<
> > L2_PAGETABLE_SHIFT));
> >  
> > -            l2_ro_mpt =
> > l3e_to_l2e(l3_ro_mpt[l3_table_offset(va)]);
> > -            l2e_write(&l2_ro_mpt[l2_table_offset(va)],
> > l2e_empty());
> > +            l2t =
> > map_l2t_from_l3e(l3_ro_mpt[l3_table_offset(va)]);
> 
> Why a 2nd mapping of the same L3 entry that you've already mapped
> into l2_ro_mpt?
> > +            l2e_write(&l2t[l2_table_offset(va)], l2e_empty());
> > +            UNMAP_DOMAIN_PAGE(l2t);
> 
> If this then weren't to go away, it should again be the lowercase
> variant imo, as the variable's scope ends here.

Hmm, I don't see a reason why l2_ro_mpt needs to be mapped again either
(and don't see why it was re-derived in the original code), so yes, I
think the map and unmap can just be dropped. Will revise.

Hongyan
diff mbox series

Patch

diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index 71c84ac593..6a0ffe088b 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -275,7 +275,8 @@  static void destroy_m2p_mapping(struct mem_hotadd_info *info)
     unsigned long i, va, rwva;
     unsigned long smap = info->spfn, emap = info->epfn;
 
-    l3_ro_mpt = l4e_to_l3e(idle_pg_table[l4_table_offset(RO_MPT_VIRT_START)]);
+    l3_ro_mpt = map_l3t_from_l4e(
+                    idle_pg_table[l4_table_offset(RO_MPT_VIRT_START)]);
 
     /*
      * No need to clean m2p structure existing before the hotplug
@@ -297,26 +298,33 @@  static void destroy_m2p_mapping(struct mem_hotadd_info *info)
             continue;
         }
 
-        l2_ro_mpt = l3e_to_l2e(l3_ro_mpt[l3_table_offset(va)]);
+        l2_ro_mpt = map_l2t_from_l3e(l3_ro_mpt[l3_table_offset(va)]);
         if (!(l2e_get_flags(l2_ro_mpt[l2_table_offset(va)]) & _PAGE_PRESENT))
         {
             i = ( i & ~((1UL << (L2_PAGETABLE_SHIFT - 3)) - 1)) +
                     (1UL << (L2_PAGETABLE_SHIFT - 3)) ;
+            UNMAP_DOMAIN_PAGE(l2_ro_mpt);
             continue;
         }
 
         pt_pfn = l2e_get_pfn(l2_ro_mpt[l2_table_offset(va)]);
         if ( hotadd_mem_valid(pt_pfn, info) )
         {
+            l2_pgentry_t *l2t;
+
             destroy_xen_mappings(rwva, rwva + (1UL << L2_PAGETABLE_SHIFT));
 
-            l2_ro_mpt = l3e_to_l2e(l3_ro_mpt[l3_table_offset(va)]);
-            l2e_write(&l2_ro_mpt[l2_table_offset(va)], l2e_empty());
+            l2t = map_l2t_from_l3e(l3_ro_mpt[l3_table_offset(va)]);
+            l2e_write(&l2t[l2_table_offset(va)], l2e_empty());
+            UNMAP_DOMAIN_PAGE(l2t);
         }
         i = ( i & ~((1UL << (L2_PAGETABLE_SHIFT - 3)) - 1)) +
               (1UL << (L2_PAGETABLE_SHIFT - 3));
+        UNMAP_DOMAIN_PAGE(l2_ro_mpt);
     }
 
+    UNMAP_DOMAIN_PAGE(l3_ro_mpt);
+
     destroy_compat_m2p_mapping(info);
 
     /* Brute-Force flush all TLB */