Re: [PATCH v1 4/5] xen/riscv: introduce device tree maping function

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.07.2024 11:31, Oleksii wrote:
> On Wed, 2024-07-10 at 12:38 +0200, Jan Beulich wrote:
>> On 03.07.2024 12:42, Oleksii Kurochko wrote:
>>> --- a/xen/arch/riscv/include/asm/config.h
>>> +++ b/xen/arch/riscv/include/asm/config.h
>>> @@ -74,6 +74,9 @@
>>>  #error "unsupported RV_STAGE1_MODE"
>>>  #endif
>>>  
>>> +#define XEN_SIZE                MB(2)
>>> +#define XEN_VIRT_END            (XEN_VIRT_START + XEN_SIZE)
>>
>> Probably wants accompanying by an assertion in the linker script. Or
>> else
>> how would one notice when Xen grows bigger than this?
> I use XEN_SIZE in the linker script here:
>  ASSERT(_end - _start <= MB(2), "Xen too large for early-boot
> assumptions")

And that's the problem: You want to switch to using XEN_SIZE there. There
should never be two separate constant that need updating at the same time.
Keep such to a single place.

>>> @@ -99,6 +102,9 @@
>>>  #define VMAP_VIRT_START         SLOTN(VMAP_SLOT_START)
>>>  #define VMAP_VIRT_SIZE          GB(1)
>>>  
>>> +#define BOOT_FDT_VIRT_START     XEN_VIRT_END
>>> +#define BOOT_FDT_VIRT_SIZE      MB(4)
>>
>> Is the 4 selected arbitrarily, or derived from something?
> Yes, it was chosen arbitrarily. I just checked that I don't have any
> DTBs larger than 2 MB, but decided to add a little extra space and
> doubled it to an additional 2 MB.

Code comment then, please, or at the very least mention of this in the
description.

>>> @@ -39,9 +42,11 @@ static unsigned long __ro_after_init
>>> phys_offset;
>>>   * isn't 2 MB aligned.
>>>   *
>>>   * CONFIG_PAGING_LEVELS page tables are needed for the identity
>>> mapping,
>>> - * except that the root page table is shared with the initial
>>> mapping
>>> + * except that the root page table is shared with the initial
>>> mapping.
>>> + *
>>> + * CONFIG_PAGING_LEVELS page tables are needed for device tree
>>> mapping.
>>>   */
>>> -#define PGTBL_INITIAL_COUNT ((CONFIG_PAGING_LEVELS - 1) * 2 + 1)
>>> +#define PGTBL_INITIAL_COUNT ((CONFIG_PAGING_LEVELS - 1) * 3 + 1 +
>>> 1)
>>
>> Considering what would happen if two or three more of such
>> requirements
>> were added, maybe better
>>
>> #define PGTBL_INITIAL_COUNT (CONFIG_PAGING_LEVELS * 3 - 1)
>>
>> ? However, why is it CONFIG_PAGING_LEVELS that's needed, and not
>> CONFIG_PAGING_LEVELS - 1? The top level table is the same as the
>> identity map's, isn't it?
> The top level table is the same, but I just wanted to be sure that if
> DTB size is bigger then 2Mb then we need 2xL0 page tables.

Makes sense, but then needs expressing that way (by using
BOOT_FDT_VIRT_SIZE). Otherwise (see also above) think of what will
happen if BOOT_FDT_VIRT_SIZE is updated without touching this
expression.

Jan