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

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

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


On 2/19/25 12:14 PM, Jan Beulich wrote:
> On 12.02.2025 17:50, Oleksii Kurochko wrote:
>> --- a/xen/arch/riscv/pt.c
>> +++ b/xen/arch/riscv/pt.c
>> @@ -185,6 +185,68 @@ 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 by using
>> + * `pte_level` argument.
>> + * `pte_level` is optional, set `pte_level`=NULL if a caller doesn't need
>> + * the level of the found pte.
> How about this, reducing redundancy a little?
>
>   * _pt_walk() can optionally return the level of the found pte. Pass NULL
>   * for `pte_level` if this information isn't needed.
>
>> +pte_t pt_walk(vaddr_t va, unsigned int *pte_level)
>> +{
>> +    pte_t *entry = _pt_walk(va, pte_level);
>> +    pte_t pte = *entry;
>> +
>> +    unmap_table(entry);
>> +
>> +    return pte;
>> +}
> "entry" especially in this context is ambiguous. I would expect a variable of
> this name to be of type pte_t, not pte_t *. How about "ptep"?

Agree with both your suggestions, it would be better to use `ptep instead of `entry`
and rephrase the comment.

>
> Preferably with these adjustments, which I'd be fine making while committing,
> Reviewed-by: Jan Beulich<jbeulich@suse.com>
>
> Considering the 4.20? tag you'll need to decide whether you still want this
> in before the release.

Considering that it is still needed a new version for patch3 of this patch series and
that the mentioned issues aren't affected no one, lets consider the full patch series for
4.21.

Thanks.

~ Oleksii

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