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

2020-04-07 Thread Hongyan Xia 
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

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

2020-04-01 Thread Jan Beulich 
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

[Xen-devel] [PATCH 5/5] x86_64/mm: map and unmap page tables in destroy_m2p_mapping

2020-03-23 Thread Hongyan Xia 
From: Wei Liu 

Signed-off-by: Wei Liu 
Reviewed-by: Hongyan Xia 
---
 xen/arch/x86/x86_64/mm.c | 16 
 1 file changed, 12 insertions(+), 4 deletions(-)

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 */
-- 
2.24.1.AMZN


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel