Re: [PATCH v2 14/17] xen/riscv: implement p2m_next_level()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.07.2025 11:42, Oleksii Kurochko wrote:
> On 7/16/25 6:12 PM, Jan Beulich wrote:
>> On 16.07.2025 17:53, Oleksii Kurochko wrote:
>>> On 7/16/25 1:43 PM, Jan Beulich wrote:
>>>> On 16.07.2025 13:32, Oleksii Kurochko wrote:
>>>>> On 7/2/25 10:35 AM, Jan Beulich wrote:
>>>>>> On 10.06.2025 15:05, Oleksii Kurochko wrote:
>>>>>>> --- a/xen/arch/riscv/p2m.c
>>>>>>> +++ b/xen/arch/riscv/p2m.c
>>>>>>> @@ -387,6 +387,17 @@ static inline bool p2me_is_valid(struct p2m_domain 
>>>>>>> *p2m, pte_t pte)
>>>>>>>         return p2m_type_radix_get(p2m, pte) != p2m_invalid;
>>>>>>>     }
>>>>>>>     
>>>>>>> +/*
>>>>>>> + * pte_is_* helpers are checking the valid bit set in the
>>>>>>> + * PTE but we have to check p2m_type instead (look at the comment above
>>>>>>> + * p2me_is_valid())
>>>>>>> + * Provide our own overlay to check the valid bit.
>>>>>>> + */
>>>>>>> +static inline bool p2me_is_mapping(struct p2m_domain *p2m, pte_t pte)
>>>>>>> +{
>>>>>>> +    return p2me_is_valid(p2m, pte) && (pte.pte & PTE_ACCESS_MASK);
>>>>>>> +}
>>>>>> Same question as on the earlier patch - does P2M type apply to 
>>>>>> intermediate
>>>>>> page tables at all? (Conceptually it shouldn't.)
>>>>> It doesn't matter whether it is an intermediate page table or a leaf PTE 
>>>>> pointing
>>>>> to a page — PTE should be valid. Considering that in the current 
>>>>> implementation
>>>>> it’s possible for PTE.v = 0 but P2M.v = 1, it is better to check P2M.v 
>>>>> instead
>>>>> of PTE.v.
>>>> I'm confused by this reply. If you want to name 2nd level page table 
>>>> entries
>>>> P2M - fine (but unhelpful). But then for any memory access there's only one
>>>> of the two involved: A PTE (Xen accesses) or a P2M (guest accesses). Hence
>>>> how can there be "PTE.v = 0 but P2M.v = 1"?
>>> I think I understand your confusion, let me try to rephrase.
>>>
>>> The reason for having both|p2m_is_valid()| and|pte_is_valid()| is that I 
>>> want to
>>> have the ability to use the P2M PTE valid bit to track which pages were 
>>> accessed
>>> by a vCPU, so that cleaning and invalidating RAM associated with the guest 
>>> vCPU
>>> won't be too expensive, for example.
>> I don't know what you're talking about here.
> 
> https://gitlab.com/xen-project/xen/-/blob/staging/xen/arch/arm/mmu/p2m.c#L1649

How does that Arm function matter here? Aiui you don't need anything like that
in RISC-V, as there caches don't need disabling temporarily.

>>> In this case, the P2M PTE valid bit will be set to 0, but the P2M PTE type 
>>> bits
>>> will be set to something other than|p2m_invalid| (even for a table entries),
>>> so when an MMU fault occurs, we can properly resolve it.
>>>
>>> So, if the P2M PTE type (what|p2m_is_valid()| checks) is set 
>>> to|p2m_invalid|, it
>>> means that the valid bit (what|pte_is_valid()| checks) should be set to 0, 
>>> so
>>> the P2M PTE is genuinely invalid.
>>>
>>> It could also be the case that the P2M PTE type isn't|p2m_invalid (and P2M 
>>> PTE valid will be intentionally set to 0 to have
>>> ability to track which pages were accessed for the reason I wrote above)|, 
>>> and when MMU fault occurs we could
>>> properly handle it and set to 1 P2M PTE valid bit to 1...
>>>
>>>> An intermediate page table entry is something Xen controls entirely. Hence
>>>> it has no (guest induced) type.
>>> ... And actually it is a reason why it is needed to set a type even for an
>>> intermediate page table entry.
>>>
>>> I hope now it is a lit bit clearer what and why was done.
>> Sadly not. I still don't see what use the P2M type in of an intermediate page
>> table is going to be. It surely can't reliably describe all of the entries 
>> that
>> page table holds. Intermediate page tables and leaf pages are just too 
>> different
>> to share a concept like this, I think. That said, I'll be happy to be shown 
>> code
>> demonstrating the contrary.
> 
> Then it is needed to introduce new p2m_type_t - p2m_table and use it.
> Would it be better?
> 
> I still need some type to have ability to distinguish if p2m is valid or not 
> from
> p2m management and hardware point of view.
> If there is no need for such distinguish why all archs introduce p2m_invalid?
> Isn't enough just to use P2M PTE valid bit?

At least on x86 we don't tag intermediate page tables with P2M types. For
ordinary leaf entries the situation is different, as there may be varying
reasons why a PTE has its valid (on x86: present) bit cleared. Hence the
type is relevant there, just to know what to do when a page is accessed
through such a not-present PTE.

Jan