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

Re: [PATCH v6 02/12] libx86: introduce helper to fetch cpuid leaf


  • To: Andrew Cooper <amc96@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Tue, 18 Jan 2022 17:17:45 +0100
  • 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-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=dX9q8j8NX9h/9vRwrio+aMOebKffHZxdKayjUcKJNSI=; b=f/dIx96F/rx5oeNsLVZir0GaXlC/D2CU2NYSItNezNTY1yQ2XrqEo/C2h73h4oh+4mNyZpACkWAch0HMlVCff/f88ig5S4sC3rUmIM7mzjQCuIrqFCQDJ0howxu1wmP/RH1RybY+sY6EOvC0B1LKr2rsiz0pl5ermqzPEQBhHAudCuqwe2v2JUjeiTQSw8NFh7i+39k0hNDAfLpI5hh6o4Q5cDEN5jeTSnvlwu0NmAJDX8DVqU7ADrPuH+qxhklQclkhGWaOX33i/RuTQw3+0q9kAX9yr4u3J56RZUlUbPAG5QHmQW1MsdY+tiRtsAi0/YTrJ5tNHSK6dw/fCp3obg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=KpPC93tqZYELmU0wShaQRxVfAOYblOtHYBnIucymcXAMyyOrzmJwALmZ3RwPVyWFQPHVCncaeDrWKz1YkqZXWfGOOC1YZd1fY7SvhFIvA6d5LgW3YmBoe7AwA4qQbuTP2ToxWBsGJCSZp3vAPzW1mj/hLNvdKSBw5yzXGssoPnfEHAe01rX4SrozFTpk0jOCN7sLWlCSZHCdAshHMixV7RD93QOxw0oibg+laatk4VloF+iYnLDFXOuC9BbGzmby3soTqzpPFte3yN+ICTnfBKLfhXprrPYnlTP1Cu66ooxGnZIFVnLdVObyw8jWwBZDY11wJZYieb50iD+3Cyjvpg==
  • Authentication-results: esa1.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Anthony PERARD <anthony.perard@xxxxxxxxxx>
  • Delivery-date: Tue, 18 Jan 2022 16:18:26 +0000
  • Ironport-data: A9a23:hrN+Pq7R8DS3ohNOuwJzuAxRtA3HchMFZxGqfqrLsTDasY5as4F+v jMdUD+DPPmOYWDzf9lwbom08R8B75OAzt9gSFNqr3thHi5G8cbLO4+Ufxz6V8+wwmwvb67FA +E2MISowBUcFyeEzvuV3zyIQUBUjclkfJKlYAL/En03FV8MpBsJ00o5wbZg294w2LBVPivW0 T/Mi5yHULOa82Yc3lI8s8pvfzs24ZweEBtB1rAPTagjUG32zhH5P7pGTU2FFFPqQ5E8IwKPb 72rIIdVXI/u10xF5tuNyt4Xe6CRK1LYFVDmZnF+A8BOjvXez8CbP2lS2Pc0MC9qZzu1c99Zy 4RkrLOPCi0VILDx3+ccTCtBFhh4BPgTkFPHCSDXXc27ykTHdz3nwul0DVFwNoodkgp1KTgQr 7pCcmlLN03dwbLtqF64YrAEasALNs7kMZlZonh95TrYEewnUdbIRKCiCdpwgm1u2pofQK22i 8wxcj5tKzrkQDd2alYeD780ze7zqXPdWmgNwL6SjfVuuDWCpOBr65DvOtfIft2BRe1Og12V4 GnB+gzRHRUyJNGZjz2f/RqEj+rEzH3TQ5gZGvu+++ICqE2ewCkfBQMbUXO/oOKlkQiuVtRHM UsW9yEy668o+ySWosLVBkPi5iTe51hFBoQWQ7ZSBByxJrT84FewH0c7DRR9dsEb88w3Wg4z0 2aPpoa8bdBwi4G9RXWY/7aSiDq9PykJMGMPDRM5oRs5D8rL+99q0E+WJjp3OOvs14CuR2msq 9yfhHFm3+17sCId60msEbkraRqIr4OBcAM67x6/somNvlIgP97Ni2BFBDHmARd8wGSxEwHpU JsswZH2AAUy4XelznPlrAIlRuDB2hp9GGeA6WOD5rF4n9hXx1atfJpL/BZ1L1pzP8APdFfBO RGP418NuM4NYCDwMMebhr5d7exwksAM8vy/B5jpgidmOMAtJGdrAgkzDaJv44wduBd1yvxuU XtqWc2tEWwbGcxaIMmeHI8gPUsQ7nlmnwv7HMmjpzz+iOb2TCPLFd8tbQXfBshkvPLsiFiEq L53aprVoyizpcWjOEE7B6ZJcwBTRZX6bLirw/FqmhmreVs5SDp/Wq6IkdvMueVNxsxoqwsBx VnkMmdww1vjn3zXbwKMb3FocrT0Wphj63k8OEQR0ZyAgBDPzK7/tPl3m0cfFVX/yNFe8A==
  • Ironport-hdrordr: A9a23:JpNWCqpj43AfPy+41UewDWsaV5s6LNV00zEX/kB9WHVpm5Oj5q KTdaUgpHzJYWgqOE3IwerwSZVpQRvnhOZICPoqTMeftWjdySiVxeRZh/qG/9SOIVyCygcw79 YHT0E6MqyPMbEYt7e53ODbKadd/DDvysnB7oqzoBkNPGUaDJ2M9z0JVjpzUHcGOzWubqBJRK Z0k/A33QZIDk5nFfhTaEN1JtTrlpnurtbLcBQGDxko5E2lljWz8oP3FBCew1M3Ty5P6a1Kyx mBryXJooGY992rwB7V0GHeq75MnsH699dFDMuQzuAINzTXjBqybogJYczHgNl1mpDp1L8Zqq iUn/4SBbUq15oXRBDvnfLZ4Xin7N/p0Q659bbXuwq/nSWzfkNINyMIv/MqTvKe0Tt+gPhMlJ tR2WSXrpxWCg6FshjcyrHzJk9Xv3vxmGEll+ELiXxZTM85U59+6aIi3G49KuZQIMrdgLpXQt WGyPust8p+YBeRanjCpGlhwNalQ3Q0GX69Mz0/k93Q3D5MkH9jyUwEgMQZg3cb7Zo4D4JJ/u LeL81T5f9zp+ItHO5A7d06MISK4632MFrxGXPXJU6iGLAMOnrLpZKy6LIp5PuycJhNyJcpgp zOXF5RqGZ3IivVeI2z9YwO9gqITHS2XDzrxM0b759luqfkTL6uNSGYUlghn8apvv1aCMzGXP S4Po5QHpbYXCrTMJcM2xe7V4hZKHEYXsFQstEnW0iWqsaOMYHuvvyzSoefGFMsK0dvZorSOA p0YNHDHrQK0qnwYA6OvPH4YQKlRqRJl6gAZJTnww==
  • Ironport-sdr: TM1IjXZT3Xqq4zutQtxBlQU37CGQrlm5O4bk8Q5uXORVbV9iNmcALS8l0KErUKib80JBIyqjNH TLmeq/ja7E4XT4e8LFXpxjmLHi7IIubeTJ8G8Y2ZdDlCyKe0MvctMdZLF7mXuqJC3BfThzrlzq KEyH7GmrsBNj569tuf+bo35+OTdB3N/jr11PIBlrsZMiWXivQNBfkdbL2qwYvCVMOG6q0036G0 KdXuVljH/DWo3JsTaDxF39oxd0EnGe7Ozz1fP4/GkMLACSDGl1e9Y+6O83llIUKyjviqTSq/HV FDQ=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Tue, Jan 18, 2022 at 12:26:18PM +0000, Andrew Cooper wrote:
> On 17/01/2022 09:48, Roger Pau Monne wrote:
> > Introduce a helper based on the current Xen guest_cpuid code in order
> > to fetch a cpuid leaf from a policy. The newly introduced function in
> > cpuid.c should not be directly called and instead the provided
> > x86_cpuid_get_leaf macro should be used that will properly deal with
> > const and non-const inputs.
> >
> > Also add a test to check that the introduced helper doesn't go over
> > the bounds of the policy.
> >
> > Note the code in x86_cpuid_copy_from_buffer is not switched to use the
> > new function because of the boundary checks against the max fields of
> > the policy, which might not be properly set at the point where
> > x86_cpuid_copy_from_buffer get called, for example when filling an
> > empty policy from scratch.
> 
> Filling an empty policy from scratch will be fine, because we always
> ascend through leaves.  This also matches the chronology of how CPUID
> developed.

I'm slightly confused by this. The main point I wanted to make here is
that x86_cpuid_copy_from_buffer cannot be switched to use
x86_cpuid_get_leaf because the later relies on accessing the different
maximum leaf/subleaf fields in the policy object: basic.max_leaf,
feat.max_subleaf and extd.max_leaf.

That would for example make the existing
test_cpuid_deserialise_failure hit a page fault, since it passes NULL
as a policy object.

I don't really felt like changing test_cpuid_deserialise_failure to
cope with the new behavior of x86_cpuid_copy_from_buffer if it was
switched to use x86_cpuid_get_leaf.

Let me know if that's OK, or if you would like
x86_cpuid_copy_from_buffer switched to use x86_cpuid_get_leaf and
consequently have the callers also adjusted.

> The most likely case to go wrong is enabling an optional feature above
> max_leaf, and getting the bump to max_leaf out of order.  That said, I
> suspect such logic would be working on an object, rather than a list.
> 
> The important point is that x86_cpuid_copy_from_buffer() is deliberately
> invariant to the order of entries for compatibility reasons, even if we
> don't expect it to matter in practice.
> 
> >
> > Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> > ---
> > Changes since v4:
> >  - Rename _x86_cpuid_get_leaf to x86_cpuid_get_leaf_const.
> >
> > Changes since v3:
> >  - New in this version.
> > ---
> > Regarding safety of the usage of array_access_nospec to obtain a
> > pointer to an element of an array, there are already other instances
> > of this usage, for example in viridian_time_wrmsr, so I would assume
> > this is fine.
> 
> It's a bit of a weird construct, and both GCC and Clang could generate
> better code, but it does look to be safe.
> 
> > ---
> >  tools/tests/cpu-policy/test-cpu-policy.c | 75 ++++++++++++++++++++++++
> >  xen/arch/x86/cpuid.c                     | 55 +++--------------
> >  xen/include/xen/lib/x86/cpuid.h          | 19 ++++++
> >  xen/lib/x86/cpuid.c                      | 52 ++++++++++++++++
> >  4 files changed, 153 insertions(+), 48 deletions(-)
> >
> > diff --git a/tools/tests/cpu-policy/test-cpu-policy.c 
> > b/tools/tests/cpu-policy/test-cpu-policy.c
> > index ed450a0997..3f777fc1fc 100644
> > --- a/tools/tests/cpu-policy/test-cpu-policy.c
> > +++ b/tools/tests/cpu-policy/test-cpu-policy.c
> > @@ -570,6 +570,80 @@ static void test_cpuid_out_of_range_clearing(void)
> >      }
> >  }
> >  
> > +static void test_cpuid_get_leaf_failure(void)
> > +{
> > +    static const struct test {
> > +        struct cpuid_policy p;
> > +        const char *name;
> > +        uint32_t leaf, subleaf;
> > +    } tests[] = {
> > +        /* Bound checking logic. */
> > +        {
> > +            .name = "Basic max leaf >= array size",
> > +            .p = {
> > +                .basic.max_leaf = CPUID_GUEST_NR_BASIC,
> > +            },
> > +        },
> > +        {
> > +            .name = "Feature max leaf >= array size",
> > +            .p = {
> > +                .basic.max_leaf = CPUID_GUEST_NR_BASIC - 1,
> > +                .feat.max_subleaf = CPUID_GUEST_NR_FEAT,
> > +            },
> > +            .leaf = 0x00000007,
> > +        },
> > +        {
> > +            .name = "Extended max leaf >= array size",
> > +            .p = {
> > +                .extd.max_leaf = 0x80000000 + CPUID_GUEST_NR_EXTD,
> > +            },
> > +            .leaf = 0x80000000,
> > +        },
> > +
> > +        {
> > +            .name = "Basic leaf >= max leaf",
> > +            .p = {
> > +                .basic.max_leaf = CPUID_GUEST_NR_BASIC - 1,
> > +            },
> > +            .leaf = CPUID_GUEST_NR_BASIC,
> > +        },
> > +        {
> > +            .name = "Feature leaf >= max leaf",
> > +            .p = {
> > +                .basic.max_leaf = CPUID_GUEST_NR_BASIC - 1,
> > +                .feat.max_subleaf = CPUID_GUEST_NR_FEAT - 1,
> > +            },
> > +            .leaf = 0x00000007,
> > +            .subleaf = CPUID_GUEST_NR_FEAT,
> > +        },
> > +        {
> > +            .name = "Extended leaf >= max leaf",
> > +            .p = {
> > +                .extd.max_leaf = 0x80000000 + CPUID_GUEST_NR_EXTD - 1,
> > +            },
> > +            .leaf = 0x80000000 + CPUID_GUEST_NR_EXTD,
> > +        },
> > +    };
> > +    const struct cpuid_policy pc;
> > +    const struct cpuid_leaf *lc;
> > +    struct cpuid_policy p;
> > +    struct cpuid_leaf *l;
> > +
> > +    /* Constness build test. */
> > +    lc = x86_cpuid_get_leaf(&pc, 0, 0);
> > +    l = x86_cpuid_get_leaf(&p, 0, 0);
> > +
> > +    printf("Testing CPUID get leaf bound checking:\n");
> > +
> > +    for ( size_t i = 0; i < ARRAY_SIZE(tests); ++i )
> > +    {
> > +        const struct test *t = &tests[i];
> 
> memdup().  It is important for tests which potentially reach out of
> bounds, so ASAN can work.

Done.

> 
> That said, you're only testing half of the boundary cases.  Perhaps more
> important important is the case where max_leaf is really out of legal
> bounds.

I was attempting to test this in the first two tests, by setting
max_leaf = CPUID_GUEST_NR_BASIC, max_subleaf = CPUID_GUEST_NR_FEAT and
extd.max_leaf also to it's max value (ie: out of legal bounds). I
could set them to ~0 if that's clearer.

> Further, it is also important to check the non-NULL cases too.
> 
> It would probably be better to have a single cpuid_policy object, and a
> list of pointers (perhaps offsets) to interesting max_leaf fields, along
> with their relevant compile time bounds.  That way, you can try all the
> interesting max_leaf values (0, limit-1, limit, ~0) and check
> NULL/non-NULLness of the answer with a simple min() calculation.

Then it would make sense to rename to test_cpuid_get_leaf_boundary.

I'm not sure I get the part of having a single cpuid_policy object, as
then we would have to go changing it's contents in order to do the
different tests rather than just having a const one for each single
test that's already setup as expected. Also I'm confused at the usage
of min(), but that's likely because I don't really get the part of
having a single cpuid_policy object.

I assume you would rather prefer your suggested testing to be
implemented here rather than in a follow up patch?

> > diff --git a/xen/include/xen/lib/x86/cpuid.h 
> > b/xen/include/xen/lib/x86/cpuid.h
> > index a4d254ea96..050cd4f9d1 100644
> > --- a/xen/include/xen/lib/x86/cpuid.h
> > +++ b/xen/include/xen/lib/x86/cpuid.h
> > @@ -431,6 +431,25 @@ int x86_cpuid_copy_from_buffer(struct cpuid_policy 
> > *policy,
> >                                 uint32_t nr_entries, uint32_t *err_leaf,
> >                                 uint32_t *err_subleaf);
> >  
> > +/**
> > + * Get a cpuid leaf from a policy object.
> > + *
> > + * @param policy      The cpuid_policy object.
> > + * @param leaf        The leaf index.
> > + * @param subleaf     The subleaf index.
> > + * @returns a pointer to the requested leaf or NULL in case of error.
> > + *
> > + * The function will perform out of bound checks. Do not call this function
> > + * directly and instead use x86_cpuid_get_leaf that will deal with both 
> > const
> > + * and non-const policies returning a pointer with constness matching that 
> > of
> > + * the input.
> > + */
> > +const struct cpuid_leaf *x86_cpuid_get_leaf_const(const struct 
> > cpuid_policy *p,
> > +                                                  uint32_t leaf,
> > +                                                  uint32_t subleaf);
> 
> Examples like this demonstrate obviously why
> 
> const struct cpuid_leaf *x86_cpuid_get_leaf_const(
>     const struct cpuid_policy *p, uint32_t leaf, uint32_t subleaf);
> 
> is a better layout.

I don't care that much in this case TBH. One way or another this is
the style used currently on the file, so it's likely better to keep it
that way.

Thanks, Roger.



 


Rackspace

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