Re: [PATCH v2 4/6] xen/riscv: introduce identity mapping

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.07.2023 10:53, Oleksii wrote:
> On Fri, 2023-07-07 at 12:51 +0200, Jan Beulich wrote:
>> On 07.07.2023 12:37, Oleksii wrote:
>>> On Thu, 2023-07-06 at 13:35 +0200, Jan Beulich wrote:
>>>> On 19.06.2023 15:34, Oleksii Kurochko wrote:
>>>>> --- a/xen/arch/riscv/mm.c
>>>>> +++ b/xen/arch/riscv/mm.c
>>>>> @@ -25,6 +25,12 @@ unsigned long __ro_after_init phys_offset;
>>>>>  #define LOAD_TO_LINK(addr) ((unsigned long)(addr) -
>>>>> phys_offset)
>>>>>  #define LINK_TO_LOAD(addr) ((unsigned long)(addr) +
>>>>> phys_offset)
>>>>>  
>>>>> +/*
>>>>> + * Should be removed as soon as enough headers will be merged
>>>>> for
>>>>> inclusion of
>>>>> + * <xen/lib.h>.
>>>>> + */
>>>>> +#define ARRAY_SIZE(arr)                (sizeof(arr) /
>>>>> sizeof((arr)[0]))
>>>>> +
>>>>>  /*
>>>>>   * It is expected that Xen won't be more then 2 MB.
>>>>>   * The check in xen.lds.S guarantees that.
>>>>> @@ -35,8 +41,10 @@ unsigned long __ro_after_init phys_offset;
>>>>>   *
>>>>>   * It might be needed one more page table in case when Xen
>>>>> load
>>>>> address
>>>>>   * isn't 2 MB aligned.
>>>>> + *
>>>>> + * (CONFIG_PAGING_LEVELS - 1) page tables are needed for
>>>>> identity
>>>>> mapping.
>>>>>   */
>>>>> -#define PGTBL_INITIAL_COUNT ((CONFIG_PAGING_LEVELS - 1) + 1)
>>>>> +#define PGTBL_INITIAL_COUNT ((CONFIG_PAGING_LEVELS - 1) * 2 +
>>>>> 1)
>>>>
>>>> How come the extra page (see the comment sentence in context)
>>>> isn't
>>>> needed for the identity-mapping case?
>>> It is needed to allocate no more than two 'nonroot' page tables (L0
>>> and
>>> L1 in case of Sv39 ) as page 'root' table ( L2 in case of Sv39 ) is
>>> always re-used.
>>>
>>> The same ( only 'nonroot' page tables might be needed to allocate )
>>> works for any MMU mode.
>>
>> Of course, but if you cross a 2Mb boundary you'll need 2 L0 tables.
> Yes, in the case of crossing a 2Mb boundary, it will require 2 L0
> tables.
> 
> Then, the number of required page tables is needed depending on Xen
> size and load address alignment. Because for each 2Mb, we need a new L0
> table.
> 
> Sure, this is not needed now ( as in xen.lds.S, we have a Xen size
> check ), but if someone increases Xen size binary to 4Mb, then the
> amount of page tables should be updated too.
> Should we take into account that?

Perhaps by way of a BUILD_BUG_ON(), yes. We want to avoid setting up
a (runtime) trap for someone to fall into.

Jan