Re: [PATCH v2 1/4] xen/arm: split set_domain_type() between arm64/arm32

[Grygorii Strashko <grygorii_strashko@xxxxxxxx>]

On 27.08.25 03:22, Volodymyr Babchuk wrote:
> Hi,
>
> Grygorii Strashko <grygorii_strashko@xxxxxxxx> writes:
>
> > From: Grygorii Strashko <grygorii_strashko@xxxxxxxx>
> >
> > Split set_domain_type() between Arm64/Arm32 sub-arches as
> > set_domain_type() implementation is going to be extended for Arm64.
> >
> > Signed-off-by: Grygorii Strashko <grygorii_strashko@xxxxxxxx>
> > ---
> > v2:
> > - no changes, rebase
> >
> >   xen/arch/arm/arm32/Makefile       |  1 +
> >   xen/arch/arm/arm32/domain-build.c | 22 ++++++++++++++++++++++
> >   xen/arch/arm/arm64/Makefile       |  1 +
> >   xen/arch/arm/arm64/domain-build.c | 24 ++++++++++++++++++++++++
> >   xen/arch/arm/dom0less-build.c     | 14 --------------
> >   xen/include/xen/dom0less-build.h  |  8 ++++++++
> >   6 files changed, 56 insertions(+), 14 deletions(-)
> >   create mode 100644 xen/arch/arm/arm32/domain-build.c
> >   create mode 100644 xen/arch/arm/arm64/domain-build.c
> Is it really worth to create two more source files just for one
> function? Maybe it is better to use already existing
> xen/arch/arm/arm*/domain.c ?
It seems a common approach used for splitting ARM subarch code.
code from arch/arm/A.c goes in
 -> arch/arm/arm32/A.c
 -> arch/arm/arm64/A.c
(just "-" is used vs "_")

This approach also simplifies any further code split (there ~250 
CONFIG_ARM_64/32 ifdefs).

One additional thing, I probably missed, is that "x-build.c" should be placeholder for 
"__init" code only and
so added as "domain-build.init.o" in Makefile.

> > diff --git a/xen/arch/arm/arm32/Makefile b/xen/arch/arm/arm32/Makefile
> > index 531168f58a0a..0fd3f5272361 100644
> > --- a/xen/arch/arm/arm32/Makefile
> > +++ b/xen/arch/arm/arm32/Makefile
> > @@ -6,6 +6,7 @@ obj-y += cache.o
> >   obj-$(CONFIG_EARLY_PRINTK) += debug.o
> >   obj-y += domctl.o
> >   obj-y += domain.o
> > +obj-y += domain-build.o
> >   obj-y += entry.o
> >   obj-y += head.o
> >   obj-y += insn.o
> > diff --git a/xen/arch/arm/arm32/domain-build.c 
> > b/xen/arch/arm/arm32/domain-build.c
> > new file mode 100644
> > index 000000000000..e34261e4a2ad
> > --- /dev/null
> > +++ b/xen/arch/arm/arm32/domain-build.c
> > @@ -0,0 +1,22 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +#include <xen/fdt-kernel.h>
> > +#include <xen/sched.h>
> > +
> > +#include <asm/domain.h>
> > +
> > +#ifdef CONFIG_DOM0LESS_BOOT
> > +void __init set_domain_type(struct domain *d, struct kernel_info *kinfo)
> > +{
> > +    /* Nothing to do */
> > +}
> > +#endif
> > +
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
> > diff --git a/xen/arch/arm/arm64/Makefile b/xen/arch/arm/arm64/Makefile
> > index 6491c5350b2e..3272fe7e4ca2 100644
> > --- a/xen/arch/arm/arm64/Makefile
> > +++ b/xen/arch/arm/arm64/Makefile
> > @@ -8,6 +8,7 @@ obj-$(CONFIG_HARDEN_BRANCH_PREDICTOR) += bpi.o
> >   obj-$(CONFIG_EARLY_PRINTK) += debug.o
> >   obj-y += domctl.o
> >   obj-y += domain.o
> > +obj-y += domain-build.o
> >   obj-y += entry.o
> >   obj-y += head.o
> >   obj-y += insn.o
> > diff --git a/xen/arch/arm/arm64/domain-build.c 
> > b/xen/arch/arm/arm64/domain-build.c
> > new file mode 100644
> > index 000000000000..3a89ee46b8c6
> > --- /dev/null
> > +++ b/xen/arch/arm/arm64/domain-build.c
> > @@ -0,0 +1,24 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +#include <xen/fdt-kernel.h>
> > +#include <xen/sched.h>
> > +
> > +#include <asm/domain.h>
> > +
> > +#ifdef CONFIG_DOM0LESS_BOOT
> > +/* TODO: make arch.type generic ? */
> > +void __init set_domain_type(struct domain *d, struct kernel_info *kinfo)
> > +{
> > +    /* type must be set before allocate memory */
> > +    d->arch.type = kinfo->arch.type;
> > +}
> > +#endif
> > +
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
> > diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> > index c8d07213e247..58f77628df1f 100644
> > --- a/xen/arch/arm/dom0less-build.c
> > +++ b/xen/arch/arm/dom0less-build.c
> > @@ -236,20 +236,6 @@ int __init make_arch_nodes(struct kernel_info *kinfo)
> >       return 0;
> >   }
> >   
> > -/* TODO: make arch.type generic ? */
> > -#ifdef CONFIG_ARM_64
> > -void __init set_domain_type(struct domain *d, struct kernel_info *kinfo)
> > -{
> > -    /* type must be set before allocate memory */
> > -    d->arch.type = kinfo->arch.type;
> > -}
> > -#else
> > -void __init set_domain_type(struct domain *d, struct kernel_info *kinfo)
> > -{
> > -    /* Nothing to do */
> > -}
> > -#endif
> > -
> >   int __init init_vuart(struct domain *d, struct kernel_info *kinfo,
> >                         const struct dt_device_node *node)
> >   {
> > diff --git a/xen/include/xen/dom0less-build.h b/xen/include/xen/dom0less-build.h
> > index 408859e3255a..3e81d8ba3a47 100644
> > --- a/xen/include/xen/dom0less-build.h
> > +++ b/xen/include/xen/dom0less-build.h
> > @@ -57,6 +57,14 @@ int init_vuart(struct domain *d, struct kernel_info *kinfo,
> >   int make_intc_domU_node(struct kernel_info *kinfo);
> >   int make_arch_nodes(struct kernel_info *kinfo);
> >   
> > +/*
> > + * Set domain type from struct kernel_info which defines guest Execution
> > + * State 32-bit/64-bit (for Arm AArch32/AArch64).
> > + * The domain type must be set before allocate_memory.
> > + *
> > + * @d: pointer to the domain structure.
> > + * @kinfo: pointer to the kinfo structure.
> > + */
> >   void set_domain_type(struct domain *d, struct kernel_info *kinfo);
> >   
> >   int init_intc_phandle(struct kernel_info *kinfo, const char *name,
--
Best regards,
-grygorii