Re: [PATCH v5 04/13] xen: extend domctl interface for cache coloring

[Julien Grall <julien@xxxxxxx>]

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.
> +        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