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

[Julien Grall <julien@xxxxxxx>]

On 14/05/2025 11:51, Orzel, Michal wrote:
> On 14/05/2025 12:06, Julien Grall wrote:
> > 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.
> But the code is not common. There are still 900 lines in arch spec dir.
Sure. But my point is rather more that static memory is not a feature 
described by the Arm Arm. Instead, it is a feature of Xen that is ti to 
device-tree. So inherently there is no reason to be implemented in arch/arm.
> > 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
> IMO it should not be made common in the first place. I don't like exposing
> callers with the big chunk of code sitting in the arch specific directory.
So the concern is we didn't go far enough rather than the feature should 
not be implemented in common for other archs, correct?
> > back to Arm for then moving back to common potentially in a few weeks time.
> What about static memory code? With the recent Oleksii code movement, the inline
> helpers like allocate_static_memory() became unreachable when the feature is
> disabled and the main purpose for helpers was to avoid ifdefery.
Sorry I am not sure which part you are referring to.

Cheers,

--
Julien Grall