Re: [PATCH v3] xen/domain: introduce generic functions for domain struct allocation and freeing

[Grygorii Strashko <grygorii_strashko@xxxxxxxx>]

Hi

Sorry for the late comment.

On 27.11.25 18:26, Oleksii Kurochko wrote:
> From: Roger Pau Monne <roger.pau@xxxxxxxxxx>
>
> Move x86's free_domain_struct() to common code since it is shared between
> architectures.
>
> Move the x86 version of alloc_domain_struct() to common code as most of the
> logic is architecture-independent. To handle the remaining architectural
> differences, introduce arch_domain_struct_memflags() for x86-specific
> allocation requirements.
>
> No functional change.
>
> Suggested-by: Jan Beulich <jbeulich@xxxxxxxx>
> [Introduce an arch-specific function instead of using a weak function]
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> ---
> Changes in v3:
>   - s/arch_alloc_domain_struct_bits/arch_domain_struct_memflags.
>   - Make x86's arch_domain_struct_memflags() to return MEMF_bits(bits) instead
>     of bits.
>   - Move "#define arch_domain_struct_memflags arch_domain_struct_memflags"
>     and declaration of arch_domain_struct_memflags() from x86/asm/pv/domain.h
>     to x86/domain.h.
>   - Update alloc_domain_struct() to work with arch_domain_struct_memflags().
>   - s/Suggested-By/Suggested-by.
>   - CI tests: 
> https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/2182821111
> ---
> Changes in v2:
> - Introduce an arch-specific function to handle differences between arch-es
>    in domain structure allocation requirements, instead of relying on a weak
>    function.
> - Move free_domain_struct() to common code.
> - Add Suggested-by: Jan Beulich <jbeulich@xxxxxxxx>.
> - Update the commit message.
> - Link to original patch:
>    
> https://lore.kernel.org/xen-devel/c08595dd7940b44a1392e16d4a2035b95b5c580b.1749829230.git.oleksii.kurochko@xxxxxxxxx/
> ---
>   xen/arch/arm/domain.c             | 17 -----------------
>   xen/arch/ppc/stubs.c              | 10 ----------
>   xen/arch/riscv/stubs.c            | 10 ----------
>   xen/arch/x86/domain.c             | 15 ++-------------
>   xen/arch/x86/include/asm/domain.h |  3 +++
>   xen/common/domain.c               | 22 ++++++++++++++++++++++
>   6 files changed, 27 insertions(+), 50 deletions(-)
[...]

>   
>   struct vcpu *alloc_vcpu_struct(const struct domain *d)
> diff --git a/xen/arch/x86/include/asm/domain.h 
> b/xen/arch/x86/include/asm/domain.h
> index 5df8c78253..386ec61745 100644
> --- a/xen/arch/x86/include/asm/domain.h
> +++ b/xen/arch/x86/include/asm/domain.h
> @@ -12,6 +12,9 @@
>   #include <public/vcpu.h>
>   #include <public/hvm/hvm_info_table.h>
>   
> +unsigned int arch_domain_struct_memflags(void);
> +#define arch_domain_struct_memflags arch_domain_struct_memflags
> +
>   #define has_32bit_shinfo(d)    ((d)->arch.has_32bit_shinfo)
>   
>   /*
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 3b6e9471c4..2e8d74cbd9 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -799,6 +799,28 @@ static int sanitise_domain_config(struct 
> xen_domctl_createdomain *config)
>       return arch_sanitise_domain_config(config);
>   }
>   
> +struct domain *alloc_domain_struct(void)
> +{
> +#ifndef arch_domain_struct_memflags
> +# define arch_domain_struct_memflags() 0
> +#endif
Is it really the right way to
hide part of common interface in common code instead of header?


> +
> +    struct domain *d;
> +
> +    BUILD_BUG_ON(sizeof(*d) > PAGE_SIZE);
> +
> +    d = alloc_xenheap_pages(0, arch_domain_struct_memflags());
> +    if ( d != NULL )
> +        clear_page(d);
> +
> +    return d;
> +}
> +
> +void free_domain_struct(struct domain *d)
> +{
> +    free_xenheap_page(d);
> +}
> +
>   struct domain *domain_create(domid_t domid,
>                                struct xen_domctl_createdomain *config,
>                                unsigned int flags)
--
Best regards,
-grygorii