Re: [PATCH v4 05/11] tools: add support for cache coloring configuration

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Mon, Jan 23, 2023 at 04:47:29PM +0100, Carlo Nonato wrote:
> diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c
> index e939d07157..064f54c349 100644
> --- a/tools/libs/ctrl/xc_domain.c
> +++ b/tools/libs/ctrl/xc_domain.c
> @@ -28,6 +28,20 @@ int xc_domain_create(xc_interface *xch, uint32_t *pdomid,
>  {
>      int err;
>      DECLARE_DOMCTL;
> +    DECLARE_HYPERCALL_BUFFER(uint32_t, llc_colors);
> +
> +    if ( config->num_llc_colors )
> +    {
> +        size_t bytes = sizeof(uint32_t) * config->num_llc_colors;
> +
> +        llc_colors = xc_hypercall_buffer_alloc(xch, llc_colors, bytes);
> +        if ( llc_colors == NULL ) {
> +            PERROR("Could not allocate LLC colors for xc_domain_create");
> +            return -ENOMEM;
> +        }
> +        memcpy(llc_colors, config->llc_colors.p, bytes);
> +        set_xen_guest_handle(config->llc_colors, llc_colors);

I think this two lines looks wrong. There is a double usage of
config->llc_colors, to both store a user pointer and then to store an
hypercall buffer. Also, accessing llc_colors.p field is probably wrong.
I guess the caller of xc_domain_create() (that is libxl) will have to
take care of the hypercall buffer. It is already filling the
xen_domctl_createdomain struct that is passed to the hypercall, so it's
probably fine to handle hypercall buffer which is part of it.

What happen in the hypervisor when both num_llc_colors and llc_colors
are set to 0 in the struct xen_domctl_createdomain? Is it fine? That is
to figure out if all users of xc_domain_create() needs to be updated.

Also, ocaml binding is "broken", or at least needs updating. This is due
to the addition of llc_colors into xen_domctl_createdomain, the size
been different than the expected size.


> +    }
>  
>      domctl.cmd = XEN_DOMCTL_createdomain;
>      domctl.domain = *pdomid;
> @@ -39,6 +53,9 @@ int xc_domain_create(xc_interface *xch, uint32_t *pdomid,
>      *pdomid = (uint16_t)domctl.domain;
>      *config = domctl.u.createdomain;
>  
> +    if ( llc_colors )
> +        xc_hypercall_buffer_free(xch, llc_colors);
> +
>      return 0;
>  }
>  
> diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
> index beec3f6b6f..6d0c768241 100644
> --- a/tools/libs/light/libxl_create.c
> +++ b/tools/libs/light/libxl_create.c
> @@ -638,6 +638,8 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config 
> *d_config,
>              .grant_opts = 
> XEN_DOMCTL_GRANT_version(b_info->max_grant_version),
>              .vmtrace_size = ROUNDUP(b_info->vmtrace_buf_kb << 10, 
> XC_PAGE_SHIFT),
>              .cpupool_id = info->poolid,
> +            .num_llc_colors = b_info->num_llc_colors,
> +            .llc_colors.p = b_info->llc_colors,
>          };
>  
>          if (info->type != LIBXL_DOMAIN_TYPE_PV) {
> diff --git a/tools/libs/light/libxl_types.idl 
> b/tools/libs/light/libxl_types.idl
> index 0cfad8508d..1f944ca6d7 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -562,6 +562,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>      ("ioports",          Array(libxl_ioport_range, "num_ioports")),
>      ("irqs",             Array(uint32, "num_irqs")),
>      ("iomem",            Array(libxl_iomem_range, "num_iomem")),
> +    ("llc_colors",       Array(uint32, "num_llc_colors")),

For this, you are going to need to add a LIBXL_HAVE_ macro in libxl.h.
They are plenty of example as well as an explanation.
But a good name I guess would be LIBXL_HAVE_BUILDINFO_LLC_COLORS along
with a short comment.

>      ("claim_mode",        libxl_defbool),
>      ("event_channels",   uint32),
>      ("kernel",           string),


Thanks,

-- 
Anthony PERARD