diff mbox series

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

Message ID 20230123154735.74832-6-carlo.nonato@minervasys.tech (mailing list archive)
State New, archived
Headers show
Series Arm cache coloring | expand

Commit Message

Carlo Nonato Jan. 23, 2023, 3:47 p.m. UTC 
Add a new "llc_colors" parameter that defines the LLC color assignment for
a domain. The user can specify one or more color ranges using the same
syntax used everywhere else for color config described in the
documentation.
The parameter is defined as a list of strings that represent the color
ranges.

Documentation is also added.

Based on original work from: Luca Miccio <lucmiccio@gmail.com>

Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech>
---
v4:
- removed overlapping color ranges checks during parsing
- moved hypercall buffer initialization in libxenctrl
---
 docs/man/xl.cfg.5.pod.in         | 10 +++++++++
 tools/libs/ctrl/xc_domain.c      | 17 ++++++++++++++
 tools/libs/light/libxl_create.c  |  2 ++
 tools/libs/light/libxl_types.idl |  1 +
 tools/xl/xl_parse.c              | 38 +++++++++++++++++++++++++++++++-
 5 files changed, 67 insertions(+), 1 deletion(-)

Comments

Anthony PERARD Jan. 26, 2023, 2:22 p.m. UTC | #1 
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,
Carlo Nonato Jan. 26, 2023, 4:34 p.m. UTC | #2 
Hi Anthony,

On Thu, Jan 26, 2023 at 3:22 PM Anthony PERARD
<anthony.perard@citrix.com> 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
diff mbox series

Patch

diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index 024bceeb61..96f9249c3d 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -2903,6 +2903,16 @@  Currently, only the "sbsa_uart" model is supported for ARM.
 
 =back
 
+=over 4
+
+=item B<llc_colors=[ "RANGE", "RANGE", ...]>
+
+Specify the Last Level Cache (LLC) color configuration for the guest.
+B<RANGE> can be either a single color value or a hypen-separated closed
+interval of colors (such as "0-4").
+
+=back
+
 =head3 x86
 
 =over 4
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);
+    }
 
     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")),
     ("claim_mode",	     libxl_defbool),
     ("event_channels",   uint32),
     ("kernel",           string),
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 853e9f357a..0f8c469fb5 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -1297,8 +1297,9 @@  void parse_config_data(const char *config_source,
     XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids, *vtpms,
                    *usbctrls, *usbdevs, *p9devs, *vdispls, *pvcallsifs_devs;
     XLU_ConfigList *channels, *ioports, *irqs, *iomem, *viridian, *dtdevs,
-                   *mca_caps;
+                   *mca_caps, *llc_colors;
     int num_ioports, num_irqs, num_iomem, num_cpus, num_viridian, num_mca_caps;
+    int num_llc_colors;
     int pci_power_mgmt = 0;
     int pci_msitranslate = 0;
     int pci_permissive = 0;
@@ -1447,6 +1448,41 @@  void parse_config_data(const char *config_source,
     if (!xlu_cfg_get_long (config, "maxmem", &l, 0))
         b_info->max_memkb = l * 1024;
 
+    if (!xlu_cfg_get_list(config, "llc_colors", &llc_colors, &num_llc_colors, 0)) {
+        int k, cur_index = 0;
+
+        b_info->num_llc_colors = 0;
+        for (i = 0; i < num_llc_colors; i++) {
+            uint32_t start = 0, end = 0;
+
+            buf = xlu_cfg_get_listitem(llc_colors, i);
+            if (!buf) {
+                fprintf(stderr,
+                        "xl: Can't get element %d in LLC color list\n", i);
+                exit(1);
+            }
+
+            if (sscanf(buf, "%" SCNu32 "-%" SCNu32, &start, &end) != 2) {
+                if (sscanf(buf, "%" SCNu32, &start) != 1) {
+                    fprintf(stderr, "xl: Invalid LLC color range: %s\n", buf);
+                    exit(1);
+                }
+                end = start;
+            } else if (start > end) {
+                fprintf(stderr,
+                        "xl: Start LLC color is greater than end: %s\n", buf);
+                exit(1);
+            }
+
+            b_info->num_llc_colors += (end - start) + 1;
+            b_info->llc_colors = (uint32_t *)realloc(b_info->llc_colors,
+                        sizeof(*b_info->llc_colors) * b_info->num_llc_colors);
+
+            for (k = start; k <= end; k++)
+                b_info->llc_colors[cur_index++] = k;
+        }
+    }
+
     if (!xlu_cfg_get_long (config, "vcpus", &l, 0)) {
         vcpus = l;
         if (libxl_cpu_bitmap_alloc(ctx, &b_info->avail_vcpus, l)) {