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

[Hongyan Xia <hx242@xen.org>]

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