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

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

Hi Anthony,

On Thu, Jan 26, 2023 at 3:22 PM Anthony PERARD
<anthony.perard@xxxxxxxxxx> wrote:
>
> 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.

This is what I did in v3 :) (https://marc.info/?l=xen-devel&m=166930291506578)
However things probably will change again because of a new interface in Xen.

> 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.

A default coloring configuration is generated if the array is null or its
length is 0. So no problems on the Xen side.

> 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.

Ok, thanks for the explanation.

> >      ("claim_mode",        libxl_defbool),
> >      ("event_channels",   uint32),
> >      ("kernel",           string),
>
>
> Thanks,
>
> --
> Anthony PERARD