Re: [PATCH 11/15] xen/arm: Allow lpae_is_{table, mapping} helpers to work on invalid entry

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

Hi,

On 08/15/2018 08:13 PM, Stefano Stabellini wrote:
> On Tue, 14 Aug 2018, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 08/14/2018 10:33 PM, Stefano Stabellini wrote:
>>> On Mon, 16 Jul 2018, Julien Grall wrote:
>>>> Currently, lpae_is_{table, mapping} helpers will always return false on
>>>> entry with the valid bit unset. However, it would be useful to have them
>>>     ^entries
>>>
>>>> operating on any entry. For instance to store information in advance but
>>>> still request a fault.
>>>>
>>>> With that change, the p2m is now providing an overlay for *_is_{table,
>>>> mapping} that will check the valid bit of the entry.
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>>
>>>> ---
>>>>    xen/arch/arm/guest_walk.c  |  2 +-
>>>>    xen/arch/arm/mm.c          |  2 +-
>>>>    xen/arch/arm/p2m.c         | 22 ++++++++++++++++++----
>>>>    xen/include/asm-arm/lpae.h | 11 +++++++----
>>>>    4 files changed, 27 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
>>>> index e3e21bdad3..4a1b4cf2c8 100644
>>>> --- a/xen/arch/arm/guest_walk.c
>>>> +++ b/xen/arch/arm/guest_walk.c
>>>> @@ -566,7 +566,7 @@ static int guest_walk_ld(const struct vcpu *v,
>>>>         * PTE is invalid or holds a reserved entry (PTE<1:0> == x0)) or if
>>>> the PTE
>>>>         * maps a memory block at level 3 (PTE<1:0> == 01).
>>>>         */
>>>> -    if ( !lpae_is_mapping(pte, level) )
>>>> +    if ( !lpae_is_valid(pte) || !lpae_is_mapping(pte, level) )
>>>>            return -EFAULT;
>>>>          /* Make sure that the lower bits of the PTE's base address are
>>>> zero. */
>>>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>>>> index e3dafe5fd7..52e57fef2f 100644
>>>> --- a/xen/arch/arm/mm.c
>>>> +++ b/xen/arch/arm/mm.c
>>>> @@ -996,7 +996,7 @@ static int create_xen_entries(enum xenmap_operation
>>>> op,
>>>>        for(; addr < addr_end; addr += PAGE_SIZE, mfn = mfn_add(mfn, 1))
>>>>        {
>>>>            entry = &xen_second[second_linear_offset(addr)];
>>>> -        if ( !lpae_is_table(*entry, 2) )
>>>> +        if ( !lpae_is_valid(*entry) || !lpae_is_table(*entry, 2) )
>>>>            {
>>>>                rc = create_xen_table(entry);
>>>>                if ( rc < 0 ) {
>>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>>> index ec3fdcb554..07925a1be4 100644
>>>> --- a/xen/arch/arm/p2m.c
>>>> +++ b/xen/arch/arm/p2m.c
>>>> @@ -219,6 +219,20 @@ static p2m_access_t p2m_mem_access_radix_get(struct
>>>> p2m_domain *p2m, gfn_t gfn)
>>>>            return radix_tree_ptr_to_int(ptr);
>>>>    }
>>>>    +/*
>>>> + * lpae_is_* helpers don't check whether the valid bit is set in the
>>>> + * PTE. Provide our own overlay to check the valid bit.
>>>> + */
>>>> +static inline bool p2m_is_mapping(lpae_t pte, unsigned int level)
>>>> +{
>>>> +    return lpae_is_valid(pte) && lpae_is_mapping(pte, level);
>>>> +}
>>>> +
>>>> +static inline bool p2m_is_superpage(lpae_t pte, unsigned int level)
>>>> +{
>>>> +    return lpae_is_valid(pte) && lpae_is_superpage(pte, level);
>>>> +}
>>>
>>> I like the idea, but it would be clearer to me if they were called
>>> lpae_is_valid_mapping and lpae_is_valid_superpage respectively. What do
>>> you think?
>>>   > Also, we might as well move them to lpae.h and use them from mm.c and
>>> guest_walk.c as appropriate?
>>
>> lpae.h is not suitable because as I said in the commit message each page-table
>> (stage-1, stage-2) may have a different view of what "valid" means.
>>
>> At the moment, that view is the same but it is not going to stay long like
>> that. Hence the reason of prefixing with p2m_. They are specific to the p2m.
>> This is giving us some more liberty in the future while making the lpae code a
>> bit more generic.
>>
>> In guest_walk.c there are only one user, so the introduction of an helper is
>> quite limited there.
> 
> I see, so by "p2m_is_mapping" you meant specifically
> "stage2_is_mapping", right? That makes sense now.

Yes. We use the term "p2m" everywhere in Xen to refer to stage-2 
page-tables (see lpae_p2m_t). So it makes sense to prefix the helpers 
with "p2m_" here.

Cheers,

-- 
Julien Grall

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