Re: [PATCH v2] xen/riscv: Increase XEN_VIRT_SIZE

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.04.2025 13:51, Oleksii Kurochko wrote:
> On 4/7/25 12:09 PM, Jan Beulich wrote:
>> On 04.04.2025 18:04, Oleksii Kurochko wrote:
>>> --- a/xen/arch/riscv/include/asm/mm.h
>>> +++ b/xen/arch/riscv/include/asm/mm.h
>>> @@ -43,13 +43,19 @@ static inline void *maddr_to_virt(paddr_t ma)
>>>    */
>>>   static inline unsigned long virt_to_maddr(unsigned long va)
>>>   {
>>> +    const unsigned int vpn1_shift = PAGETABLE_ORDER + PAGE_SHIFT;
>>> +    const unsigned long va_vpn = va >> vpn1_shift;
>>> +    const unsigned long xen_virt_start_vpn =
>>> +        _AC(XEN_VIRT_START, UL) >> vpn1_shift;
>>> +    const unsigned long xen_virt_end_vpn =
>>> +        xen_virt_start_vpn + ((XEN_VIRT_SIZE >> vpn1_shift) - 1);
>>> +
>>>       if ((va >= DIRECTMAP_VIRT_START) &&
>>>           (va <= DIRECTMAP_VIRT_END))
>>>           return directmapoff_to_maddr(va - directmap_virt_start);
>>>   
>>> -    BUILD_BUG_ON(XEN_VIRT_SIZE != MB(2));
>>> -    ASSERT((va >> (PAGETABLE_ORDER + PAGE_SHIFT)) ==
>>> -           (_AC(XEN_VIRT_START, UL) >> (PAGETABLE_ORDER + PAGE_SHIFT)));
>>> +    BUILD_BUG_ON(XEN_VIRT_SIZE > GB(1));
>>> +    ASSERT((va_vpn >= xen_virt_start_vpn) && (va_vpn <= xen_virt_end_vpn));
>> Not all of the range is backed by memory, and for the excess space the
>> translation is therefore (likely) wrong. Which better would be caught by
>> the assertion?
> 
> Backed here means that the memory is actually mapped?
> 
> IIUC it is needed to check only for the range [XEN_VIRT_START, 
> XEN_VIRT_START+xen_phys_size]
> where xen_phys_size=(unsigned long)_end - (unsigned long)_start.
> 
> Did I understand you correctly?

I think so, yes. Depending on what you (intend to) do to .init.* at the
end of boot, that range may later also want excluding.

>>> --- a/xen/arch/riscv/mm.c
>>> +++ b/xen/arch/riscv/mm.c
>>> @@ -31,20 +31,27 @@ unsigned long __ro_after_init phys_offset; /* = 
>>> load_start - XEN_VIRT_START */
>>>   #define LOAD_TO_LINK(addr) ((unsigned long)(addr) - phys_offset)
>>>   
>>>   /*
>>> - * It is expected that Xen won't be more then 2 MB.
>>> + * It is expected that Xen won't be more then XEN_VIRT_SIZE MB.
>>>    * The check in xen.lds.S guarantees that.
>>> - * At least 3 page tables (in case of Sv39 ) are needed to cover 2 MB.
>>> - * One for each page level table with PAGE_SIZE = 4 Kb.
>>>    *
>>> - * One L0 page table can cover 2 MB(512 entries of one page table * 
>>> PAGE_SIZE).
>>> + * Root page table is shared with the initial mapping and is declared
>>> + * separetely. (look at stage1_pgtbl_root)
>>>    *
>>> - * It might be needed one more page table in case when Xen load address
>>> - * isn't 2 MB aligned.
>>> + * An amount of page tables between root page table and L0 page table
>>> + * (in the case of Sv39 it covers L1 table):
>>> + *   (CONFIG_PAGING_LEVELS - 2) are needed for an identity mapping and
>>> + *   the same amount are needed for Xen.
>>>    *
>>> - * CONFIG_PAGING_LEVELS page tables are needed for the identity mapping,
>>> - * except that the root page table is shared with the initial mapping
>>> + * An amount of L0 page tables:
>>> + *   (512 entries of one L0 page table covers 2MB == 
>>> 1<<XEN_PT_LEVEL_SHIFT(1))
>>> + *   XEN_VIRT_SIZE >> XEN_PT_LEVEL_SHIFT(1) are needed for Xen and
>>> + *   one L0 is needed for indenity mapping.
>>> + *
>>> + *   It might be needed one more page table in case when Xen load
>>> + *   address isn't 2 MB aligned.
>> Shouldn't we guarantee that?
> 
> I think it's sufficient to guarantee 4KB alignment.
> 
> The only real benefit I see in enforcing larger alignment is that it likely 
> enables
> the use of superpages for mapping, which would reduce TLB pressure.
> But perhaps I'm missing something?

No, it's indeed mainly that.

> Or did you mean that if 2MB alignment isn't guaranteed, then we might need 
> two extra
> page tables—one if the start address isn't 2MB aligned, and the Xen size is 
> larger than 2MB?
> Then yes one more page table should be added to PGTBL_INITIAL_COUNT.

Well, of course - if alignment isn't guaranteed, crossing whatever boundaries
of course needs accounting for.

Jan