Re: [Xen-devel] [PATCH 1/2] x86/mm: factor out the code for shattering an l3 PTE

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 07/12/2019 19:04, Julien Grall wrote:
> Hi Hongyan,
>
> On 06/12/2019 15:53, Hongyan Xia wrote:
>> map_pages_to_xen and modify_xen_mappings are performing almost exactly
>> the same operations when shattering an l3 PTE, the only difference
>> being whether we want to flush.
>>
>> Signed-off-by: Hongyan Xia <hongyxia@xxxxxxxxxx>
>> ---
>>   xen/arch/x86/mm.c | 85 ++++++++++++++++++++++-------------------------
>>   1 file changed, 40 insertions(+), 45 deletions(-)
>>
>> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
>> index 7d4dd80a85..42aaaa1083 100644
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -5151,6 +5151,43 @@ l1_pgentry_t *virt_to_xen_l1e(unsigned long v)
>>                            flush_area_local((const void *)v, f) : \
>>                            flush_area_all((const void *)v, f))
>>   +/* Shatter an l3 entry and populate l2. If virt is passed in, also
>> do flush. */
>> +static void shatter_l3e(l3_pgentry_t *pl3e, l2_pgentry_t *l2t,
>> +        unsigned long virt, bool locking)
>> +{
>> +    unsigned int i;
>> +    l3_pgentry_t ol3e = *pl3e;
>> +
>> +    for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++ )
>> +        l2e_write(l2t + i,
>> +                  l2e_from_pfn(l3e_get_pfn(ol3e) +
>> +                               (i << PAGETABLE_ORDER),
>> +                               l3e_get_flags(ol3e)));
>
> I understand this is just a factor out of the current code, but the
> code feels wrong (at least in theory) and wasteful.
>
> You would allocate the L2 table, prepare it and then free it if the L3
> entry has _PAGE_PRESENT or _PAGE_PSE cleared.
>
> Also, in theory, there is nothing preventing the l3 flags to be
> modified behind your back. So you could end up to generate the l2
> entries with the old l3 flags.
>
> In nutshell, it feels to me that the shattering/allocation should
> happen with the lock taken. This would also allow you to not allocate
> the l2 table when you are removing the page.

The existing behaviour is deliberate and most likely wants to remain.

It makes adjustments safe to concurrent modifications, while reducing
the critical section of the lock to only the alteration of the live PTEs.

We don't expect concurrent conflicting updates to these pagetables at
all, but extending the locked region around the memory allocation and
writing the new pagetable is a bottlekneck to parallel updates of
independent addresses.

~Andrew

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