Re: [PATCH] libx86: Introduce x86_cpu_policy_calculate_compatible() with MSR_ARCH_CAPS handling

[Jan Beulich <jbeulich@suse.com>]

On 05.05.2021 14:15, Andrew Cooper wrote:
> On 05/05/2021 07:39, Jan Beulich wrote:
>> On 04.05.2021 23:31, Andrew Cooper wrote:
>>> --- a/tools/tests/cpu-policy/test-cpu-policy.c
>>> +++ b/tools/tests/cpu-policy/test-cpu-policy.c
>>> @@ -775,6 +775,154 @@ static void test_is_compatible_failure(void)
>>>      }
>>>  }
>>>  
>>> +static void test_calculate_compatible_success(void)
>>> +{
>>> +    static struct test {
>>> +        const char *name;
>>> +        struct {
>>> +            struct cpuid_policy p;
>>> +            struct msr_policy m;
>>> +        } a, b, out;
>>> +    } tests[] = {
>>> +        {
>>> +            "arch_caps, b short max_leaf",
>>> +            .a = {
>>> +                .p.basic.max_leaf = 7,
>>> +                .p.feat.arch_caps = true,
>>> +                .m.arch_caps.rdcl_no = true,
>>> +            },
>>> +            .b = {
>>> +                .p.basic.max_leaf = 6,
>>> +                .p.feat.arch_caps = true,
>>> +                .m.arch_caps.rdcl_no = true,
>> Is this legitimate input in the first place?
> 
> I've been debating this for a long time, and have decided "yes".
> 
> We have the xend format, and libxl format, and
> 
> cpuid=["host:max_leaf=6"]
> 
> ought not to be rejected simply because it hasn't also gone and zeroed
> every higher piece of information as well.
> 
> In production, this function will be used once per host in the pool, and
> once more if any custom configuration is specified.
> 
> Requiring both inputs to be of the normal form isn't necessary for this
> to function correctly (and indeed, would only add extra overhead), but
> the eventual result will be cleaned/shrunk/etc as appropriate.

Makes sense to me.

>>> --- a/xen/lib/x86/policy.c
>>> +++ b/xen/lib/x86/policy.c
>>> @@ -29,6 +29,9 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
>>>      if ( ~host->msr->platform_info.raw & guest->msr->platform_info.raw )
>>>          FAIL_MSR(MSR_INTEL_PLATFORM_INFO);
>>>  
>>> +    if ( ~host->msr->arch_caps.raw & guest->msr->arch_caps.raw )
>>> +        FAIL_MSR(MSR_ARCH_CAPABILITIES);
>> Doesn't this need special treatment of RSBA, just like it needs specially
>> treating below?
> 
> No.  If RSBA is clear in 'host', then Xen doesn't know about it, and it
> cannot be set in the policy, and cannot be offered to guests.

How does this play together with the comment in
x86_cpu_policy_calculate_compatible() saying "accumulate"? If it's
clear in one policy because it has to be clear in the host's, how
can it be valid for it to get set there in the result, just because
it was set in the other input?

>>> @@ -43,6 +46,50 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
>>>      return ret;
>>>  }
>>>  
>>> +int x86_cpu_policy_calculate_compatible(const struct cpu_policy *a,
>>> +                                        const struct cpu_policy *b,
>>> +                                        struct cpu_policy *out,
>>> +                                        struct cpu_policy_errors *err)
>>> +{
>>> +    const struct cpuid_policy *ap = a->cpuid, *bp = b->cpuid;
>>> +    const struct msr_policy *am = a->msr, *bm = b->msr;
>>> +    struct cpuid_policy *cp = out->cpuid;
>>> +    struct msr_policy *mp = out->msr;
>> Hmm, okay - this would not work with my proposal in reply to your
>> other patch. The output would instead need to have pointers
>> allocated here then.
>>
>>> +    memset(cp, 0, sizeof(*cp));
>>> +    memset(mp, 0, sizeof(*mp));
>>> +
>>> +    cp->basic.max_leaf = min(ap->basic.max_leaf, bp->basic.max_leaf);
>> Any reason you don't do the same right away for the max extended
>> leaf?
> 
> I did the minimum for RSBA testing.  The line needs to be drawn somewhere.

I see. I was thinking that it would be nice if the various related
pieces could remain in sync (or at least new code not staying behind
what we already handle elsewhere), i.e. this one doing at least the
equivalent of what x86_cpu_policies_are_compatible() is currently
capable of dealing with. This, again, would make it easier to spot
all the places that need adjusting when something new is to be added.
But I'm not going to insist - this is largely your realm anyway.

Jan