|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v5 04/13] xen: extend domctl interface for cache coloring
Hi Julien,
On Fri, Jan 5, 2024 at 6:26 PM Julien Grall <julien@xxxxxxx> wrote:
>
> Hi Carlo,
>
> On 02/01/2024 09:51, Carlo Nonato wrote:
> > 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@xxxxxxxxx>
> >
> > Signed-off-by: Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>
> > Signed-off-by: Marco Solieri <marco.solieri@xxxxxxxxxxxxxxx>
> > ---
> > v5:
> > - added a new hypercall to set colors
> > - uint for the guest handle
> > v4:
> > - updated XEN_DOMCTL_INTERFACE_VERSION
> > ---
> > xen/arch/arm/llc-coloring.c | 17 +++++++++++++++++
> > xen/common/domctl.c | 11 +++++++++++
> > xen/include/public/domctl.h | 10 +++++++++-
> > xen/include/xen/llc-coloring.h | 3 +++
> > 4 files changed, 40 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/arm/llc-coloring.c b/xen/arch/arm/llc-coloring.c
> > index 5ce58aba70..a08614ec36 100644
> > --- a/xen/arch/arm/llc-coloring.c
> > +++ b/xen/arch/arm/llc-coloring.c
> > @@ -9,6 +9,7 @@
> > * Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>
> > */
> > #include <xen/errno.h>
> > +#include <xen/guest_access.h>
> > #include <xen/keyhandler.h>
> > #include <xen/llc-coloring.h>
> > #include <xen/param.h>
> > @@ -278,6 +279,22 @@ int 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)
> > +{
> > + if ( d->num_llc_colors )
> > + return -EEXIST;
> > +
> > + if ( domain_alloc_colors(d, config->num_llc_colors) )
>
> domain_alloc_colors() doesn't sanity check config->num_llc_colors before
> allocating the array. You want a check the size before so we would not
> try to allocate an arbitrary amount of memory.
>
> > + return -ENOMEM;
> > +
> > + if ( copy_from_guest(d->llc_colors, config->llc_colors,
> > + config->num_llc_colors) )
> > + return -EFAULT;
> > +
> > + return domain_check_colors(d);
> > +}
> > +
> > /*
> > * Local variables:
> > * mode: C
> > 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");
>
> To me, the message doesn't match the check in
> domain_set_llc_colors_domctl(). But I think you want to check that no
> memory was yet allocated to the domain. Otherwise, you coloring will be
> wrong.
>
> Also, it is a bit unclear why you print a message for -EEXIST but not
> the others. In this instance, I would consider to print nothing at all.
The problem here is that we don't support recoloring. When a domain is
created it receives a coloring configuration and it can't change. If this
hypercall is called twice I have to stop the second time somehow. I'm ok
with printing nothing, but -EEXIST to me seems logical.
> > + break;
> > +
> > default:
> > ret = arch_do_domctl(op, d, u_domctl);
> > break;
> > diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> > index a33f9ec32b..2b12069294 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 0x00000016
> > +#define XEN_DOMCTL_INTERFACE_VERSION 0x00000017
> >
> > /*
> > * NB. xen_domctl.domain is an IN/OUT parameter for this operation.
> > @@ -1190,6 +1190,12 @@ 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 */
> > + unsigned int num_llc_colors;
>
> I think there will be a padding here. So can you make it explicit?
>
> > + XEN_GUEST_HANDLE_64(uint) llc_colors;
> > +};
> > +
> > struct xen_domctl {
> > uint32_t cmd;
> > #define XEN_DOMCTL_createdomain 1
> > @@ -1277,6 +1283,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 +1346,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 cedd97d4b5..fa2edc8ad8 100644
> > --- a/xen/include/xen/llc-coloring.h
> > +++ b/xen/include/xen/llc-coloring.h
> > @@ -33,6 +33,9 @@ extern bool llc_coloring_enabled;
> > void domain_llc_coloring_free(struct domain *d);
> > void domain_dump_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__ */
> >
> > /*
>
> Cheers,
>
> --
> Julien Grall
Thanks.
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |