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
