Re: [PATCH v6 05/15] xen: extend domctl interface for cache coloring

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

Hi Jan,

On Thu, Feb 1, 2024 at 2:51 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 29.01.2024 18:18, Carlo Nonato wrote:
> > @@ -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;
>
> With "ret" still being 0, this amounts to "successfully ignored". Ought
> to be -EOPNOTSUPP, I guess.

Yep.

> > +        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");
>
> If at all a dprintk(). But personally I think even that's too much - we
> don't do so elsewhere, I don't think.

I'm going to drop the printk.

> > --- a/xen/common/llc-coloring.c
> > +++ b/xen/common/llc-coloring.c
> > @@ -4,6 +4,7 @@
> >   *
> >   * Copyright (C) 2022 Xilinx Inc.
> >   */
> > +#include <xen/guest_access.h>
> >  #include <xen/keyhandler.h>
> >  #include <xen/llc-coloring.h>
> >  #include <xen/param.h>
> > @@ -229,6 +230,30 @@ int __init 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)
>
> What purpose has the "domctl" in the function name?

To signal that it's called from domctl. Do you suggest leaving it out?

> > +{
> > +    unsigned int *colors;
> > +
> > +    if ( d->num_llc_colors )
> > +        return -EEXIST;
> > +
> > +    if ( !config->num_llc_colors )
> > +        return domain_set_default_colors(d);
> > +
> > +    colors = alloc_colors(config->num_llc_colors);
> > +    if ( !colors )
> > +        return -ENOMEM;
>
> Hmm, I see here you call the function without first having bounds checked
> the input. But in case of too big a value, -ENOMEM is inappropriate, so
> such a check wants adding up front anyway.

Yeah, ok.

> > +    if ( copy_from_guest(colors, config->llc_colors, 
> > config->num_llc_colors) )
> > +        return -EFAULT;
>
> There again wants to be a check that the pointed to values are the same,
> or at least of the same size. Else you'd need to do element-wise copy.
>
> > +    d->llc_colors = colors;
> > +    d->num_llc_colors = config->num_llc_colors;
> > +
> > +    return domain_check_colors(d);
>
> And if this fails, you leave the domain with the bad settings? Shouldn't
> you check and only then store pointer and count?

Yes. I'm gonna change it.

Thanks.

> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -1190,6 +1190,13 @@ 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 */
> > +    uint32_t num_llc_colors;
> > +    uint32_t padding;
>
> I see you've added padding, but: You don't check it to be zero. Plus
> the overwhelming majority of padding fields is named "pad".
>
> Jan