diff mbox series

[v7,05/14] xen: extend domctl interface for cache coloring

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

Commit Message

Carlo Nonato March 15, 2024, 10:58 a.m. UTC 
Add a new domctl hypercall to allow the user to set LLC coloring
configurations. Colors can be set only once, just after domain creation,
since recoloring isn't supported.

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>
---
v7:
- -EOPNOTSUPP returned in case of hypercall called without llc_coloring_enabled
- domain_set_llc_colors_domctl() renamed to domain_set_llc_colors()
- added padding and input bound checks to domain_set_llc_colors()
- removed alloc_colors() helper usage from domain_set_llc_colors()
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            |  8 ++++++++
 xen/common/llc-coloring.c      | 34 ++++++++++++++++++++++++++++++++++
 xen/include/public/domctl.h    |  9 +++++++++
 xen/include/xen/llc-coloring.h |  2 ++
 4 files changed, 53 insertions(+)

Comments

Jan Beulich March 19, 2024, 3:37 p.m. UTC | #1 
On 15.03.2024 11:58, Carlo Nonato wrote:
> @@ -219,6 +220,39 @@ void domain_llc_coloring_free(struct domain *d)
>      xfree(__va(__pa(d->llc_colors)));
>  }
>  
> +int domain_set_llc_colors(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);
> +
> +    if ( config->num_llc_colors > max_nr_colors || config->pad )

The check of "pad" wants carrying out in all cases; I expect it wants
moving to the caller.

> +        return -EINVAL;
> +
> +    colors = xmalloc_array(unsigned int, config->num_llc_colors);
> +    if ( !colors )
> +        return -ENOMEM;
> +
> +    if ( copy_from_guest(colors, config->llc_colors, config->num_llc_colors) )
> +        return -EFAULT;

You're leaking "colors" when taking this or ...

> +    if ( !check_colors(colors, config->num_llc_colors) )
> +    {
> +        printk(XENLOG_ERR "Bad LLC color config for %pd\n", d);
> +        return -EINVAL;

... this error path.

Jan
Carlo Nonato March 21, 2024, 3:11 p.m. UTC | #2 
Hi Jan,

On Tue, Mar 19, 2024 at 4:37 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 15.03.2024 11:58, Carlo Nonato wrote:
> > @@ -219,6 +220,39 @@ void domain_llc_coloring_free(struct domain *d)
> >      xfree(__va(__pa(d->llc_colors)));
> >  }
> >
> > +int domain_set_llc_colors(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);
> > +
> > +    if ( config->num_llc_colors > max_nr_colors || config->pad )
>
> The check of "pad" wants carrying out in all cases; I expect it wants
> moving to the caller.

Ok.

> > +        return -EINVAL;
> > +
> > +    colors = xmalloc_array(unsigned int, config->num_llc_colors);
> > +    if ( !colors )
> > +        return -ENOMEM;
> > +
> > +    if ( copy_from_guest(colors, config->llc_colors, config->num_llc_colors) )
> > +        return -EFAULT;
>
> You're leaking "colors" when taking this or ...
>
> > +    if ( !check_colors(colors, config->num_llc_colors) )
> > +    {
> > +        printk(XENLOG_ERR "Bad LLC color config for %pd\n", d);
> > +        return -EINVAL;
>
> ... this error path.

You're right.

> Jan

Thanks.

On Tue, Mar 19, 2024 at 4:37 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 15.03.2024 11:58, Carlo Nonato wrote:
> > @@ -219,6 +220,39 @@ void domain_llc_coloring_free(struct domain *d)
> >      xfree(__va(__pa(d->llc_colors)));
> >  }
> >
> > +int domain_set_llc_colors(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);
> > +
> > +    if ( config->num_llc_colors > max_nr_colors || config->pad )
>
> The check of "pad" wants carrying out in all cases; I expect it wants
> moving to the caller.
>
> > +        return -EINVAL;
> > +
> > +    colors = xmalloc_array(unsigned int, config->num_llc_colors);
> > +    if ( !colors )
> > +        return -ENOMEM;
> > +
> > +    if ( copy_from_guest(colors, config->llc_colors, config->num_llc_colors) )
> > +        return -EFAULT;
>
> You're leaking "colors" when taking this or ...
>
> > +    if ( !check_colors(colors, config->num_llc_colors) )
> > +    {
> > +        printk(XENLOG_ERR "Bad LLC color config for %pd\n", d);
> > +        return -EINVAL;
>
> ... this error path.
>
> Jan
diff mbox series

Patch

diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index f5a71ee5f7..6c940ac833 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,13 @@  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 )
+            ret = domain_set_llc_colors(d, &op->u.set_llc_colors);
+        else
+            ret = -EOPNOTSUPP;
+        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 ebd7087dc2..9c1f152b96 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>
@@ -219,6 +220,39 @@  void domain_llc_coloring_free(struct domain *d)
     xfree(__va(__pa(d->llc_colors)));
 }
 
+int domain_set_llc_colors(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);
+
+    if ( config->num_llc_colors > max_nr_colors || config->pad )
+        return -EINVAL;
+
+    colors = xmalloc_array(unsigned int, config->num_llc_colors);
+    if ( !colors )
+        return -ENOMEM;
+
+    if ( copy_from_guest(colors, config->llc_colors, config->num_llc_colors) )
+        return -EFAULT;
+
+    if ( !check_colors(colors, config->num_llc_colors) )
+    {
+        printk(XENLOG_ERR "Bad LLC color config for %pd\n", d);
+        return -EINVAL;
+    }
+
+    d->llc_colors = colors;
+    d->num_llc_colors = config->num_llc_colors;
+
+    return 0;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index a33f9ec32b..d44eac8775 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 pad;
+    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 ee82932266..b3801fca00 100644
--- a/xen/include/xen/llc-coloring.h
+++ b/xen/include/xen/llc-coloring.h
@@ -29,6 +29,8 @@  static inline void domain_llc_coloring_free(struct domain *d) {}
 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(struct domain *d,
+                          const struct xen_domctl_set_llc_colors *config);
 
 #endif /* __COLORING_H__ */