Re: [PATCH v3 4/9] tools/xl: add support for cache coloring configuration

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Sat, Oct 22, 2022 at 05:51:15PM +0200, Carlo Nonato wrote:
> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> index b2901e04cf..5f53cec8bf 100644
> --- a/docs/man/xl.cfg.5.pod.in
> +++ b/docs/man/xl.cfg.5.pod.in
> @@ -2880,6 +2880,16 @@ Currently, only the "sbsa_uart" model is supported for 
> ARM.
>  
>  =back
>  
> +=over 4
> +
> +=item B<colors=[ "COLORS_RANGE", "COLORS_RANGE", ...]>

Instead of COLORS_RANGE, maybe NUMBER_RANGE? Or just RANGE?

> +Specify the LLC color configuration for the guest. B<COLORS_RANGE> can be 
> either
> +a single color value or a hypen-separated closed interval of colors
> +(such as "0-4").

Does "yellow-blue" works? Or "red-violet", to have a rainbow? :-)

So, "colors" as the name of a new configuration option isn't going to
work. To me, that would refer to VM managment, like maybe help to
categorise VM in some kind of tools, and not a part of the configuration
of the domain.

Could you invent a name that is more specific? Maybe "cache_colors" or
something, or "vcpu_cache_colors".

> +=back
> +
>  =head3 x86
>  
>  =over 4
> diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
> index b9dd2deedf..94c511912c 100644
> --- a/tools/libs/light/libxl_create.c
> +++ b/tools/libs/light/libxl_create.c
> @@ -615,6 +615,7 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config 
> *d_config,
>      struct xs_permissions rwperm[1];
>      struct xs_permissions noperm[1];
>      xs_transaction_t t = 0;
> +    DECLARE_HYPERCALL_BUFFER(unsigned int, colors);
>  
>      /* convenience aliases */
>      libxl_domain_create_info *info = &d_config->c_info;
> @@ -676,6 +677,16 @@ int libxl__domain_make(libxl__gc *gc, 
> libxl_domain_config *d_config,
>              goto out;
>          }
>  
> +        if (d_config->b_info.num_colors) {
> +            size_t bytes = sizeof(unsigned int) * 
> d_config->b_info.num_colors;
> +            colors = xc_hypercall_buffer_alloc(ctx->xch, colors, bytes);

Hypercall stuff is normally done in another library, libxenctrl (or
maybe others like libxenguest). Is there a reason to do that here?

> +            memcpy(colors, d_config->b_info.colors, bytes);
> +            set_xen_guest_handle(create.arch.colors, colors);
> +            create.arch.num_colors = d_config->b_info.num_colors;
> +            create.arch.from_guest = 1;

"arch" stuff is better dealt with in libxl__arch_domain_prepare_config().
(unless it isn't arch specific in the next revision of the series)

> +            LOG(DEBUG, "Setup %u domain colors", 
> d_config->b_info.num_colors);



> +        }
> +
>          for (;;) {
>              uint32_t local_domid;
>              bool recent;
> @@ -922,6 +933,7 @@ retry_transaction:
>      rc = 0;
>   out:
>      if (t) xs_transaction_end(ctx->xsh, t, 1);
> +    if (colors) xc_hypercall_buffer_free(ctx->xch, colors);
>      return rc;
>  }
>  
> diff --git a/tools/libs/light/libxl_types.idl 
> b/tools/libs/light/libxl_types.idl
> index d634f304cd..642173af1a 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -557,6 +557,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")),
> +    ("colors",           Array(uint32, "num_colors")),

So the colors is added to arch specific config in
xen_domctl_createdomain, maybe we should do the same here and move it to
the "arch_arm" struct. Or if that is declared common in hypervisor, then
it is file to leave it here.

Also, "colors" needs to be rename to something more specific.

>      ("claim_mode",        libxl_defbool),
>      ("event_channels",   uint32),
>      ("kernel",           string),
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index 1b5381cef0..e6b2c7acff 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -1220,8 +1220,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;
> -    int num_ioports, num_irqs, num_iomem, num_cpus, num_viridian, 
> num_mca_caps;
> +                   *mca_caps, *colors;
> +    int num_ioports, num_irqs, num_iomem, num_cpus, num_viridian, 
> num_mca_caps,
> +        num_colors;

Please, add a new lines instead of increasing the number of declared
variable on a single line.

>      int pci_power_mgmt = 0;
>      int pci_msitranslate = 0;
>      int pci_permissive = 0;
> @@ -1370,6 +1371,53 @@ 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, "colors", &colors, &num_colors, 0)) {
> +        int k, p, cur_index = 0;
> +
> +        b_info->num_colors = 0;
> +        /* Get number of colors based on ranges */
> +        for (i = 0; i < num_colors; i++) {
> +            uint32_t start = 0, end = 0;
> +
> +            buf = xlu_cfg_get_listitem(colors, i);
> +            if (!buf) {
> +                fprintf(stderr,
> +                    "xl: Unable to get element %d in colors range list\n", 
> i);
> +                exit(1);
> +            }
> +
> +            if (sscanf(buf, "%u-%u", &start, &end) != 2) {
> +                if (sscanf(buf, "%u", &start) != 1) {

I think you want %"SCNu32" instead of %u as both start and end are
uint32_t.

> +                    fprintf(stderr, "xl: Invalid color range: %s\n", buf);
> +                    exit(1);
> +                }
> +                end = start;
> +            }
> +            else if (start > end) {

Can you put the "else" on the same line as "}" ?
> +                fprintf(stderr,
> +                        "xl: Start color is greater than end color: %s\n", 
> buf);
> +                exit(1);
> +            }
> +
> +            /* Check for overlaps */
> +            for (k = start; k <= end; k++) {
> +                for (p = 0; p < b_info->num_colors; p++) {
> +                    if (b_info->colors[p] == k) {
> +                        fprintf(stderr, "xl: Overlapped ranges not 
> allowed\n");

Why is that an issue? Could overlap just been ignored?

> +                        exit(1);
> +                    }
> +                }
> +            }
> +
> +            b_info->num_colors += (end - start) + 1;
> +            b_info->colors = (uint32_t *)realloc(b_info->colors,
> +                                sizeof(*b_info->colors) * 
> b_info->num_colors);
> +
> +            for (k = start; k <= end; k++)
> +                b_info->colors[cur_index++] = k;

This `b_info->colors` feels like it could be a bitmap like for "vcpus"
or other config that deal with ranges.

libxl has plenty of functions to deal with bitmap that xl can use,
starting with libxl_bitmap_alloc(), maybe it would make dealing with
cache color easier, like no need to check for overlaps, but there
doesn't seems to be a function to deal with a growing bitmap so one
would have to find the highest cache value before allocating the bitmap
are deal with realloc.

I guess using bitmap or not kind of depend of the interface with the
hypervisor, if it take a list of number, then a list of number is fine
here too.


Thanks,

-- 
Anthony PERARD