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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.11.2022 15:50, Carlo Nonato wrote:
> Hi x86 devs,

Any reason you didn't include Roger?

> 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)?

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

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

Jan