[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v6 05/15] xen: extend domctl interface for cache coloring
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |