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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

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

At the moment, our ARCH_CAPS in the policy is special for dom0 alone to
see, which is why RSBA isn't unilaterally set, but it will just as soon
as the toolstack logic for handling MSRs properly is in place.

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

~Andrew