Re: [Xen-devel] [PATCH MM-PART3 v2 10/12] xen/arm: mm: Rework Xen page-tables walk during update

[Julien Grall <julien.grall@arm.com>]

Hi Stefano,

On 6/12/19 11:52 PM, Stefano Stabellini wrote:
> On Tue, 14 May 2019, Julien Grall wrote:
>> Currently, xen_pt_update_entry() is only able to update the region covered
>> by xen_second (i.e 0 to 0x7fffffff).
>>
>> Because of the restriction we end to have multiple functions in mm.c
>> modifying the page-tables differently.
>>
>> Furthermore, we never walked the page-tables fully. This means that any
>> change in the layout may requires major rewrite of the page-tables code.
>>
>> Lastly, we have been quite lucky that no one ever tried to pass an address
>> outside this range because it would have blown-up.
>>
>> xen_pt_update_entry() is reworked to walk over the page-tables every
>> time. The logic has been borrowed from arch/arm/p2m.c and contain some
>> limitations for the time being:
>>      - Superpage cannot be shattered
>>      - Only level 3 (i.e 4KB) can be done
>>
>> Note that the parameter 'addr' has been renamed to 'virt' to make clear
>> we are dealing with a virtual address.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
>>
>> ---
>>      Changes in v2:
>>          - Add Andrii's reviewed-by
>> ---
>>   xen/arch/arm/mm.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++-------
>>   1 file changed, 106 insertions(+), 15 deletions(-)
>>
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index f5979f549b..9a40754f44 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -984,6 +984,53 @@ static void xen_unmap_table(const lpae_t *table)
>>       unmap_domain_page(table);
>>   }
>>   
>> +#define XEN_TABLE_MAP_FAILED 0
>> +#define XEN_TABLE_SUPER_PAGE 1
>> +#define XEN_TABLE_NORMAL_PAGE 2
> 
> Minor NIT: do we want to have XEN_TABLE_MAP_FAILED be -1 to follow the
> pattern that errors are < 0 ? Not important though.

The value of XEN_TABLE_* here does not matter, you can see it as an 
open-coded enum. This was borrowed from arm/p2m.c (which was based on 
x86/mm/p2m-pt.c).

For the time being, I would prefer to keep it as is because it makes 
easier to spot the difference with the p2m code. I can consider 
switching the two to enum afterwards.

> 
> 
>> +/*
>> + * Take the currently mapped table, find the corresponding entry,
>> + * and map the next table, if available.
>> + *
>> + * The read_only parameters indicates whether intermediate tables should
>> + * be allocated when not present.
> 
> I wonder if it would be a good idea to rename read_only to something
> more obviously connected to the idea that tables get created. Maybe
> create_missing? It would have to match the variable and comment added
> below in xen_pt_update_entry. I don't have a strong opinion on this.

Same as above here, the comment is a replicate of p2m.c

> 
> 
>> + * Return values:
>> + *  XEN_TABLE_MAP_FAILED: Either read_only was set and the entry
>> + *  was empty, or allocating a new page failed.
>> + *  XEN_TABLE_NORMAL_PAGE: next level mapped normally
>> + *  XEN_TABLE_SUPER_PAGE: The next entry points to a superpage.
>> + */
>> +static int xen_pt_next_level(bool read_only, unsigned int level,
>> +                             lpae_t **table, unsigned int offset)
>> +{
>> +    lpae_t *entry;
>> +    int ret;
>> +
>> +    entry = *table + offset;
>> +
>> +    if ( !lpae_is_valid(*entry) )
>> +    {
>> +        if ( read_only )
>> +            return XEN_TABLE_MAP_FAILED;
>> +
>> +        ret = create_xen_table(entry);
>> +        if ( ret )
>> +            return XEN_TABLE_MAP_FAILED;
>> +    }
>> +
>> +    ASSERT(lpae_is_valid(*entry));
> 
> Why the ASSERT just after the lpae_is_valid check above?

When the entry is invalid, the new page table will be allocated and the 
entry will be generated. The rest of the function will then be executed. 
The ASSERT() here confirms the entry we have in hand is valid in all the 
cases.

Cheers,

-- 
Julien Grall

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