Re: [PATCH v1 6/6] xen/riscv: enable DOMAIN_BUILD_HELPERS

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.02.2026 13:54, Oleksii Kurochko wrote:
> On 2/12/26 5:39 PM, Jan Beulich wrote:
>> On 12.02.2026 17:21, Oleksii Kurochko wrote:
>>> --- a/xen/arch/riscv/include/asm/p2m.h
>>> +++ b/xen/arch/riscv/include/asm/p2m.h
>>> @@ -44,6 +44,9 @@
>>>   #define P2M_LEVEL_MASK(p2m, lvl) \
>>>       (P2M_TABLE_OFFSET(p2m, lvl) << P2M_GFN_LEVEL_SHIFT(lvl))
>>>   
>>> +/* Holds the bit size of IPAs in p2m tables */
>>> +extern unsigned int p2m_ipa_bits;
>> Hmm, I can spot a declaration and ...
>>
>>> --- a/xen/arch/riscv/p2m.c
>>> +++ b/xen/arch/riscv/p2m.c
>>> @@ -51,6 +51,12 @@ static struct gstage_mode_desc __ro_after_init 
>>> max_gstage_mode = {
>>>       .name = "Bare",
>>>   };
>>>   
>>> +/*
>>> + * Set to the maximum configured support for IPA bits, so the number of 
>>> IPA bits can be
>>> + * restricted by external entity (e.g. IOMMU).
>>> + */
>>> +unsigned int __read_mostly p2m_ipa_bits = PADDR_BITS;
>> ... a definition, but neither a use nor a place where the variable would
>> be set. Hmm, yes, I see common/device-tree/domain-build.c uses it. Then
>> the following questions arise:
>> - What does "ipa" stand for? Is this a term sensible in RISC-V context at
>>    all?
> 
> IPA stands for GPA. (maybe it would better to rename the in common code to 
> gpa too).
> It was used as common code uses p2m_ipa_bits.
> 
> Yes, I miss to set p2m_ipa_bits properly in p2m_init() where G-stage MMU mode 
> is set.
> 
>> Judging from the comment at the decl, isn't it PPN width (plus
>>    PAGE_SHIFT) that it describes?
> 
> It is PPN width + PAGE_SHIFT what is equal to PADDR_BITS (44bit PPN + 12 bit 
> PAGE_SHIFT).
> 
>> - With there not being anyone writing to the variable, why is it not
>>    const (or even a #define), or at the very least __ro_after_init?
>> And no, "Arm has it like this" doesn't count as an answer. Considering
>> all the review comments you've got so far you should know by now that you
>> shouldn't copy things blindly.
> 
> It was added because of the usage in common/device-tree/domain-build.c.

Well, I understand that, but this isn't the way to do. And you've been through
such before. Anything you want to share between arch-es that isn't shared yet,
will want some suitable abstraction done. Like giving variables names which
are appropriate independent of the arch.

>>> --- a/xen/include/public/arch-riscv.h
>>> +++ b/xen/include/public/arch-riscv.h
>>> @@ -50,6 +50,14 @@ typedef uint64_t xen_ulong_t;
>>>   
>>>   #if defined(__XEN__) || defined(__XEN_TOOLS__)
>>>   
>>> +#define GUEST_RAM_BANKS   1
>>> +
>>> +#define GUEST_RAM0_BASE   xen_mk_ullong(0x80000000) /* 2GB of low RAM @ 
>>> 2GB */
>>> +#define GUEST_RAM0_SIZE   xen_mk_ullong(0x80000000)
>>> +
>>> +#define GUEST_RAM_BANK_BASES   { GUEST_RAM0_BASE }
>>> +#define GUEST_RAM_BANK_SIZES   { GUEST_RAM0_SIZE }
>> Hmm, does RISC-V really want to go with compile-time constants here?
> 
> It is needed for allocate_memory() for guest domains, so it is expected
> to be compile-time constant with the current code of common dom0less
> approach.
> 
> It represents the start of RAM address for DomU and the maximum RAM size
> (the actual size will be calculated based on what is mentioned in DomU node
> in dts) and then will be used to generate memory node for DomU 
> (GUEST_RAM0_BASE
> as RAM start address and min(GUEST_RAM0_SIZE, dts->domU->memory->size) as a
> RAM size).
> 
>>   And
>> if so, why would guests be limited to just 2 Gb?
> 
> It is enough for guest domain I am using in dom0less mode.

And what others may want to use RISC-V for once it actually becomes usable
isn't relevant? As you start adding things to the public headers, you will
need to understand that you can't change easily what once was put there.
Everything there is part of the ABI, and the ABI needs to remain stable
(within certain limits).

>> That may more efficiently
>> be RV32 guests then, with perhaps just an RV32 hypervisor.
> 
> I  didn't get this point. Could you please explain differently what do you
> mean?

If all you want are 2Gb guests, why would such guests be 64-bit? And with
(iirc) RV32 permitting more than 4Gb (via PPN being 22 bits wide), perhaps
even a 32-bit hypervisor would suffice?

Jan