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

[Julien Grall <julien@xxxxxxx>]

Hi Carlo,

On 08/01/2024 11:19, Carlo Nonato wrote:
> Hi Julien,
>
> On Mon, Jan 8, 2024 at 12:01 PM Julien Grall <julien@xxxxxxx> wrote:
> > Hi Carlo,
> >
> > On 08/01/2024 10:27, Carlo Nonato wrote:
> > > On Fri, Jan 5, 2024 at 6:26 PM Julien Grall <julien@xxxxxxx> wrote:
> > > > 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.
> > Looking at your check what you prevent is a toolstack updating the array
> > twice. But that would be ok (/!\ I am not saying we should allow it) so
> > long no memory has been allocated to the domain.
> >
> > But I also consider we would re-color once we started to allocate memory
> > for the domain (either RAM or P2M). This seems to be missed out in your
> > check.
> So you want to be able to change colors if no memory has yet been allocated?
No. I am saying that that we should not be able to allow changing the 
colors after the memory has been allocated. To give an example, your 
current code would allow:
  1) Setup the P2M pools or allocate RAM
  2) Set the color

This would render the coloring configuration moot.

Whether we want to allow changing the coloring before hand is a 
different question and as I wrote earlier on, I don't mind if you want 
to forbid that.
> I don't know what to check that.
You can check the size of the P2M pool (d->arch.paging.p2m_total_pages) 
is still 0. I think for RAM, you can check d->tot_pages == 0.
Cheers,

--
Julien Grall