On 21.04.2021 13:33, Hongyan Xia wrote: > On Tue, 2021-04-20 at 14:17 +0200, Jan Beulich wrote: >> On 06.04.2021 13:05, Hongyan Xia wrote: >>> @@ -5305,6 +5339,8 @@ int map_pages_to_xen( >>> pl1e = virt_to_xen_l1e(virt); >>> if ( pl1e == NULL ) >>> goto out; >>> + >>> + UNMAP_DOMAIN_PAGE(pl1e); >>> } >> >> Unmapping the page right after mapping it looks suspicious. I see >> that >> further down we have >> >> pl1e = l2e_to_l1e(*pl2e) + l1_table_offset(virt); >> >> but don't you need to also change that? Actually, you do in patch 2, >> but the latest then the double mapping should imo be avoided. > > I would say the code was already suspicious to begin with, since pl1e > would be overwritten immediately below even before this patch. The > purpose of the virt_to_xen_l1e() is only to populate the L1 table. > > Performance-wise the double map should be pretty harmless since the > mapping is in the cache, so I actually prefer it as is. Alternatively, > I can initialise pl1e to NULL at the beginning of the block and only do > the > > pl1e = l2e_to_l1e(*pl2e) + l1_table_offset(virt); > > when the pl1e is still NULL. If you are okay I will go with this.
I'd prefer this alternative, indeed, as it'll make the overall code look less odd. Albeit maybe not here, but in the subsequent patch. Jan
