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

Commit Message

Carlo Nonato Jan. 29, 2024, 5:18 p.m. UTC

Update the domctl interface to allow the user to set coloring configurations
from the toolstack.

Implement also the functionality for arm64.

Based on original work from: Luca Miccio <[e-mail]>

Signed-off-by: Carlo Nonato <[e-mail]>
Signed-off-by: Marco Solieri <[e-mail]>
---
v6:
- reverted the XEN_DOMCTL_INTERFACE_VERSION bump
- reverted to uint32 for the guest handle
- explicit padding added to the domctl struct
- rewrote domain_set_llc_colors_domctl() to be more explicit
v5:
- added a new hypercall to set colors
- uint for the guest handle
v4:
- updated XEN_DOMCTL_INTERFACE_VERSION
---
 xen/common/domctl.c            | 11 +++++++++++
 xen/common/llc-coloring.c      | 25 +++++++++++++++++++++++++
 xen/include/public/domctl.h    |  9 +++++++++
 xen/include/xen/llc-coloring.h |  3 +++
 4 files changed, 48 insertions(+)

Comments

Jan Beulich Feb. 1, 2024, 1:51 p.m. UTC | #1

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.

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

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

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

> +    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?

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

Carlo Nonato Feb. 3, 2024, 11:41 a.m. UTC | #2

Hi Jan,

On Thu, Feb 1, 2024 at 2:51 PM Jan Beulich <[e-mail]> 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

Jan Beulich Feb. 5, 2024, 9:37 a.m. UTC | #3

On 03.02.2024 12:41, Carlo Nonato wrote:
> On Thu, Feb 1, 2024 at 2:51 PM Jan Beulich <[e-mail]> wrote:
>> On 29.01.2024 18:18, Carlo Nonato wrote:
>>> --- 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?

Yes. Names want to be descriptive, but also not be overly long. Imo.

Jan

Carlo Nonato Feb. 6, 2024, 11:46 a.m. UTC | #4

Hi Jan,

On Thu, Feb 1, 2024 at 2:51 PM Jan Beulich <[e-mail]> 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.
>
> > +        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.
>
> > --- 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?
>
> > +{
> > +    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.
>
> > +    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.

Sorry to bring this back again, but I've just noticed copy_from_guest()
already checks for type compatibility. For what regards the size I don't think
I understood what to check. colors is defined to be of the same size of
config->llc_colors.

Thanks.

> > +    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?
>
> > --- 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

Jan Beulich Feb. 6, 2024, 11:51 a.m. UTC | #5

On 06.02.2024 12:46, Carlo Nonato wrote:
> On Thu, Feb 1, 2024 at 2:51 PM Jan Beulich <[e-mail]> wrote:
>> On 29.01.2024 18:18, Carlo Nonato wrote:
>>> @@ -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?
>>
>>> +{
>>> +    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.
>>
>>> +    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.
> 
> Sorry to bring this back again, but I've just noticed copy_from_guest()
> already checks for type compatibility. For what regards the size I don't think
> I understood what to check. colors is defined to be of the same size of
> config->llc_colors.

Oh, you're right. But the other case was a memcpy(), wasn't it?

Jan

Carlo Nonato Feb. 6, 2024, noon UTC | #6

On Tue, Feb 6, 2024 at 12:51 PM Jan Beulich <[e-mail]> wrote:
>
> On 06.02.2024 12:46, Carlo Nonato wrote:
> > On Thu, Feb 1, 2024 at 2:51 PM Jan Beulich <[e-mail]> wrote:
> >> On 29.01.2024 18:18, Carlo Nonato wrote:
> >>> @@ -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?
> >>
> >>> +{
> >>> +    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.
> >>
> >>> +    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.
> >
> > Sorry to bring this back again, but I've just noticed copy_from_guest()
> > already checks for type compatibility. For what regards the size I don't think
> > I understood what to check. colors is defined to be of the same size of
> > config->llc_colors.
>
> Oh, you're right. But the other case was a memcpy(), wasn't it?

Yes, in that case your suggestion was needed.

Thanks again.

> Jan

Patch

diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index f5a71ee5f7..b6867d0602 100644
--- 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;
diff --git a/xen/common/llc-coloring.c b/xen/common/llc-coloring.c
index 983de44a47..aaf0606c00 100644
--- 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)
+{
+    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;
+
+    if ( copy_from_guest(colors, config->llc_colors, config->num_llc_colors) )
+        return -EFAULT;
+
+    d->llc_colors = colors;
+    d->num_llc_colors = config->num_llc_colors;
+
+    return domain_check_colors(d);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index a33f9ec32b..d090cdb2dd 100644
--- 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;
+    XEN_GUEST_HANDLE_64(uint32) llc_colors;
+};
+
 struct xen_domctl {
     uint32_t cmd;
 #define XEN_DOMCTL_createdomain                   1
@@ -1277,6 +1284,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 +1347,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;
 };
diff --git a/xen/include/xen/llc-coloring.h b/xen/include/xen/llc-coloring.h
index 1a73080c98..a82081367f 100644
--- a/xen/include/xen/llc-coloring.h
+++ b/xen/include/xen/llc-coloring.h
@@ -28,6 +28,9 @@  unsigned int get_llc_way_size(void);
 void arch_llc_coloring_init(void);
 int dom0_set_llc_colors(struct domain *d);
 
+int domain_set_llc_colors_domctl(struct domain *d,
+                                 const struct xen_domctl_set_llc_colors *config);
+
 #endif /* __COLORING_H__ */
 
 /*