diff mbox series

[v4,04/11] xen: extend domctl interface for cache coloring

Message ID 20230123154735.74832-5-carlo.nonato@minervasys.tech (mailing list archive)
State New, archived
Headers show
Series Arm cache coloring | expand

Commit Message

Carlo Nonato Jan. 23, 2023, 3:47 p.m. UTC 
This commit updates the domctl interface to allow the user to set cache
coloring configurations from the toolstack.
It also implements the functionality for arm64.

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>
---
v4:
- updated XEN_DOMCTL_INTERFACE_VERSION
---
 xen/arch/arm/llc_coloring.c    | 14 ++++++++++++++
 xen/common/domctl.c            | 12 +++++++++++-
 xen/include/public/domctl.h    |  6 +++++-
 xen/include/xen/llc_coloring.h |  4 ++++
 4 files changed, 34 insertions(+), 2 deletions(-)

Comments

Jan Beulich Jan. 24, 2023, 4:29 p.m. UTC | #1 
On 23.01.2023 16:47, Carlo Nonato wrote:
> @@ -275,6 +276,19 @@ unsigned int *dom0_llc_colors(unsigned int *num_colors)
>      return colors;
>  }
>  
> +unsigned int *llc_colors_from_guest(struct xen_domctl_createdomain *config)

const struct ...?

> +{
> +    unsigned int *colors;
> +
> +    if ( !config->num_llc_colors )
> +        return NULL;
> +
> +    colors = alloc_colors(config->num_llc_colors);

Error handling needs to occur here; the panic() in alloc_colors() needs
to go away.

> @@ -434,7 +436,15 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>              rover = dom;
>          }
>  
> -        d = domain_create(dom, &op->u.createdomain, false);
> +        if ( llc_coloring_enabled )
> +        {
> +            llc_colors = llc_colors_from_guest(&op->u.createdomain);
> +            num_llc_colors = op->u.createdomain.num_llc_colors;

I think you would better avoid setting num_llc_colors to non-zero if
you got back NULL from the function. It's at best confusing.

> @@ -92,6 +92,10 @@ struct xen_domctl_createdomain {
>      /* CPU pool to use; specify 0 or a specific existing pool */
>      uint32_t cpupool_id;
>  
> +    /* IN LLC coloring parameters */
> +    uint32_t num_llc_colors;
> +    XEN_GUEST_HANDLE(uint32) llc_colors;

Despite your earlier replies I continue to be unconvinced that this
is information which needs to be available right at domain_create.
Without that you'd also get away without the sufficiently odd
domain_create_llc_colored(). (Odd because: Think of two or three
more extended features appearing, all of which want a special cased
domain_create().)

Jan
Carlo Nonato Jan. 25, 2023, 4:27 p.m. UTC | #2 
Hi Jan,

On Tue, Jan 24, 2023 at 5:29 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 23.01.2023 16:47, Carlo Nonato wrote:
> > @@ -275,6 +276,19 @@ unsigned int *dom0_llc_colors(unsigned int *num_colors)
> >      return colors;
> >  }
> >
> > +unsigned int *llc_colors_from_guest(struct xen_domctl_createdomain *config)
>
> const struct ...?
>
> > +{
> > +    unsigned int *colors;
> > +
> > +    if ( !config->num_llc_colors )
> > +        return NULL;
> > +
> > +    colors = alloc_colors(config->num_llc_colors);
>
> Error handling needs to occur here; the panic() in alloc_colors() needs
> to go away.
>
> > @@ -434,7 +436,15 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> >              rover = dom;
> >          }
> >
> > -        d = domain_create(dom, &op->u.createdomain, false);
> > +        if ( llc_coloring_enabled )
> > +        {
> > +            llc_colors = llc_colors_from_guest(&op->u.createdomain);
> > +            num_llc_colors = op->u.createdomain.num_llc_colors;
>
> I think you would better avoid setting num_llc_colors to non-zero if
> you got back NULL from the function. It's at best confusing.
>
> > @@ -92,6 +92,10 @@ struct xen_domctl_createdomain {
> >      /* CPU pool to use; specify 0 or a specific existing pool */
> >      uint32_t cpupool_id;
> >
> > +    /* IN LLC coloring parameters */
> > +    uint32_t num_llc_colors;
> > +    XEN_GUEST_HANDLE(uint32) llc_colors;
>
> Despite your earlier replies I continue to be unconvinced that this
> is information which needs to be available right at domain_create.
> Without that you'd also get away without the sufficiently odd
> domain_create_llc_colored(). (Odd because: Think of two or three
> more extended features appearing, all of which want a special cased
> domain_create().)

Yes, I definitely see your point. Still there is the p2m table allocation
problem that you and Julien have discussed previously. I'm not sure I
understood what the approach is.

> Jan
Jan Beulich Jan. 26, 2023, 7:25 a.m. UTC | #3 
On 24.01.2023 17:29, Jan Beulich wrote:
> On 23.01.2023 16:47, Carlo Nonato wrote:
>> @@ -92,6 +92,10 @@ struct xen_domctl_createdomain {
>>      /* CPU pool to use; specify 0 or a specific existing pool */
>>      uint32_t cpupool_id;
>>  
>> +    /* IN LLC coloring parameters */
>> +    uint32_t num_llc_colors;
>> +    XEN_GUEST_HANDLE(uint32) llc_colors;
> 
> Despite your earlier replies I continue to be unconvinced that this
> is information which needs to be available right at domain_create.
> Without that you'd also get away without the sufficiently odd
> domain_create_llc_colored(). (Odd because: Think of two or three
> more extended features appearing, all of which want a special cased
> domain_create().)

And perhaps the real question is: Why do the two items need passing
to a special variant of domain_create() in the first place? The
necessary information already is passed to the normal function via
struct xen_domctl_createdomain. All it would take is to read the
array from guest space later, when struct domain was already
allocated and is hence available for storing the pointer. (Passing
the count separately is redundant in any event.)

Jan
Julien Grall Jan. 26, 2023, 10:20 a.m. UTC | #4 
Hi,

On 25/01/2023 16:27, Carlo Nonato wrote:
> On Tue, Jan 24, 2023 at 5:29 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 23.01.2023 16:47, Carlo Nonato wrote:
>>> @@ -275,6 +276,19 @@ unsigned int *dom0_llc_colors(unsigned int *num_colors)
>>>       return colors;
>>>   }
>>>
>>> +unsigned int *llc_colors_from_guest(struct xen_domctl_createdomain *config)
>>
>> const struct ...?
>>
>>> +{
>>> +    unsigned int *colors;
>>> +
>>> +    if ( !config->num_llc_colors )
>>> +        return NULL;
>>> +
>>> +    colors = alloc_colors(config->num_llc_colors);
>>
>> Error handling needs to occur here; the panic() in alloc_colors() needs
>> to go away.
>>
>>> @@ -434,7 +436,15 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>>               rover = dom;
>>>           }
>>>
>>> -        d = domain_create(dom, &op->u.createdomain, false);
>>> +        if ( llc_coloring_enabled )
>>> +        {
>>> +            llc_colors = llc_colors_from_guest(&op->u.createdomain);
>>> +            num_llc_colors = op->u.createdomain.num_llc_colors;
>>
>> I think you would better avoid setting num_llc_colors to non-zero if
>> you got back NULL from the function. It's at best confusing.
>>
>>> @@ -92,6 +92,10 @@ struct xen_domctl_createdomain {
>>>       /* CPU pool to use; specify 0 or a specific existing pool */
>>>       uint32_t cpupool_id;
>>>
>>> +    /* IN LLC coloring parameters */
>>> +    uint32_t num_llc_colors;
>>> +    XEN_GUEST_HANDLE(uint32) llc_colors;
>>
>> Despite your earlier replies I continue to be unconvinced that this
>> is information which needs to be available right at domain_create.
>> Without that you'd also get away without the sufficiently odd
>> domain_create_llc_colored(). (Odd because: Think of two or three
>> more extended features appearing, all of which want a special cased
>> domain_create().)
> 
> Yes, I definitely see your point. Still there is the p2m table allocation
> problem that you and Julien have discussed previously. I'm not sure I
> understood what the approach is.

Henry has sent a series [1] to remove the requirement to allocate the 
P2M in domain_create().

With that series applied, there requirements to pass the colors at 
domain creation should be lifted.

Cheers,

[1] 
https://lore.kernel.org/xen-devel/20230116015820.1269387-1-Henry.Wang@arm.com/

> 
>> Jan
Carlo Nonato Jan. 26, 2023, 11:18 a.m. UTC | #5 
Hi Jan,

On Thu, Jan 26, 2023 at 8:25 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 24.01.2023 17:29, Jan Beulich wrote:
> > On 23.01.2023 16:47, Carlo Nonato wrote:
> >> @@ -92,6 +92,10 @@ struct xen_domctl_createdomain {
> >>      /* CPU pool to use; specify 0 or a specific existing pool */
> >>      uint32_t cpupool_id;
> >>
> >> +    /* IN LLC coloring parameters */
> >> +    uint32_t num_llc_colors;
> >> +    XEN_GUEST_HANDLE(uint32) llc_colors;
> >
> > Despite your earlier replies I continue to be unconvinced that this
> > is information which needs to be available right at domain_create.
> > Without that you'd also get away without the sufficiently odd
> > domain_create_llc_colored(). (Odd because: Think of two or three
> > more extended features appearing, all of which want a special cased
> > domain_create().)
>
> And perhaps the real question is: Why do the two items need passing
> to a special variant of domain_create() in the first place? The
> necessary information already is passed to the normal function via
> struct xen_domctl_createdomain. All it would take is to read the
> array from guest space later, when struct domain was already
> allocated and is hence available for storing the pointer. (Passing
> the count separately is redundant in any event.)

This was our first approach. However, struct xen_domctl_createdomain is used
both by domctl (pointing to guest memory) and by Xen itself (using Xen memory)
and Julien wasn't happy with this approach because it required some
kind of hack.

See this message from him:

https://marc.info/?l=xen-devel&m=166637496520053

and my answer:

https://marc.info/?l=xen-devel&m=166782830201561

> Jan
>
Carlo Nonato Jan. 26, 2023, 11:19 a.m. UTC | #6 
Hi Julien and Jan,

On Thu, Jan 26, 2023 at 11:21 AM Julien Grall <julien@xen.org> wrote:
>
> Hi,
>
> On 25/01/2023 16:27, Carlo Nonato wrote:
> > On Tue, Jan 24, 2023 at 5:29 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 23.01.2023 16:47, Carlo Nonato wrote:
> >>> @@ -275,6 +276,19 @@ unsigned int *dom0_llc_colors(unsigned int *num_colors)
> >>>       return colors;
> >>>   }
> >>>
> >>> +unsigned int *llc_colors_from_guest(struct xen_domctl_createdomain *config)
> >>
> >> const struct ...?
> >>
> >>> +{
> >>> +    unsigned int *colors;
> >>> +
> >>> +    if ( !config->num_llc_colors )
> >>> +        return NULL;
> >>> +
> >>> +    colors = alloc_colors(config->num_llc_colors);
> >>
> >> Error handling needs to occur here; the panic() in alloc_colors() needs
> >> to go away.
> >>
> >>> @@ -434,7 +436,15 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> >>>               rover = dom;
> >>>           }
> >>>
> >>> -        d = domain_create(dom, &op->u.createdomain, false);
> >>> +        if ( llc_coloring_enabled )
> >>> +        {
> >>> +            llc_colors = llc_colors_from_guest(&op->u.createdomain);
> >>> +            num_llc_colors = op->u.createdomain.num_llc_colors;
> >>
> >> I think you would better avoid setting num_llc_colors to non-zero if
> >> you got back NULL from the function. It's at best confusing.
> >>
> >>> @@ -92,6 +92,10 @@ struct xen_domctl_createdomain {
> >>>       /* CPU pool to use; specify 0 or a specific existing pool */
> >>>       uint32_t cpupool_id;
> >>>
> >>> +    /* IN LLC coloring parameters */
> >>> +    uint32_t num_llc_colors;
> >>> +    XEN_GUEST_HANDLE(uint32) llc_colors;
> >>
> >> Despite your earlier replies I continue to be unconvinced that this
> >> is information which needs to be available right at domain_create.
> >> Without that you'd also get away without the sufficiently odd
> >> domain_create_llc_colored(). (Odd because: Think of two or three
> >> more extended features appearing, all of which want a special cased
> >> domain_create().)
> >
> > Yes, I definitely see your point. Still there is the p2m table allocation
> > problem that you and Julien have discussed previously. I'm not sure I
> > understood what the approach is.
>
> Henry has sent a series [1] to remove the requirement to allocate the
> P2M in domain_create().
>
> With that series applied, there requirements to pass the colors at
> domain creation should be lifted.
>
> Cheers,
>
> [1]
> https://lore.kernel.org/xen-devel/20230116015820.1269387-1-Henry.Wang@arm.com/

Really nice. Thanks to both.

> >
> >> Jan
>
> --
> Julien Grall
diff mbox series

Patch

diff --git a/xen/arch/arm/llc_coloring.c b/xen/arch/arm/llc_coloring.c
index 51f057d7c9..2d0457cdbc 100644
--- a/xen/arch/arm/llc_coloring.c
+++ b/xen/arch/arm/llc_coloring.c
@@ -10,6 +10,7 @@ 
  */
 #include <xen/bitops.h>
 #include <xen/errno.h>
+#include <xen/guest_access.h>
 #include <xen/keyhandler.h>
 #include <xen/llc_coloring.h>
 #include <xen/param.h>
@@ -275,6 +276,19 @@  unsigned int *dom0_llc_colors(unsigned int *num_colors)
     return colors;
 }
 
+unsigned int *llc_colors_from_guest(struct xen_domctl_createdomain *config)
+{
+    unsigned int *colors;
+
+    if ( !config->num_llc_colors )
+        return NULL;
+
+    colors = alloc_colors(config->num_llc_colors);
+    copy_from_guest(colors, config->llc_colors, config->num_llc_colors);
+
+    return colors;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index ad71ad8a4c..505626ec46 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>
@@ -409,6 +410,7 @@  long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
     {
         domid_t        dom;
         static domid_t rover = 0;
+        unsigned int *llc_colors = NULL, num_llc_colors = 0;
 
         dom = op->domain;
         if ( (dom > 0) && (dom < DOMID_FIRST_RESERVED) )
@@ -434,7 +436,15 @@  long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
             rover = dom;
         }
 
-        d = domain_create(dom, &op->u.createdomain, false);
+        if ( llc_coloring_enabled )
+        {
+            llc_colors = llc_colors_from_guest(&op->u.createdomain);
+            num_llc_colors = op->u.createdomain.num_llc_colors;
+        }
+
+        d = domain_create_llc_colored(dom, &op->u.createdomain, false,
+                                      llc_colors, num_llc_colors);
+
         if ( IS_ERR(d) )
         {
             ret = PTR_ERR(d);
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 51be28c3de..49cccc8503 100644
--- 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 0x00000015
+#define XEN_DOMCTL_INTERFACE_VERSION 0x00000016
 
 /*
  * NB. xen_domctl.domain is an IN/OUT parameter for this operation.
@@ -92,6 +92,10 @@  struct xen_domctl_createdomain {
     /* CPU pool to use; specify 0 or a specific existing pool */
     uint32_t cpupool_id;
 
+    /* IN LLC coloring parameters */
+    uint32_t num_llc_colors;
+    XEN_GUEST_HANDLE(uint32) llc_colors;
+
     struct xen_arch_domainconfig arch;
 };
 
diff --git a/xen/include/xen/llc_coloring.h b/xen/include/xen/llc_coloring.h
index 625930d378..2855f38296 100644
--- a/xen/include/xen/llc_coloring.h
+++ b/xen/include/xen/llc_coloring.h
@@ -24,6 +24,8 @@  int domain_llc_coloring_init(struct domain *d, unsigned int *colors,
 void domain_llc_coloring_free(struct domain *d);
 void domain_dump_llc_colors(struct domain *d);
 
+unsigned int *llc_colors_from_guest(struct xen_domctl_createdomain *config);
+
 #else
 
 #define llc_coloring_enabled (false)
@@ -36,6 +38,8 @@  static inline int domain_llc_coloring_init(struct domain *d,
 }
 static inline void domain_llc_coloring_free(struct domain *d) {}
 static inline void domain_dump_llc_colors(struct domain *d) {}
+static inline unsigned int *llc_colors_from_guest(
+    struct xen_domctl_createdomain *config) { return NULL; }
 
 #endif /* CONFIG_HAS_LLC_COLORING */