Re: [PATCH v3 2/9] xen/arm: add cache coloring initialization for domains

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

On Mon, Nov 21, 2022 at 4:14 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 21.11.2022 15:50, Carlo Nonato wrote:
> > Hi x86 devs,
>
> Any reason you didn't include Roger?

Nope. Sorry, forgot to add him.

> > I want to ask you some questions about this patch because in the previous
> > version me and Julien have discussed how cache colors should be passed in
> > domain creation. You should be able to read that discussion, anyway here is
> > a link to it
> >
> > https://marc.info/?l=xen-devel&m=166151802002263
>
> I've looked at the first few entries, without finding an answer to ...
>
> > In short, using struct xen_arch_domainconfig works fine only when domctl
> > hypercall is issued. That struct contains a XEN_GUEST_HANDLE so it
> > should point to guest memory and must not be used when creating a domain
> > from Xen itself (i.e. dom0 or dom0less domains). The easy way to go is then
> > changing the domain_create() signature to require also a color array and its
> > length to be passed in for these latter cases.
> > Are you ok with that? See below for more comments.
>
> ... my immediate question: Does supplying the colors necessarily need to
> done right at domain creation? Wouldn't it suffice to be done before first
> allocating memory to the new domain, i.e. via a separate domctl (and then
> for Xen-created domains via a separate Xen-internal function, which the
> new domctl handling would also call)? Or do colors also affect the
> allocation of struct domain itself (and pointers hanging off of it)?

This would be really good. The only problem I can see is the p2m allocation
which is done during domain creation. With the current set of patches it
results in a "Failed to allocate P2M pages" since we want to have p2m tables
allocated with the same color of the domain and a null page is returned
because we have no colors.

> > Another question is then if xen_arch_domainconfig is the right place where 
> > to
> > put the coloring fields for domctl hypercall value passing.
> > See below for more comments.
>
> I think I said so before in other contexts: To me this coloring thing
> isn't Arm-specific, and hence - despite only being implemented for Arm
> right now - would preferably be generic at the interface level.

Ok, I'll try to do that.

> >> @@ -232,6 +233,62 @@ bool __init coloring_init(void)
> >>      return true;
> >>  }
> >>
> >> +int domain_coloring_init(struct domain *d,
> >> +                         const struct xen_arch_domainconfig *config)
> >> +{
> >> +    if ( is_domain_direct_mapped(d) )
> >> +    {
> >> +        printk(XENLOG_ERR
> >> +               "Can't enable coloring and directmap at the same time for 
> >> %pd\n",
> >> +               d);
> >> +        return -EINVAL;
> >> +    }
> >> +
> >> +    if ( is_hardware_domain(d) )
> >> +    {
> >> +        d->arch.colors = dom0_colors;
> >> +        d->arch.num_colors = dom0_num_colors;
> >> +    }
> >> +    else if ( config->num_colors == 0 )
> >> +    {
> >> +        printk(XENLOG_WARNING
> >> +               "Color config not found for %pd. Using default\n", d);
> >> +        d->arch.colors = xzalloc_array(unsigned int, max_colors);
> >> +        d->arch.num_colors = set_default_domain_colors(d->arch.colors);
> >> +    }
> >> +    else
> >> +    {
> >> +        d->arch.colors = xzalloc_array(unsigned int, config->num_colors);
> >> +        d->arch.num_colors = config->num_colors;
> >> +        if ( config->from_guest )
> >> +            copy_from_guest(d->arch.colors, config->colors, 
> >> config->num_colors);
> >> +        else
> >> +            memcpy(d->arch.colors, config->colors.p,
> >> +                   sizeof(unsigned int) * config->num_colors);
> >> +    }
> >
> > Question 1:
> > Here is the current hacky solution in action: using config->from_guest to
> > decide whether to call copy_from_guest() or memcpy(). This is a no go for
> > Julien (and also for me right now). In my current work, I tried to get rid
> > of this field simply by calling copy_from_guest() only in domctl.c, but this
> > solution still isn't easy to maintain because the config->colors.p field can
> > either be a guest pointer or a Xen one and mixing the two semantics can be
> > problematic.
>
> You simply cannot expect copy_from_guest() to work when the source pointer
> is not a guest one.
>
> >> --- a/xen/include/public/arch-arm.h
> >> +++ b/xen/include/public/arch-arm.h
> >> @@ -314,6 +314,8 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
> >>  #define XEN_DOMCTL_CONFIG_TEE_NONE      0
> >>  #define XEN_DOMCTL_CONFIG_TEE_OPTEE     1
> >>
> >> +__DEFINE_XEN_GUEST_HANDLE(color_t, unsigned int);
> >
> > Question 2:
> > This color_t definition is employed because the guest handle for
> > "unsigned int" (uint) is defined later (public/xen.h) and (citing Julien):
> >
> >> Hmmm... And I guess we can't define "unsigned int" earlier because they
> >> rely on macro defined in arch-arm.h?
> >
> > So the solution could be to move everything up a level in
> > xen_domctl_createdomain, where using uint wouldn't be a problem.
> > If this goes to common code then should it be guarded with some #ifdef
> > or not?
>
> As per above I'd say it shouldn't. But then you also shouldn't use
> "unsigned int" in any new additions to the public interface. Only
> fixed width types are suitable to use here.

Got it.

> Jan

Thanks.
- Carlo Nonato