Re: [PATCH 11/36] xen/include: define hypercall parameter for coloring

[Julien Grall <julien@xxxxxxx>]

Hi,

On 04/03/2022 17:46, Marco Solieri wrote:
> From: Luca Miccio <lucmiccio@xxxxxxxxx>
>
> During domU creation process the colors selection has to be passed to
> the Xen hypercall.
> This is generally done using what Xen calls GUEST_HANDLE_PARAMS. In this
> case a simple bitmask for the coloring configuration suffices.
> Currently the maximum amount of supported colors is 128.
> Add a new parameter that allows us to pass both the colors bitmask
> and the number of elements in it.
>
> Signed-off-by: Luca Miccio <lucmiccio@xxxxxxxxx>
> Signed-off-by: Marco Solieri <marco.solieri@xxxxxxxxxxxxxxx>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> ---
>   xen/arch/arm/include/asm/coloring.h | 2 --
>   xen/include/public/arch-arm.h       | 8 ++++++++
I would prefer if the structure is defined in the same patch that will 
use it. This would make easier to figure out if the structure is indeed 
suitable.
>   2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/coloring.h 
> b/xen/arch/arm/include/asm/coloring.h
> index fdd46448d7..1f7e0dde79 100644
> --- a/xen/arch/arm/include/asm/coloring.h
> +++ b/xen/arch/arm/include/asm/coloring.h
> @@ -23,8 +23,6 @@
>   #ifndef __ASM_ARM_COLORING_H__
>   #define __ASM_ARM_COLORING_H__
>   
> -#define MAX_COLORS_CELLS 4
> -
In general, we should avoid moving code that was introduced within the 
same series.
In this case, I am not convinced we should use a static array to 
communicate the information between the toolstack and Xen.
This would make more difficult for the user to tweak update the number 
of colors.
Instead, I think it should be better to expose to the toolstack the 
number of color supported and allocate a dynamic array.
>   #ifdef CONFIG_COLORING
>   #include <xen/sched.h>
>   
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index 94b31511dd..627cc42164 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -303,6 +303,12 @@ struct vcpu_guest_context {
>   typedef struct vcpu_guest_context vcpu_guest_context_t;
>   DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
>   
> +#define MAX_COLORS_CELLS 4
> +struct color_guest_config {
> +    uint32_t max_colors;
> +    uint32_t colors[MAX_COLORS_CELLS];
> +};
This looks like an open-coded version of xenctl_bitmap. Can you have a 
look to use it?
I would expect this will reduce how much code you introduced in the next 
patch.
> +
>   /*
>    * struct xen_arch_domainconfig's ABI is covered by
>    * XEN_DOMCTL_INTERFACE_VERSION.
> @@ -335,6 +341,8 @@ struct xen_arch_domainconfig {
>        *
>        */
>       uint32_t clock_frequency;
> +    /* IN */
> +    struct color_guest_config colors;
>   };
>   #endif /* __XEN__ || __XEN_TOOLS__ */
>   
--
Julien Grall