Message ID | 7143c2a1e0c7ca46b3ace329d7dcab85e0b5c87c.1584955616.git.hongyxia@amazon.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | use new API for Xen page tables | expand |
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
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 --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 */