Re: [for 4.22 v5 10/18] xen/riscv: implement p2m_set_range()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.11.2025 16:28, Oleksii Kurochko wrote:
> 
> On 11/17/25 9:56 AM, Jan Beulich wrote:
>>>>> +#define P2M_MAX_ROOT_LEVEL 4
>>>>> +
>>>>> +#define P2M_DECLARE_OFFSETS(var, addr) \
>>>>> +    unsigned int var[P2M_MAX_ROOT_LEVEL] = {-1};\
>>>>> +    for ( unsigned int i = 0; i <= gstage_root_level; i++ ) \
>>>>> +        var[i] = calc_offset(i, addr);
>>>> This surely is more than just "declare", and it's dealing with all levels 
>>>> no
>>>> matter whether you actually will use all offsets.
>>> I will rename|P2M_DECLARE_OFFSETS| to|P2M_BUILD_LEVEL_OFFSETS()|.
>>>
>>> But how can I know which offset I will actually need to use?
>>> If we take the following loop as an example:
>>>     |for( level = P2M_ROOT_LEVEL; level > target; level-- ) { ||/* ||* 
>>> Don't try to allocate intermediate page tables if the mapping ||* is 
>>> about to be removed. ||*/ ||rc = p2m_next_level(p2m, 
>>> !removing_mapping, ||level, &table, offsets[level]); ||... ||} |It 
>>> walks from|P2M_ROOT_LEVEL| down to|target|, where|target| is determined at 
>>> runtime.
>>>
>>> If you mean that, for example, when the G-stage mode is Sv39, there is no 
>>> need to allocate
>>> an array with 4 entries (or 5 entries if we consider Sv57, so 
>>> P2M_MAX_ROOT_LEVEL should be
>>> updated), because Sv39 only uses 3 page table levels — then yes, in theory 
>>> it could be
>>> smaller. But I don't think it is a real issue if the|offsets[]| array on 
>>> the stack has a
>>> few extra unused entries.
>>>
>>> If preferred, Icould allocate the array dynamically based 
>>> on|gstage_root_level|.
>>> Would that be better?
>> Having a few unused entries isn't a big deal imo. What I'm not happy with 
>> here is
>> that you may_initialize_ more entries than actually needed. I have no good
>> suggestion within the conceptual framework you use for page walking (the same
>> issue iirc exists in host page table walks, just that the calculations there 
>> are
>> cheaper).
> 
> The loop inside|P2M_DECLARE_OFFSETS()| uses|gstage_root_level|, so only the 
> entries that
> are actually going to be used are initialized.
> 
> You probably mean that it’s possible we don’t need to walk all the tables 
> because a
> leaf page-table entry appears earlier than the L0 page table for some gfns?

Yes.

> IMO, it’s not
> really a big deal, because at worst we just spend some time calculating 
> something that
> isn’t actually needed, but considering that it will be just extra 2 calls in 
> the worst case
> (when mapping is 1g, for no reason we calculated offsets for L1 and L0) of 
> calc_offset()
> it won't affect performance too much.

Well, it's your call in the end.

Jan