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.
- 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 cleanup avoids empty stubs on other architectures and moves
ARM-specific logic into arch code where it belongs.
Also, don't loose a return value of functions called in
Arm's make_arch_nodes().
Suggested-by: Michal Orzel <michal.orzel@amd.com>
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
xen/arch/arm/dom0less-build.c | 42 +++++++++++++++++-------
xen/common/device-tree/dom0less-build.c | 30 ++---------------
xen/include/asm-generic/dom0less-build.h | 3 +-
3 files changed, 36 insertions(+), 39 deletions(-)
diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
index a49764f0ad..592173268f 100644
--- a/xen/arch/arm/dom0less-build.c
+++ b/xen/arch/arm/dom0less-build.c
@@ -220,9 +220,14 @@ int __init make_arch_nodes(struct kernel_info *kinfo)
{
int ret;
+ ret = make_resv_memory_node(kinfo, GUEST_ROOT_ADDRESS_CELLS,
+ GUEST_ROOT_SIZE_CELLS);
+ if ( ret )
+ return ret;
+
ret = make_psci_node(kinfo->fdt);
if ( ret )
- return -EINVAL;
+ return ret;
if ( kinfo->arch.vpl011 )
{
@@ -230,26 +235,41 @@ int __init make_arch_nodes(struct kernel_info *kinfo)
ret = make_vpl011_uart_node(kinfo);
#endif
if ( ret )
- return -EINVAL;
+ return ret;
}
return 0;
}
-/* TODO: make arch.type generic ? */
-#ifdef CONFIG_ARM_64
-void __init set_domain_type(struct domain *d, struct kernel_info *kinfo)
+int __init arch_construct_domU(struct kernel_info *kinfo,
+ const struct dt_device_node *node)
{
+ int rc = 0;
+ struct domain *d = kinfo->d;
+
+#ifdef CONFIG_ARM_64
/* 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
+ if ( !is_hardware_domain(d) )
+ {
+ if ( dt_find_property(node, "xen,static-mem", NULL) )
+ {
+ if ( !is_domain_direct_mapped(d) )
+ allocate_static_memory(d, kinfo, node);
+ else
+ assign_static_memory_11(d, kinfo, node);
+ }
+
+ rc = process_shm(d, kinfo, node);
+ if ( rc < 0 )
+ return rc;
+ }
+
+ return rc;
+}
+
int __init init_vuart(struct domain *d, struct kernel_info *kinfo,
const struct dt_device_node *node)
{
diff --git a/xen/common/device-tree/dom0less-build.c b/xen/common/device-tree/dom0less-build.c
index 2c56f13771..f6aabc2093 100644
--- a/xen/common/device-tree/dom0less-build.c
+++ b/xen/common/device-tree/dom0less-build.c
@@ -28,14 +28,6 @@
#include <asm/dom0less-build.h>
#include <asm/setup.h>
-#if __has_include(<asm/static-memory.h>)
-# include <asm/static-memory.h>
-#endif
-
-#if __has_include(<asm/static-shmem.h>)
-# include <asm/static-shmem.h>
-#endif
-
#define XENSTORE_PFN_LATE_ALLOC UINT64_MAX
static domid_t __initdata xs_domid = DOMID_INVALID;
@@ -507,12 +499,6 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
if ( ret )
goto err;
-#ifdef CONFIG_STATIC_SHM
- ret = make_resv_memory_node(kinfo, addrcells, sizecells);
- if ( ret )
- goto err;
-#endif
-
/*
* domain_handle_dtb_bootmodule has to be called before the rest of
* the device tree is generated because it depends on the value of
@@ -787,7 +773,9 @@ static int __init construct_domU(struct domain *d,
if ( rc < 0 )
return rc;
- set_domain_type(d, &kinfo);
+ rc = arch_construct_domU(&kinfo, node);
+ if ( rc )
+ return rc;
if ( is_hardware_domain(d) )
{
@@ -799,18 +787,6 @@ static int __init construct_domU(struct domain *d,
{
if ( !dt_find_property(node, "xen,static-mem", NULL) )
allocate_memory(d, &kinfo);
-#ifdef CONFIG_STATIC_MEMORY
- else if ( !is_domain_direct_mapped(d) )
- allocate_static_memory(d, &kinfo, node);
- else
- assign_static_memory_11(d, &kinfo, node);
-#endif
-
-#ifdef CONFIG_STATIC_SHM
- rc = process_shm(d, &kinfo, node);
- if ( rc < 0 )
- return rc;
-#endif
rc = init_vuart(d, &kinfo, node);
if ( rc < 0 )
diff --git a/xen/include/asm-generic/dom0less-build.h b/xen/include/asm-generic/dom0less-build.h
index e0ad0429ec..78142e71ca 100644
--- a/xen/include/asm-generic/dom0less-build.h
+++ b/xen/include/asm-generic/dom0less-build.h
@@ -56,7 +56,8 @@ 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);
-void set_domain_type(struct domain *d, struct kernel_info *kinfo);
+int arch_construct_domU(struct kernel_info *kinfo,
+ const struct dt_device_node *node);
int init_intc_phandle(struct kernel_info *kinfo, const char *name,
const int node_next, const void *pfdt);
--
2.49.0Hi 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? > - 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). Cheers, -- Julien Grall
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. ~ Oleksii
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@amazon.com> Cheers, -- Julien Grall
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. > > 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. My opinion is that they should be exposed at once when the support for other arches appear. Today we ended up with caller protected with #ifdef and the majority of code in arch dir. > 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. > > So: > > Nacked-by: Julien Grall <jgrall@amazon.com> Ok. No more discussion from my side then. ~Michal
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
On 14/05/2025 13:18, Julien Grall wrote: > > > 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. I agree, it can and should be made common. But exposing only callers makes no sense at all for me. Callers should be exposed together with feature code movement. > >>> >>> 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? Yes. Oleksii exposed only callers. His intention was not to make static feature common. > >> >>> 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. With the current code, allocate_static_memory(), assign_static_memory_11() callers (that were moved to common) are protected with #ifdef. This causes the helpers defined in case CONFIG_STATIC_MEMORY is not defined to be unreachable (see static-memory.h). ~Michal
On Wed, 14 May 2025, Orzel, Michal wrote: > > 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. > I agree, it can and should be made common. But exposing only callers makes no > sense at all for me. Callers should be exposed together with feature code movement. [...] > >>> 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. > With the current code, allocate_static_memory(), assign_static_memory_11() > callers (that were moved to common) are protected with #ifdef. This causes the > helpers defined in case CONFIG_STATIC_MEMORY is not defined to be unreachable > (see static-memory.h). At a high level I agree with Julien, this is the kind of feature that should be common. In fact, I would even say that I consider the related device tree bindings common. But looking at the code movements and the #ifdef's, I think that as of today, this patch series is improving things. So my Ack was not at all a statement to say that the feature should be Arm-only. To the contrary. But it was only a practical consideration about the state of the code right now. I do think that RISC-V should gain support for it at some point and we should share the code as much as possible.
On 15/05/2025 02:07, Stefano Stabellini wrote: > On Wed, 14 May 2025, Orzel, Michal wrote: >>> 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. >> I agree, it can and should be made common. But exposing only callers makes no >> sense at all for me. Callers should be exposed together with feature code movement. > > [...] > >>>>> 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. >> With the current code, allocate_static_memory(), assign_static_memory_11() >> callers (that were moved to common) are protected with #ifdef. This causes the >> helpers defined in case CONFIG_STATIC_MEMORY is not defined to be unreachable >> (see static-memory.h). > > At a high level I agree with Julien, this is the kind of feature that > should be common. In fact, I would even say that I consider the related > device tree bindings common. But looking at the code movements and the > #ifdef's, I think that as of today, this patch series is improving > things. I've said multiple times already that I also agree that static mem/shmem should be common. It was not the point of this discussion. My only concern is that it should be done in a proper way and not in 5% like it was done recently that also resulted in issues I mentioned above. Therefore I also (hence my tag) believe we should accept it. ~Michal
On 13/05/2025 16: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. > - 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 cleanup avoids empty stubs on other architectures and moves > ARM-specific logic into arch code where it belongs. > > Also, don't loose a return value of functions called in > Arm's make_arch_nodes(). > > Suggested-by: Michal Orzel <michal.orzel@amd.com> Thanks, it looks better now. > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> Reviewed-by: Michal Orzel <michal.orzel@amd.com> possibly with the remark from Stefano fixed. ~Michal
On Tue, 13 May 2025, 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.
> - 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 cleanup avoids empty stubs on other architectures and moves
> ARM-specific logic into arch code where it belongs.
>
> Also, don't loose a return value of functions called in
> Arm's make_arch_nodes().
>
> Suggested-by: Michal Orzel <michal.orzel@amd.com>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
> xen/arch/arm/dom0less-build.c | 42 +++++++++++++++++-------
> xen/common/device-tree/dom0less-build.c | 30 ++---------------
> xen/include/asm-generic/dom0less-build.h | 3 +-
> 3 files changed, 36 insertions(+), 39 deletions(-)
>
> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> index a49764f0ad..592173268f 100644
> --- a/xen/arch/arm/dom0less-build.c
> +++ b/xen/arch/arm/dom0less-build.c
> @@ -220,9 +220,14 @@ int __init make_arch_nodes(struct kernel_info *kinfo)
> {
> int ret;
>
> + ret = make_resv_memory_node(kinfo, GUEST_ROOT_ADDRESS_CELLS,
> + GUEST_ROOT_SIZE_CELLS);
> + if ( ret )
> + return ret;
> +
> ret = make_psci_node(kinfo->fdt);
> if ( ret )
> - return -EINVAL;
> + return ret;
>
> if ( kinfo->arch.vpl011 )
> {
> @@ -230,26 +235,41 @@ int __init make_arch_nodes(struct kernel_info *kinfo)
> ret = make_vpl011_uart_node(kinfo);
> #endif
> if ( ret )
> - return -EINVAL;
> + return ret;
> }
>
> return 0;
> }
>
> -/* TODO: make arch.type generic ? */
> -#ifdef CONFIG_ARM_64
> -void __init set_domain_type(struct domain *d, struct kernel_info *kinfo)
> +int __init arch_construct_domU(struct kernel_info *kinfo,
> + const struct dt_device_node *node)
> {
> + int rc = 0;
> + struct domain *d = kinfo->d;
> +
> +#ifdef CONFIG_ARM_64
> /* 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
NIT: I think it would be nicer to do
if ( is_hardware_domain(d) )
return rc;
but it is also OK as below
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> + if ( !is_hardware_domain(d) )
> + {
> + if ( dt_find_property(node, "xen,static-mem", NULL) )
> + {
> + if ( !is_domain_direct_mapped(d) )
> + allocate_static_memory(d, kinfo, node);
> + else
> + assign_static_memory_11(d, kinfo, node);
> + }
> +
> + rc = process_shm(d, kinfo, node);
> + if ( rc < 0 )
> + return rc;
> + }
> +
> + return rc;
> +}
> +
> int __init init_vuart(struct domain *d, struct kernel_info *kinfo,
> const struct dt_device_node *node)
> {
> diff --git a/xen/common/device-tree/dom0less-build.c b/xen/common/device-tree/dom0less-build.c
> index 2c56f13771..f6aabc2093 100644
> --- a/xen/common/device-tree/dom0less-build.c
> +++ b/xen/common/device-tree/dom0less-build.c
> @@ -28,14 +28,6 @@
> #include <asm/dom0less-build.h>
> #include <asm/setup.h>
>
> -#if __has_include(<asm/static-memory.h>)
> -# include <asm/static-memory.h>
> -#endif
> -
> -#if __has_include(<asm/static-shmem.h>)
> -# include <asm/static-shmem.h>
> -#endif
> -
> #define XENSTORE_PFN_LATE_ALLOC UINT64_MAX
>
> static domid_t __initdata xs_domid = DOMID_INVALID;
> @@ -507,12 +499,6 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
> if ( ret )
> goto err;
>
> -#ifdef CONFIG_STATIC_SHM
> - ret = make_resv_memory_node(kinfo, addrcells, sizecells);
> - if ( ret )
> - goto err;
> -#endif
> -
> /*
> * domain_handle_dtb_bootmodule has to be called before the rest of
> * the device tree is generated because it depends on the value of
> @@ -787,7 +773,9 @@ static int __init construct_domU(struct domain *d,
> if ( rc < 0 )
> return rc;
>
> - set_domain_type(d, &kinfo);
> + rc = arch_construct_domU(&kinfo, node);
> + if ( rc )
> + return rc;
>
> if ( is_hardware_domain(d) )
> {
> @@ -799,18 +787,6 @@ static int __init construct_domU(struct domain *d,
> {
> if ( !dt_find_property(node, "xen,static-mem", NULL) )
> allocate_memory(d, &kinfo);
> -#ifdef CONFIG_STATIC_MEMORY
> - else if ( !is_domain_direct_mapped(d) )
> - allocate_static_memory(d, &kinfo, node);
> - else
> - assign_static_memory_11(d, &kinfo, node);
> -#endif
> -
> -#ifdef CONFIG_STATIC_SHM
> - rc = process_shm(d, &kinfo, node);
> - if ( rc < 0 )
> - return rc;
> -#endif
>
> rc = init_vuart(d, &kinfo, node);
> if ( rc < 0 )
> diff --git a/xen/include/asm-generic/dom0less-build.h b/xen/include/asm-generic/dom0less-build.h
> index e0ad0429ec..78142e71ca 100644
> --- a/xen/include/asm-generic/dom0less-build.h
> +++ b/xen/include/asm-generic/dom0less-build.h
> @@ -56,7 +56,8 @@ 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);
>
> -void set_domain_type(struct domain *d, struct kernel_info *kinfo);
> +int arch_construct_domU(struct kernel_info *kinfo,
> + const struct dt_device_node *node);
>
> int init_intc_phandle(struct kernel_info *kinfo, const char *name,
> const int node_next, const void *pfdt);
> --
> 2.49.0
>
On 5/14/25 1:58 AM, Stefano Stabellini wrote:
> On Tue, 13 May 2025, 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.
>> - 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 cleanup avoids empty stubs on other architectures and moves
>> ARM-specific logic into arch code where it belongs.
>>
>> Also, don't loose a return value of functions called in
>> Arm's make_arch_nodes().
>>
>> Suggested-by: Michal Orzel<michal.orzel@amd.com>
>> Signed-off-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>
>> ---
>> xen/arch/arm/dom0less-build.c | 42 +++++++++++++++++-------
>> xen/common/device-tree/dom0less-build.c | 30 ++---------------
>> xen/include/asm-generic/dom0less-build.h | 3 +-
>> 3 files changed, 36 insertions(+), 39 deletions(-)
>>
>> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
>> index a49764f0ad..592173268f 100644
>> --- a/xen/arch/arm/dom0less-build.c
>> +++ b/xen/arch/arm/dom0less-build.c
>> @@ -220,9 +220,14 @@ int __init make_arch_nodes(struct kernel_info *kinfo)
>> {
>> int ret;
>>
>> + ret = make_resv_memory_node(kinfo, GUEST_ROOT_ADDRESS_CELLS,
>> + GUEST_ROOT_SIZE_CELLS);
>> + if ( ret )
>> + return ret;
>> +
>> ret = make_psci_node(kinfo->fdt);
>> if ( ret )
>> - return -EINVAL;
>> + return ret;
>>
>> if ( kinfo->arch.vpl011 )
>> {
>> @@ -230,26 +235,41 @@ int __init make_arch_nodes(struct kernel_info *kinfo)
>> ret = make_vpl011_uart_node(kinfo);
>> #endif
>> if ( ret )
>> - return -EINVAL;
>> + return ret;
>> }
>>
>> return 0;
>> }
>>
>> -/* TODO: make arch.type generic ? */
>> -#ifdef CONFIG_ARM_64
>> -void __init set_domain_type(struct domain *d, struct kernel_info *kinfo)
>> +int __init arch_construct_domU(struct kernel_info *kinfo,
>> + const struct dt_device_node *node)
>> {
>> + int rc = 0;
>> + struct domain *d = kinfo->d;
>> +
>> +#ifdef CONFIG_ARM_64
>> /* 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
> NIT: I think it would be nicer to do
>
> if ( is_hardware_domain(d) )
> return rc;
>
> but it is also OK as below
>
> Reviewed-by: Stefano Stabellini<sstabellini@kernel.org>
Thanks.
It would be really nicer, I'll update that in the next one version.
~ Oleksii
>
>
>> + if ( !is_hardware_domain(d) )
>> + {
>> + if ( dt_find_property(node, "xen,static-mem", NULL) )
>> + {
>> + if ( !is_domain_direct_mapped(d) )
>> + allocate_static_memory(d, kinfo, node);
>> + else
>> + assign_static_memory_11(d, kinfo, node);
>> + }
>> +
>> + rc = process_shm(d, kinfo, node);
>> + if ( rc < 0 )
>> + return rc;
>> + }
>> +
>> + return rc;
>> +}
>> +
>> int __init init_vuart(struct domain *d, struct kernel_info *kinfo,
>> const struct dt_device_node *node)
>> {
>> diff --git a/xen/common/device-tree/dom0less-build.c b/xen/common/device-tree/dom0less-build.c
>> index 2c56f13771..f6aabc2093 100644
>> --- a/xen/common/device-tree/dom0less-build.c
>> +++ b/xen/common/device-tree/dom0less-build.c
>> @@ -28,14 +28,6 @@
>> #include <asm/dom0less-build.h>
>> #include <asm/setup.h>
>>
>> -#if __has_include(<asm/static-memory.h>)
>> -# include <asm/static-memory.h>
>> -#endif
>> -
>> -#if __has_include(<asm/static-shmem.h>)
>> -# include <asm/static-shmem.h>
>> -#endif
>> -
>> #define XENSTORE_PFN_LATE_ALLOC UINT64_MAX
>>
>> static domid_t __initdata xs_domid = DOMID_INVALID;
>> @@ -507,12 +499,6 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
>> if ( ret )
>> goto err;
>>
>> -#ifdef CONFIG_STATIC_SHM
>> - ret = make_resv_memory_node(kinfo, addrcells, sizecells);
>> - if ( ret )
>> - goto err;
>> -#endif
>> -
>> /*
>> * domain_handle_dtb_bootmodule has to be called before the rest of
>> * the device tree is generated because it depends on the value of
>> @@ -787,7 +773,9 @@ static int __init construct_domU(struct domain *d,
>> if ( rc < 0 )
>> return rc;
>>
>> - set_domain_type(d, &kinfo);
>> + rc = arch_construct_domU(&kinfo, node);
>> + if ( rc )
>> + return rc;
>>
>> if ( is_hardware_domain(d) )
>> {
>> @@ -799,18 +787,6 @@ static int __init construct_domU(struct domain *d,
>> {
>> if ( !dt_find_property(node, "xen,static-mem", NULL) )
>> allocate_memory(d, &kinfo);
>> -#ifdef CONFIG_STATIC_MEMORY
>> - else if ( !is_domain_direct_mapped(d) )
>> - allocate_static_memory(d, &kinfo, node);
>> - else
>> - assign_static_memory_11(d, &kinfo, node);
>> -#endif
>> -
>> -#ifdef CONFIG_STATIC_SHM
>> - rc = process_shm(d, &kinfo, node);
>> - if ( rc < 0 )
>> - return rc;
>> -#endif
>>
>> rc = init_vuart(d, &kinfo, node);
>> if ( rc < 0 )
>> diff --git a/xen/include/asm-generic/dom0less-build.h b/xen/include/asm-generic/dom0less-build.h
>> index e0ad0429ec..78142e71ca 100644
>> --- a/xen/include/asm-generic/dom0less-build.h
>> +++ b/xen/include/asm-generic/dom0less-build.h
>> @@ -56,7 +56,8 @@ 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);
>>
>> -void set_domain_type(struct domain *d, struct kernel_info *kinfo);
>> +int arch_construct_domU(struct kernel_info *kinfo,
>> + const struct dt_device_node *node);
>>
>> int init_intc_phandle(struct kernel_info *kinfo, const char *name,
>> const int node_next, const void *pfdt);
>> --
>> 2.49.0
>>
© 2016 - 2025 Red Hat, Inc.