Re: [PATCH v3 8/9] xen/riscv: page table handling

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.08.2024 11:33, oleksii.kurochko@xxxxxxxxx wrote:
> On Tue, 2024-07-30 at 16:22 +0200, Jan Beulich wrote:
>> On 24.07.2024 17:31, Oleksii Kurochko wrote:
>>> --- a/xen/arch/riscv/include/asm/page.h
>>> +++ b/xen/arch/riscv/include/asm/page.h
>>> @@ -34,6 +34,7 @@
>>>  #define PTE_LEAF_DEFAULT            (PTE_VALID | PTE_READABLE |
>>> PTE_WRITABLE)
>>>  #define PTE_TABLE                   (PTE_VALID)
>>>  
>>> +#define PAGE_HYPERVISOR_RO          (PTE_VALID | PTE_READABLE)
>>>  #define PAGE_HYPERVISOR_RW          (PTE_VALID | PTE_READABLE |
>>> PTE_WRITABLE)
>>
>> No PAGE_HYPERVISOR_RX?
> I haven't used it at the moment, so I haven't provided it.

I'm inclined to suggest to put it there right away. You will need it
rather sooner than later.

>>> +    unsigned long pbmt:2;
>>> +    unsigned long n:1;
>>> +#elif RV_STAGE1_MODE == SATP_MODE_SV48
>>> +    unsigned long ppn0:9;
>>> +    unsigned long ppn1:9;
>>> +    unsigned long ppn2:9;
>>> +    unsigned long ppn3:17;
>>> +    unsigned long rsw2:7;
>>> +    unsigned long pbmt:2;
>>> +    unsigned long n:1;
>>> +#else
>>> +#error "Add proper bits for SATP_MODE"
>>> +#endif
>>> +} pt_t;
>>
>> I can't spot a use anywhere of e.g. ppn0. Would be nice to understand
>> in
>> what contexts these bitfields are going to be used. I notice you
>> specifically
>> don't use them in e.g. pte_is_table().
> I don't use them at the moment. I just introduced them for the possible
> future using. I can re-check what of them I am using in my private
> branches and come up here only with that one which are really used.

Just to clarify: If you need any of the bitfields, then of course you
want to introduce all of them, properly named. Yet with the PTE_*
constants I'm wondering whether really you need them in addition.

>>> +/* Sanity check of the entry */
>>> +static bool xen_pt_check_entry(pte_t entry, mfn_t mfn, unsigned
>>> int level,
>>> +                               unsigned int flags)
>>
>> The comment wants extending to indicate what the parameters mean wrt
>> what
>> is going to be checked. For example, ...
>>
>>> +{
>>> +    /* Sanity check when modifying an entry. */
>>> +    if ( mfn_eq(mfn, INVALID_MFN) )
>>
>> ... it's unclear to me why incoming INVALID_MFN would indicate
>> modification
>> of an entry, whereas further down _PAGE_PRESENT supposedly indicates
>> insertion.
> The statement inside if isn't correct. It should be:
>    if ( (flags & _PAGE_PRESENT) && mfn_eq(mfn, INVALID_MFN) )
> 
> INVALID_MFN indicates modification because of how xen_pt_update() is
> used:
>    int map_pages_to_xen(unsigned long virt,
>                         mfn_t mfn,
>                         unsigned long nr_mfns,
>                         unsigned int flags)
>    {
>        return xen_pt_update(virt, mfn, nr_mfns, flags);
>    }
>    
>    int __init populate_pt_range(unsigned long virt, unsigned long nr_mfns)
>    {
>        return xen_pt_update(virt, INVALID_MFN, nr_mfns, _PAGE_PRESENT);
>    }
>    
>    int destroy_xen_mappings(unsigned long s, unsigned long e)
>    {
>        ASSERT(IS_ALIGNED(s, PAGE_SIZE));
>        ASSERT(IS_ALIGNED(e, PAGE_SIZE));
>        ASSERT(s <= e);
>        return xen_pt_update(s, INVALID_MFN, (e - s) >> PAGE_SHIFT, 0);
>    }
>    
>    int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int
>    nf)
>    {
>        ASSERT(IS_ALIGNED(s, PAGE_SIZE));
>        ASSERT(IS_ALIGNED(e, PAGE_SIZE));
>        ASSERT(s <= e);
>        return xen_pt_update(s, INVALID_MFN, (e - s) >> PAGE_SHIFT, nf);
>    }
> 
> The idea is the following:
>   the MFN is not valid and we are not populating page table. This means
> we either modify entry or remove an entry.

Right. My request stands though: The comment ahead of the function wants
extending to state how it is to be used correctly.

>>> +    mfn = pte_get_mfn(*entry);
>>> +
>>> +    xen_unmap_table(*table);
>>> +    *table = xen_map_table(mfn);
>>> +
>>> +    return XEN_TABLE_NORMAL_PAGE;
>>> +}
>>
>> Normal page? Not normal table?
> It just mean that this points not to super_page so potenially the in
> the next one table will have an entry that would be normal page.

Or a normal page table, if you haven't reached leaf level yet. IOW maybe
better XEN_TABLE_NORMAL, covering both cases?

>>> +    unsigned int target = arch_target;
>>> +    pte_t *table;
>>> +    /*
>>> +     * The intermediate page tables are read-only when the MFN is
>>> not valid
>>> +     * This means we either modify permissions or remove an entry.
>>> +     */
>>> +    bool read_only = mfn_eq(mfn, INVALID_MFN);
>>
>> I'm afraid I can't make a connection between the incoming MFN being
>> INVALID_MFN and intermediate tables being intended to remain
>> unaltered.
> 
> It is becuase of xen_pt_update() is used:
>    int __init populate_pt_range(unsigned long virt, unsigned long
>    nr_mfns)
>    {
>        return xen_pt_update(virt, INVALID_MFN, nr_mfns, _PAGE_PRESENT);
>    }
> So if pt is only populated then they are read_only and so they shouldn't
> be allocated what means ptes are only or being removed or modified.

Like above, such special assumptions would better be put in a comment.

>>> +int map_pages_to_xen(unsigned long virt,
>>> +                     mfn_t mfn,
>>> +                     unsigned long nr_mfns,
>>> +                     unsigned int flags)
>>> +{
>>> +    return xen_pt_update(virt, mfn, nr_mfns, flags);
>>> +}
>>
>> Why this wrapping of two functions taking identical arguments?
> map_pages_to_xen() sounds more clear regarding the way how it should be
> used.
> 
> xen_pt_update() will be also used inside other functions. Look at the
> example above.

They could as well use map_pages_to_xen() then? Or else the wrapper may
want to check (assert) that it is _not_ called with one of the special
case arguments that xen_pt_update() knows how to deal with?

Jan