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

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

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


On 3/31/25 6:14 PM, Jan Beulich wrote:
> On 31.03.2025 17:20, Oleksii Kurochko wrote:
>> A randconfig job failed with the following issue:
>>    riscv64-linux-gnu-ld: Xen too large for early-boot assumptions
>>
>> The reason is that enabling the UBSAN config increased the size of
>> the Xen binary.
>>
>> Increase XEN_VIRT_SIZE to reserve enough space, allowing both UBSAN
>> and GCOV to be enabled together, with some slack for future growth.
> At some point you may want to use 2M mappings for .text (rx), .rodata
> (r), and .data (rw). Together with .init that would then completely
> fill those 8Mb afaict. Hence you may want to go a little further right
> away, e.g. to 16Mb.

It makes sense to me. I'll update to 16 Mb then right now.

>> +        _AC(XEN_VIRT_START, UL) >> vpn1_shift;
>> +    const unsigned long xen_virt_end_vpn =
>> +        xen_virt_starn_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 != MB(8));
> Is it necessary to be != ? Won't > suffice?

It could be just > MB(2). Or perphaps >=.

>
>> +    ASSERT((va_vpn >= xen_virt_starn_vpn) && (va_vpn <= xen_virt_end_vpn));
> Are you sure about <= on the rhs of the && ?

I am using -1 [ ((XEN_VIRT_SIZE >> vpn1_shift) - 1) ] when calculating the xen_virt_end_vpn to make the range inclusive.
So it should be fine.

>
>> --- a/xen/arch/riscv/mm.c
>> +++ b/xen/arch/riscv/mm.c
>> @@ -31,20 +31,21 @@ 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 8 MB.
>>    * The check in xen.lds.S guarantees that.
>> - * At least 3 page tables (in case of Sv39 ) are needed to cover 2 MB.
>> + * At least 6 page tables (in case of Sv39) are needed to cover 8 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).
>> + * Four L0 page table can cover 8 MB(512 entries of
>> + * one page table * PAGE_SIZE).
>>    *
>>    * It might be needed one more page table in case when Xen load address
>>    * isn't 2 MB aligned.
>>    *
>> - * CONFIG_PAGING_LEVELS page tables are needed for the identity mapping,
>> + * (CONFIG_PAGING_LEVELS + 2) page tables are needed for the identity mapping,
>>    * except that the root page table is shared with the initial mapping
>>    */
>> -#define PGTBL_INITIAL_COUNT ((CONFIG_PAGING_LEVELS - 1) * 2 + 1)
>> +#define PGTBL_INITIAL_COUNT ((CONFIG_PAGING_LEVELS + 2) * 2 + 1)
> I'm in trouble fitting the comment updates with the update of the #define. Why
> would more tables be needed for the identity mapping?

Agree, it isn't needed more tables for the identity mapping.

>   Why does XEN_VIRT_SIZE
> not appear anywhere here?

I just used 8 Mb explicitly in the comment but I think you really asked me about definition
of PGTBL_INITIAL_COUNT where I just explicitly take into account 3 extra pages for L0.
I will update it with using of XEN_VIRT_SIZE to have more generic definition of PGTBL_INITIAL_COUNT.

Thanks

~ Oleksii

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