xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH] libx86: Introduce x86_cpu_policy_calculate_compatible() with MSR_ARCH_CAPS handling
Date: Wed, 5 May 2021 13:15:59 +0100	[thread overview]
Message-ID: <4def95ca-7488-09bf-19fa-d1fa0fa55427@citrix.com> (raw)
In-Reply-To: <28384167-fbd0-d3ff-c064-ee88f5891580@suse.com>

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



  reply	other threads:[~2021-05-05 12:16 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-04 21:31 Andrew Cooper
2021-05-05  6:39 ` Jan Beulich
2021-05-05 12:15   ` Andrew Cooper [this message]
2021-05-05 12:32     ` Jan Beulich
2021-05-05 10:04 ` Roger Pau Monné
2021-05-05 12:37   ` Andrew Cooper
2021-05-05 13:02     ` Roger Pau Monné
2021-05-05 14:29       ` Andrew Cooper
2021-05-05 14:48         ` Jan Beulich
2021-05-05 14:50           ` Andrew Cooper
2021-05-05 15:00             ` Jan Beulich
2021-05-05 15:18               ` Andrew Cooper

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4def95ca-7488-09bf-19fa-d1fa0fa55427@citrix.com \
    --to=andrew.cooper3@citrix.com \
    --cc=jbeulich@suse.com \
    --cc=roger.pau@citrix.com \
    --cc=wl@xen.org \
    --cc=xen-devel@lists.xenproject.org \
    --subject='Re: [PATCH] libx86: Introduce x86_cpu_policy_calculate_compatible() with MSR_ARCH_CAPS handling' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).