Re: [PATCH v1 27/27] xen/riscv: add initial dom0less infrastructure support

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.03.2026 18:09, Oleksii Kurochko wrote:
> Enable dom0less support for RISC-V by selecting HAS_DOM0LESS and
> providing the minimal architecture hooks required by the common
> dom0less infrastructure.
> 
> Add stub implementations for architecture-specific helpers used when
> building domains from the device tree. These currently perform no
> additional work but allow the generic dom0less code to build and run
> on RISC-V.
> 
> Introduce max_init_domid as a runtime variable rather than a constant
> so that it can be updated during dom0less domain creation.
> 
> Provide missing helpers and definitions required by the domain
> construction code,

I'm wondering about the splitting among patches: There's half a dozen
(effectively stub) functions which are added here, and then there is
the single init_vuart() which was split out into the earlier patch.
What's the pattern behind this, i.e. why isn't init_vuart() also
being added here?

> including domain bitness helpers and the
> p2m_set_allocation() prototype.
> 
> Additionally define the guest magic memory region in the public
> RISC-V interface.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> ---
> Open questions:
>  - Move declaration of p2m_set_allocation() to xen/fdt-domain-build.h or
>    xen/dom0less-build.h as it is used in common code of Dom0less and
>    there is not too much sense in declaration of it for each arch which
>    supports Dom0less. It could be ifdef-ed in common header as, at the
>    momemnt, it is used only for Dom0less.

Having a common declaration of p2m_set_allocation() is certainly a
sensible thing to do, but not in DT or dom0less headers. p2m-common.h
is, going from file names, the only plausible place for it to go.
Whether that (a) works and (b) makes sense are separate questions.

>  - Shouldn't declaration/defintion of max_init_domid move to common code
>    instead of having it for each architecture separately? If yes, then what
>    would be the best place.

What would you use to decide whether the declaration or #define is
needed? (Plausible headers to put it can surely be found: console.h,
domain.h, and perhaps more.)

> --- a/xen/arch/riscv/include/asm/domain.h
> +++ b/xen/arch/riscv/include/asm/domain.h
> @@ -20,6 +20,14 @@ struct hvm_domain
>      uint64_t              params[HVM_NR_PARAMS];
>  };
>  
> +#ifdef CONFIG_RISCV_64
> +#define is_32bit_domain(d) (0)
> +#define is_64bit_domain(d) (1)
> +#else
> +#define is_32bit_domain(d) (1)
> +#define is_64bit_domain(d) (0)
> +#endif

First, please use true/false. Then, while I agree with the RV32 part, 32-bit
guests surely will need to be an option on a 64-bit hypervisor. Imo you'd
better introduced a field in struct arch_domain to carry that information
(or to derive it from) right away. That wouldn't be set to non-zero for the
time being, i.e. that same constant-true/false would still result.

Otherwise I don't see why you use #ifdef; you could then have things
simpler as

#define is_32bit_domain(d) IS_ENABLED(CONFIG_RISCV_32)
#define is_64bit_domain(d) IS_ENABLED(CONFIG_RISCV_64)

(but I specifically don't recommend going this route).

> --- a/xen/arch/riscv/setup.c
> +++ b/xen/arch/riscv/setup.c
> @@ -32,6 +32,8 @@
>  #include <asm/traps.h>
>  #include <asm/vsbi.h>
>  
> +domid_t max_init_domid = 0;

The initializer isn't of much use, is it? Instead add __read_mostly, like
Arm has it?

> --- a/xen/include/public/arch-riscv.h
> +++ b/xen/include/public/arch-riscv.h
> @@ -58,6 +58,9 @@ typedef uint64_t xen_ulong_t;
>  #define GUEST_RAM_BANK_BASES   { GUEST_RAM0_BASE }
>  #define GUEST_RAM_BANK_SIZES   { GUEST_RAM0_SIZE }
>  
> +#define GUEST_MAGIC_BASE  xen_mk_ullong(0x39000000)
> +#define GUEST_MAGIC_SIZE  xen_mk_ullong(0x01000000)

What is this, and why does it need putting in the public interface? Plus
how come the numbers are exactly the same as what Arm uses?

Jan