Re: [PATCH for 4.20? v3 1/3] xen/riscv: implement software page table walking

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3382 bytes --]


On 2/12/25 12:27 PM, Jan Beulich wrote:
> On 12.02.2025 12:13, Oleksii Kurochko wrote:
>> On 2/10/25 5:32 PM, Jan Beulich wrote:
>>> On 07.02.2025 14:13, Oleksii Kurochko wrote:
>>>> --- a/xen/arch/riscv/pt.c
>>>> +++ b/xen/arch/riscv/pt.c
>>>> @@ -185,6 +185,57 @@ static int pt_next_level(bool alloc_tbl, pte_t **table, unsigned int offset)
>>>>        return XEN_TABLE_NORMAL;
>>>>    }
>>>>    
>>>> +/*
>>>> + * _pt_walk() performs software page table walking and returns the pte_t of
>>>> + * a leaf node or the leaf-most not-present pte_t if no leaf node is found
>>>> + * for further analysis.
>>>> + * Additionally, _pt_walk() returns the level of the found pte.
>>> That's optional, which I think wants expressing here.
>>>
>>>> + */
>>>> +static pte_t *_pt_walk(vaddr_t va, unsigned int *pte_level)
>>>> +{
>>>> +    const mfn_t root = get_root_page();
>>>> +    unsigned int level;
>>>> +    pte_t *table;
>>>> +
>>>> +    DECLARE_OFFSETS(offsets, va);
>>>> +
>>>> +    table = map_table(root);
>>> This mapping operation doesn't look to have a counterpart. Aiui ...
>>>
>>>> +    /*
>>>> +     * Find `table` of an entry which corresponds to `va` by iterating for each
>>>> +     * page level and checking if the entry points to a next page table or
>>>> +     * to a page.
>>>> +     *
>>>> +     * Two cases are possible:
>>>> +     * - ret == XEN_TABLE_SUPER_PAGE means that the entry was found;
>>>> +     *   (Despite the name) XEN_TABLE_SUPER_PAGE also covers 4K mappings. If
>>>> +     *   pt_next_level() is called for page table level 0, it results in the
>>>> +     *   entry being a pointer to a leaf node, thereby returning
>>>> +     *   XEN_TABLE_SUPER_PAGE, despite of the fact this leaf covers 4k mapping.
>>>> +     * - ret == XEN_TABLE_MAP_NONE means that requested `va` wasn't actually
>>>> +     *   mapped.
>>>> +     */
>>>> +    for ( level = HYP_PT_ROOT_LEVEL; ; --level )
>>>> +    {
>>>> +        int ret = pt_next_level(false, &table, offsets[level]);
>>> ... the mapping may be replaced here, but a new mapping will then still
>>> be held by this function and ...
>>>
>>>> +        if ( ret == XEN_TABLE_MAP_NONE || ret == XEN_TABLE_SUPER_PAGE )
>>>> +            break;
>>>> +
>>>> +        ASSERT(level);
>>>> +    }
>>>> +
>>>> +    if ( pte_level )
>>>> +        *pte_level = level;
>>>> +
>>>> +    return table + offsets[level];
>>>> +}
>>> ... the final one then be transferred to the caller.
>>>
>>>> +pte_t pt_walk(vaddr_t va, unsigned int *pte_level)
>>>> +{
>>>> +    return *_pt_walk(va, pte_level);
>>>> +}
>>> Hence aiui there needs to be an unmap operation here.
>> As _pt_walk() returns page table entry, it is needed to transform entry to table.
>>
>> Do we have any function in Xen for that?
> I don't understand. Both unmap_domain_page() and pmap_unmap() ignore
> the low bits of the passed in address.

Missed that. Then it is safe to use unmap_table() with page table entry.

Thanks.

~ Oleksii

>> Or the best I can do is:
>>     pte_t *table = *_pt_walk(va, pte_level) - TABLE_OFFSET(pt_linear_offset(*pte_level, va)
>> (of course, it is needed to check if pte_level isn't null and do some extra actions if it is NULL)
>>
>> As an option, as all page tables are PAGE_SIZE aligned, we could use PAGE_OFFSET():
>>    pte_t *entry = _pt_walk(va, pte_level);
>>    pte_t *table = PAGE_OFFSET(entry);
>>
>> ~ Oleksii
>>
>>

[-- Attachment #2: Type: text/html, Size: 4803 bytes --]