Re: [PATCH v1 2/3] xen/dom0less: refactor architecture-specific DomU construction

[Julien Grall <julien@xxxxxxx>]

Hi,

On 14/05/2025 10:57, Oleksii Kurochko wrote:
> On 5/14/25 9:25 AM, Julien Grall wrote:
> > Hi Oleksii,
> >
> > On 13/05/2025 15:29, Oleksii Kurochko wrote:
> > > Refactor construct_domU() to improve architecture separation and reduce
> > > reliance on ARM-specific logic in common code:
> > > - Drop set_domain_type() from generic code. This function is specific
> > >    to ARM and serves no purpose on other architectures like RISC-V,
> > >    which lack the arch.type field in kernel_info.
> > So you will only ever boot one type of domain on RISC-V? IOW, no 32-bit
> > or else?
> The bitness of the guest and host should match. So, an RV32 host should run
> RV32 guests, and an RV64 host should run RV64 guests and so on.
> (I'm not really sure that on RISC-V it is possible to run with RV64 host
> an RV32 guest, but need to double-check)
>
> Therefore, my plan for RISC-V is to have the following:
>    #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
>
> With this definition, there's no need to use|(d)->arch.type| for RISC-V.
>
> > > - Introduce arch_construct_domU() to encapsulate architecture-specific
> > >    DomU construction steps.
> > > - Implement arch_construct_domU() for ARM. This includes:
> > >    - Setting the domain type for CONFIG_ARM64.
> > >    - Handling static memory allocation if xen,static-mem is present in
> > >      the device tree.
> > >    - Processing static shared memory.
> > > - Move call of make_resv_memory_node() to Arm's make_arch_nodes() as
> > >    this call is specific to CONFIG_STATIC_SHM which is ARM specific,
> > >    at least, now.
> > This looks shortsighted. If there is a plan to use CONFIG_STATIC_SHM 
> > on RISC-V (I don't see why not today), then
> > I think the code should stick in common/ even if it is not fully usable
> > yet (that's the whole point of have CONFIG_* options).
> We don't use this feature in the downstream repo, but I can imagine some 
> cases where it could be useful. So, for now, its
> use is purely theoretical and it is a reason why I agreed with Michal 
> and returned back these changes to Arm.
I strongly disagree with this statement. If a feature is not 
architecture specific (like static shared memory), then the code ought 
to be in common code so it can be re-used by others.
Also, AFAIK, the bulk of the static shared memory code is in common. So 
it would be rather easy to add support for RISC-V if we wanted to.
Given the code is already in common, it is rather silly to move the code 
back to Arm for then moving back to common potentially in a few weeks time.
So:

Nacked-by: Julien Grall <jgrall@xxxxxxxxxx>

Cheers,

--
Julien Grall