Re: [PATCH v5 04/13] xen: extend domctl interface for cache coloring

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

Hi Jan,

On Mon, Jan 8, 2024 at 9:43 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 02.01.2024 10:51, Carlo Nonato wrote:
> > --- a/xen/arch/arm/llc-coloring.c
> > +++ b/xen/arch/arm/llc-coloring.c
> > @@ -9,6 +9,7 @@
> >   *    Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>
> >   */
> >  #include <xen/errno.h>
> > +#include <xen/guest_access.h>
> >  #include <xen/keyhandler.h>
> >  #include <xen/llc-coloring.h>
> >  #include <xen/param.h>
> > @@ -278,6 +279,22 @@ int dom0_set_llc_colors(struct domain *d)
> >      return domain_check_colors(d);
> >  }
> >
> > +int domain_set_llc_colors_domctl(struct domain *d,
> > +                                 const struct xen_domctl_set_llc_colors 
> > *config)
> > +{
> > +    if ( d->num_llc_colors )
> > +        return -EEXIST;
> > +
> > +    if ( domain_alloc_colors(d, config->num_llc_colors) )
> > +        return -ENOMEM;
> > +
> > +    if ( copy_from_guest(d->llc_colors, config->llc_colors,
> > +                         config->num_llc_colors) )
> > +        return -EFAULT;
> > +
> > +    return domain_check_colors(d);
> > +}
>
> What part of this is Arm-specific? I ask in particular because while you
> place this in an Arm-specific source file, ...
>
> > --- a/xen/common/domctl.c
> > +++ b/xen/common/domctl.c
> > @@ -8,6 +8,7 @@
> >
> >  #include <xen/types.h>
> >  #include <xen/lib.h>
> > +#include <xen/llc-coloring.h>
> >  #include <xen/err.h>
> >  #include <xen/mm.h>
> >  #include <xen/sched.h>
> > @@ -858,6 +859,16 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> > u_domctl)
> >                  __HYPERVISOR_domctl, "h", u_domctl);
> >          break;
> >
> > +    case XEN_DOMCTL_set_llc_colors:
> > +        if ( !llc_coloring_enabled )
> > +            break;
> > +
> > +        ret = domain_set_llc_colors_domctl(d, &op->u.set_llc_colors);
> > +        if ( ret == -EEXIST )
> > +            printk(XENLOG_ERR
> > +                   "Can't set LLC colors on an already created domain\n");
> > +        break;
> > +
> >      default:
> >          ret = arch_do_domctl(op, d, u_domctl);
> >          break;
>
> ... you don't handle the new domctl in Arm's arch_do_domctl().

No arm specific code here. I need a new xen/common/llc-coloring.c file where
to put common stuff.

> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -21,7 +21,7 @@
> >  #include "hvm/save.h"
> >  #include "memory.h"
> >
> > -#define XEN_DOMCTL_INTERFACE_VERSION 0x00000016
> > +#define XEN_DOMCTL_INTERFACE_VERSION 0x00000017
>
> There's no need for such a bump when ...
>
> > @@ -1190,6 +1190,12 @@ struct xen_domctl_vmtrace_op {
> >  typedef struct xen_domctl_vmtrace_op xen_domctl_vmtrace_op_t;
> >  DEFINE_XEN_GUEST_HANDLE(xen_domctl_vmtrace_op_t);
> >
> > +struct xen_domctl_set_llc_colors {
> > +    /* IN LLC coloring parameters */
> > +    unsigned int num_llc_colors;
> > +    XEN_GUEST_HANDLE_64(uint) llc_colors;
> > +};
> > +
> >  struct xen_domctl {
> >      uint32_t cmd;
> >  #define XEN_DOMCTL_createdomain                   1
> > @@ -1277,6 +1283,7 @@ struct xen_domctl {
> >  #define XEN_DOMCTL_vmtrace_op                    84
> >  #define XEN_DOMCTL_get_paging_mempool_size       85
> >  #define XEN_DOMCTL_set_paging_mempool_size       86
> > +#define XEN_DOMCTL_set_llc_colors                87
> >  #define XEN_DOMCTL_gdbsx_guestmemio            1000
> >  #define XEN_DOMCTL_gdbsx_pausevcpu             1001
> >  #define XEN_DOMCTL_gdbsx_unpausevcpu           1002
> > @@ -1339,6 +1346,7 @@ struct xen_domctl {
> >          struct xen_domctl_vuart_op          vuart_op;
> >          struct xen_domctl_vmtrace_op        vmtrace_op;
> >          struct xen_domctl_paging_mempool    paging_mempool;
> > +        struct xen_domctl_set_llc_colors    set_llc_colors;
> >          uint8_t                             pad[128];
> >      } u;
> >  };
>
> ... all you do is add a new domctl.
>
> As to the new struct - you'll want to use uint<N>_t there, not
> unsigned int.
>
> Jan

Thanks.