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 <lucmiccio@gmail.com>
Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech>
---
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(+)
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__ */
/*
--
2.34.1On 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
Hi Jan,
On Thu, Feb 1, 2024 at 2:51 PM Jan Beulich <jbeulich@suse.com> 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
On 06.02.2024 12:46, Carlo Nonato wrote:
> On Thu, Feb 1, 2024 at 2:51 PM Jan Beulich <jbeulich@suse.com> 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
On Tue, Feb 6, 2024 at 12:51 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 06.02.2024 12:46, Carlo Nonato wrote:
> > On Thu, Feb 1, 2024 at 2:51 PM Jan Beulich <jbeulich@suse.com> 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
Hi Jan,
On Thu, Feb 1, 2024 at 2:51 PM Jan Beulich <jbeulich@suse.com> 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
On 03.02.2024 12:41, Carlo Nonato wrote: > On Thu, Feb 1, 2024 at 2:51 PM Jan Beulich <jbeulich@suse.com> 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
© 2016 - 2026 Red Hat, Inc.