[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Wed, 5 May 2021 12:04:27 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=8GfCaZebeokF8HQp/UuL/ytllX9RZLsOLOzN+/++ny8=; b=Cx5KlGXDVUxNOKJ/mA+0RrkJ9oxnmH3cOySyAzScqmS0yKsNgmrSV1BFYxh08rS3QjQZY08yaBoD8i1nCYauIHqBXHVhgqtClHSvkqBO0e/UeTcUDTxFLX5dbP5Q6QBwHwxpLHIqVRUmIIBQOaGpx8aGYeJkHZ3bhG3C93NgnP+8tBT066iXcxxz5PrURL91kgSzNcd6+R9lavo1Qn+96ByUoXu7HcA/6gT/K7z3sqtY+HMmI4IRXn3xU6pwmkLwcv5mMvCaK/zozIJJMtmGDSfVEA69UEid46hpUlBg3zRa8EGJ6EtTw7xDqrBFlomIj73WQpVdbkvRRWy8SDcr0w==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=CRbZU3493m6g4w8SVJ52ZnZpjCGivz0yTy0Gewzc0HAaHR4hi8z0NimI81OP6TSEv7pafVIUyQGRGQYLHhkxnaabjxBmVolWMvibJPt11xDADmRW52CzxAz9qZkR1f2YCVSvoj1IjHcMjMDEPiCB3skCxnD2Q7KJaoizVd0BorHtRv/AkN52qgnM46n0Z1KH0iq6z0qVJ+PxWYoTtBKg7dE1YHj7DwtxvlMk5i9axH+2TkzHdwOEXJSUbEtzLdqrNgmqDAK5bUfhgPeg6GRn747yv5/vE6jBy5cqGM8C4Wh8v+DQrXT1qhzsHLnqT8qgmTaZEtekG8wQKXRZoS3oig==
  • Authentication-results: esa6.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Jan Beulich <JBeulich@xxxxxxxx>, Wei Liu <wl@xxxxxxx>
  • Delivery-date: Wed, 05 May 2021 10:04:43 +0000
  • Ironport-hdrordr: A9a23:i621cK4EdA/bQNVdZgPXwdKCI+orLtY04lQ7vn1ZYRZeftWE0+ Wnm/oG3RH54QxhP00Is9aGJaWGXDf4/Zl6/YEeMd6ZLW/bkUGvK5xv6pan/i34F0TFh5tg/I pDU4w7Mt3/ClBmkd33iTPTL/8MyMSKmZrY4Nv24GxqSWhRBZ1IyydcJkKlHlZtRA9AbKBJYK a0wsZcvTKvdTA2Q62Adx04dtPOrdHKi57qCCRub3VKhzWmty+i67LxDnGjvis2bjJVzb8utU jDngDpj5/T0c2T9x7G22ffq6lRgdvqo+EzZ/Ckt859EFTRozftQL4kerWZ+Bgpvemk6T8R4a DxiiZlG/421lT8USWepwD31wzpzTA0gkWSsWOwsD/dodfkXnYBAcJHgo5VGyG112Mw+NVnlK 5b1WOQsJRaSRnJmSj76tDSEwtnjUq5uz4jlvQPh3tUXc8fZdZq3Pci1VIQFI0fWCH37I1iF+ VxFsTR+etbajqhHgfkl3gqxsetUHQ1FgqHRUZHutX96UktoFlpi1YdgMQE2m4a8p8gQYQB+O jeW54Y6Y1mX4sKaeZ0HqMbTdGqD3Gle2OxDEuCZUniUKkcf27Wp4Wy6Kwt/+e0dJFN1pc0lZ jbOWkoy1IaagbyDYmHxtlW6BzXBG65Wz7uxswb/ZR/t7HmLYCbQBGrWRQyl4+7pOgERtfeRu /bAuMlP9bzaXbrEZxEmwn3RpNSJXQEUMB9gKdFZ3ue5t/OIpfn8vfWaurXOdPWYEYZc3K6H3 8KRjS2O8la9ECsXRbD8WvsZ08=
  • Ironport-sdr: XvV3tTP5rZgjrQgr1z12BlG6GuSl4GTb3XU4D3iD4KqSSUz7UACumwnxRUzn7hGlKk38dEFW/P Kz/9j7+k0sAKVX6OelCmamMiC0yjbZuq8Wr7sKB8FwTWPLTde2ENoxyy8/Tu+ESCOrLfDk38hp 5vhDKYjTJcmnUxpVaHcBf2fzFSRy5tMVXVjE0Sx/jHlbjKDv+6Cw6bTfRXKiWVC280hOLQszJE NoOlQkkPry3IveUXwRjMk/chYPz50r0RDtL0qsKdgfQxubYthU0rVv+pglNoksiYxcMbe1VLrn exA=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Tue, May 04, 2021 at 10:31:20PM +0100, Andrew Cooper wrote:
> Just as with x86_cpu_policies_are_compatible(), make a start on this function
> with some token handling.
> 
> Add levelling support for MSR_ARCH_CAPS, because RSBA has interesting
> properties, and introduce test_calculate_compatible_success() to the unit
> tests, covering various cases where the arch_caps CPUID bit falls out, and
> with RSBA accumulating rather than intersecting across the two.
> 
> Extend x86_cpu_policies_are_compatible() with a check for MSR_ARCH_CAPS, which
> was arguably missing from c/s e32605b07ef "x86: Begin to introduce support for
> MSR_ARCH_CAPS".
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> ---
>  tools/include/xen-tools/libs.h           |   5 ++
>  tools/tests/cpu-policy/test-cpu-policy.c | 150 
> +++++++++++++++++++++++++++++++
>  xen/include/xen/lib/x86/cpu-policy.h     |  22 +++++
>  xen/lib/x86/policy.c                     |  47 ++++++++++
>  4 files changed, 224 insertions(+)
> 
> diff --git a/tools/include/xen-tools/libs.h b/tools/include/xen-tools/libs.h
> index a16e0c3807..4de10efdea 100644
> --- a/tools/include/xen-tools/libs.h
> +++ b/tools/include/xen-tools/libs.h
> @@ -63,4 +63,9 @@
>  #define ROUNDUP(_x,_w) (((unsigned long)(_x)+(1UL<<(_w))-1) & 
> ~((1UL<<(_w))-1))
>  #endif
>  
> +#ifndef _AC
> +#define __AC(X, Y)   (X ## Y)
> +#define _AC(X, Y)    __AC(X, Y)
> +#endif

You need to remove those definitions from libxl_internal.h.

>  #endif       /* __XEN_TOOLS_LIBS__ */
> diff --git a/tools/tests/cpu-policy/test-cpu-policy.c 
> b/tools/tests/cpu-policy/test-cpu-policy.c
> index 75973298df..455b4fe3c0 100644
> --- 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,
> +            },
> +            .out = {
> +                .p.basic.max_leaf = 6,
> +            },
> +        },
> +        {
> +            "arch_caps, b feat missing",
> +            .a = {
> +                .p.basic.max_leaf = 7,
> +                .p.feat.arch_caps = true,
> +                .m.arch_caps.rdcl_no = true,
> +            },
> +            .b = {
> +                .p.basic.max_leaf = 7,
> +                .m.arch_caps.rdcl_no = true,
> +            },
> +            .out = {
> +                .p.basic.max_leaf = 7,
> +            },
> +        },
> +        {
> +            "arch_caps, b rdcl_no missing",
> +            .a = {
> +                .p.basic.max_leaf = 7,
> +                .p.feat.arch_caps = true,
> +                .m.arch_caps.rdcl_no = true,
> +            },
> +            .b = {
> +                .p.basic.max_leaf = 7,
> +                .p.feat.arch_caps = true,
> +            },
> +            .out = {
> +                .p.basic.max_leaf = 7,
> +                .p.feat.arch_caps = true,
> +            },
> +        },
> +        {
> +            "arch_caps, rdcl_no ok",
> +            .a = {
> +                .p.basic.max_leaf = 7,
> +                .p.feat.arch_caps = true,
> +                .m.arch_caps.rdcl_no = true,
> +            },
> +            .b = {
> +                .p.basic.max_leaf = 7,
> +                .p.feat.arch_caps = true,
> +                .m.arch_caps.rdcl_no = true,
> +            },
> +            .out = {
> +                .p.basic.max_leaf = 7,
> +                .p.feat.arch_caps = true,
> +                .m.arch_caps.rdcl_no = true,
> +            },
> +        },
> +        {
> +            "arch_caps, rsba accum",
> +            .a = {
> +                .p.basic.max_leaf = 7,
> +                .p.feat.arch_caps = true,
> +                .m.arch_caps.rsba = true,
> +            },
> +            .b = {
> +                .p.basic.max_leaf = 7,
> +                .p.feat.arch_caps = true,
> +            },
> +            .out = {
> +                .p.basic.max_leaf = 7,
> +                .p.feat.arch_caps = true,
> +                .m.arch_caps.rsba = true,
> +            },
> +        },
> +    };
> +    struct cpu_policy_errors no_errors = INIT_CPU_POLICY_ERRORS;
> +
> +    printf("Testing calculate compatibility success:\n");
> +
> +    for ( size_t i = 0; i < ARRAY_SIZE(tests); ++i )
> +    {
> +        struct test *t = &tests[i];
> +        struct cpuid_policy *p = malloc(sizeof(struct cpuid_policy));
> +        struct msr_policy *m = malloc(sizeof(struct msr_policy));
> +        struct cpu_policy a = {
> +            &t->a.p,
> +            &t->a.m,
> +        }, b = {
> +            &t->b.p,
> +            &t->b.m,
> +        }, out = {
> +            p,
> +            m,
> +        };
> +        struct cpu_policy_errors e;
> +        int res;
> +
> +        if ( !p || !m )
> +            err(1, "%s() malloc failure", __func__);
> +
> +        res = x86_cpu_policy_calculate_compatible(&a, &b, &out, &e);
> +
> +        /* Check the expected error output. */
> +        if ( res != 0 || memcmp(&no_errors, &e, sizeof(no_errors)) )
> +        {
> +            fail("  Test '%s' expected no errors\n"
> +                 "    got res %d { leaf %08x, subleaf %08x, msr %08x }\n",
> +                 t->name, res, e.leaf, e.subleaf, e.msr);
> +            goto test_done;
> +        }
> +
> +        if ( memcmp(&t->out.p, p, sizeof(*p)) )
> +        {
> +            fail("  Test '%s' resulting CPUID policy not as expected\n",
> +                 t->name);
> +            goto test_done;
> +        }
> +
> +        if ( memcmp(&t->out.m, m, sizeof(*m)) )
> +        {
> +            fail("  Test '%s' resulting MSR policy not as expected\n",
> +                 t->name);
> +            goto test_done;
> +        }

In order to assert that we don't mess things up, I would also add a
x86_cpu_policies_are_compatible check here:

res = x86_cpu_policies_are_compatible(&a, &out, &e);
if ( res )
    fail("  Test '%s' created incompatible policy\n"
         "    got res %d { leaf %08x, subleaf %08x, msr %08x }\n",
         t->name, res, e.leaf, e.subleaf, e.msr);
res = x86_cpu_policies_are_compatible(&b, &out, &e);
if ( res )
    fail("  Test '%s' created incompatible policy\n"
         "    got res %d { leaf %08x, subleaf %08x, msr %08x }\n",
         t->name, res, e.leaf, e.subleaf, e.msr);

> +
> +    test_done:
> +        free(p);
> +        free(m);
> +    }
> +}
> +
>  int main(int argc, char **argv)
>  {
>      printf("CPU Policy unit tests\n");
> @@ -793,6 +941,8 @@ int main(int argc, char **argv)
>      test_is_compatible_success();
>      test_is_compatible_failure();
>  
> +    test_calculate_compatible_success();
> +
>      if ( nr_failures )
>          printf("Done: %u failures\n", nr_failures);
>      else
> diff --git a/xen/include/xen/lib/x86/cpu-policy.h 
> b/xen/include/xen/lib/x86/cpu-policy.h
> index 5a2c4c7b2d..0422a15557 100644
> --- a/xen/include/xen/lib/x86/cpu-policy.h
> +++ b/xen/include/xen/lib/x86/cpu-policy.h
> @@ -37,6 +37,28 @@ int x86_cpu_policies_are_compatible(const struct 
> cpu_policy *host,
>                                      const struct cpu_policy *guest,
>                                      struct cpu_policy_errors *err);
>  
> +/*
> + * Given two policies, calculate one which is compatible with each.
> + *
> + * i.e. Given host @a and host @b, calculate what to give a VM so it can live
> + * migrate between the two.
> + *
> + * @param a        A cpu_policy.
> + * @param b        Another cpu_policy.
> + * @param out      A policy compatible with @a and @b.
> + * @param err      Optional hint for error diagnostics.
> + * @returns -errno
> + *
> + * For typical usage, @a and @b should be system policies of the same type
> + * (i.e. PV/HVM default/max) from different hosts.  In the case that an
> + * incompatibility is detected, the optional err pointer may identify the
> + * problematic leaf/subleaf and/or MSR.
> + */
> +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);
> +
>  #endif /* !XEN_LIB_X86_POLICIES_H */
>  
>  /*
> diff --git a/xen/lib/x86/policy.c b/xen/lib/x86/policy.c
> index f6cea4e2f9..06039e8aa8 100644
> --- 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);

It might be nice to expand test_is_compatible_{success,failure} to
account for arch_caps being checked now.

Shouldn't this check also take into account that host might not have
RSBA set, but it's legit for a guest policy to have it?

if ( ~host->msr->arch_caps.raw & guest->msr->arch_caps.raw & ~POL_MASK )
    FAIL_MSR(MSR_ARCH_CAPABILITIES);

Maybe POL_MASK should be renamed and defined in a header so it's
widely available?

> +
>  #undef FAIL_MSR
>  #undef FAIL_CPUID
>  #undef NA
> @@ -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)

I think this should be in an #ifndef __XEN__ protected region?

There's no need to expose this to the hypervisor, as I would expect it
will never have to do compatible policy generation? (ie: it will
always be done by the toolstack?)

> +{
> +    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;
> +
> +    memset(cp, 0, sizeof(*cp));
> +    memset(mp, 0, sizeof(*mp));
> +
> +    cp->basic.max_leaf = min(ap->basic.max_leaf, bp->basic.max_leaf);
> +
> +    if ( cp->basic.max_leaf >= 7 )
> +    {
> +        cp->feat.max_subleaf = min(ap->feat.max_subleaf, 
> bp->feat.max_subleaf);
> +
> +        cp->feat.raw[0].b = ap->feat.raw[0].b & bp->feat.raw[0].b;
> +        cp->feat.raw[0].c = ap->feat.raw[0].c & bp->feat.raw[0].c;
> +        cp->feat.raw[0].d = ap->feat.raw[0].d & bp->feat.raw[0].d;
> +    }
> +
> +    /* TODO: Far more. */

Right, my proposed patch (07/13) went a bit further and also leveled
1c, 1d, Da1, e1c, e1d, e7d, e8b and e21a, and we also need to level
a couple of max_leaf fields.

I'm happy for this to go in first, and I can rebase the extra logic I
have on top of this one.

Thanks, Roger.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.