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

[Jan Beulich <jbeulich@xxxxxxxx>]

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? Judging from the comment at the decl, isn't it PPN width (plus
  PAGE_SHIFT) that it describes?
- 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.

> --- 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? And
if so, why would guests be limited to just 2 Gb? That may more efficiently
be RV32 guests then, with perhaps just an RV32 hypervisor.

Jan