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

Re: [PATCH 2/7] x86/xstate: Cross-check dynamic XSTATE sizes at boot


  • To: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Thu, 23 May 2024 17:34:25 +0200
  • Autocrypt: addr=jbeulich@xxxxxxxx; keydata= xsDiBFk3nEQRBADAEaSw6zC/EJkiwGPXbWtPxl2xCdSoeepS07jW8UgcHNurfHvUzogEq5xk hu507c3BarVjyWCJOylMNR98Yd8VqD9UfmX0Hb8/BrA+Hl6/DB/eqGptrf4BSRwcZQM32aZK 7Pj2XbGWIUrZrd70x1eAP9QE3P79Y2oLrsCgbZJfEwCgvz9JjGmQqQkRiTVzlZVCJYcyGGsD /0tbFCzD2h20ahe8rC1gbb3K3qk+LpBtvjBu1RY9drYk0NymiGbJWZgab6t1jM7sk2vuf0Py O9Hf9XBmK0uE9IgMaiCpc32XV9oASz6UJebwkX+zF2jG5I1BfnO9g7KlotcA/v5ClMjgo6Gl MDY4HxoSRu3i1cqqSDtVlt+AOVBJBACrZcnHAUSuCXBPy0jOlBhxPqRWv6ND4c9PH1xjQ3NP nxJuMBS8rnNg22uyfAgmBKNLpLgAGVRMZGaGoJObGf72s6TeIqKJo/LtggAS9qAUiuKVnygo 3wjfkS9A3DRO+SpU7JqWdsveeIQyeyEJ/8PTowmSQLakF+3fote9ybzd880fSmFuIEJldWxp Y2ggPGpiZXVsaWNoQHN1c2UuY29tPsJgBBMRAgAgBQJZN5xEAhsDBgsJCAcDAgQVAggDBBYC AwECHgECF4AACgkQoDSui/t3IH4J+wCfQ5jHdEjCRHj23O/5ttg9r9OIruwAn3103WUITZee e7Sbg12UgcQ5lv7SzsFNBFk3nEQQCACCuTjCjFOUdi5Nm244F+78kLghRcin/awv+IrTcIWF hUpSs1Y91iQQ7KItirz5uwCPlwejSJDQJLIS+QtJHaXDXeV6NI0Uef1hP20+y8qydDiVkv6l IreXjTb7DvksRgJNvCkWtYnlS3mYvQ9NzS9PhyALWbXnH6sIJd2O9lKS1Mrfq+y0IXCP10eS FFGg+Av3IQeFatkJAyju0PPthyTqxSI4lZYuJVPknzgaeuJv/2NccrPvmeDg6Coe7ZIeQ8Yj t0ARxu2xytAkkLCel1Lz1WLmwLstV30g80nkgZf/wr+/BXJW/oIvRlonUkxv+IbBM3dX2OV8 AmRv1ySWPTP7AAMFB/9PQK/VtlNUJvg8GXj9ootzrteGfVZVVT4XBJkfwBcpC/XcPzldjv+3 HYudvpdNK3lLujXeA5fLOH+Z/G9WBc5pFVSMocI71I8bT8lIAzreg0WvkWg5V2WZsUMlnDL9 mpwIGFhlbM3gfDMs7MPMu8YQRFVdUvtSpaAs8OFfGQ0ia3LGZcjA6Ik2+xcqscEJzNH+qh8V m5jjp28yZgaqTaRbg3M/+MTbMpicpZuqF4rnB0AQD12/3BNWDR6bmh+EkYSMcEIpQmBM51qM EKYTQGybRCjpnKHGOxG0rfFY1085mBDZCH5Kx0cl0HVJuQKC+dV2ZY5AqjcKwAxpE75MLFkr wkkEGBECAAkFAlk3nEQCGwwACgkQoDSui/t3IH7nnwCfcJWUDUFKdCsBH/E5d+0ZnMQi+G0A nAuWpQkjM1ASeQwSHEeAWPgskBQL
  • Cc: Roger Pau Monné <roger.pau@xxxxxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Thu, 23 May 2024 15:34:45 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 23.05.2024 13:16, Andrew Cooper wrote:
> Right now, xstate_ctxt_size() performs a cross-check of size with CPUID in for
> every call.  This is expensive, being used for domain create/migrate, as well
> as to service certain guest CPUID instructions.
> 
> Instead, arrange to check the sizes once at boot.  See the code comments for
> details.  Right now, it just checks hardware against the algorithm
> expectations.  Later patches will add further cross-checking.
> 
> Introduce the missing X86_XCR0_* and X86_XSS_* constants, and a couple of
> missing CPUID bits.  This is to maximise coverage in the sanity check, even if
> we don't expect to use/virtualise some of these features any time soon.  Leave
> HDC and HWP alone for now.  We don't have CPUID bits from them stored nicely.

Since you say "the missing", ...

> --- a/xen/arch/x86/include/asm/x86-defns.h
> +++ b/xen/arch/x86/include/asm/x86-defns.h
> @@ -77,7 +77,7 @@
>  #define X86_CR4_PKS        0x01000000 /* Protection Key Supervisor */
>  
>  /*
> - * XSTATE component flags in XCR0
> + * XSTATE component flags in XCR0 | MSR_XSS
>   */
>  #define X86_XCR0_FP_POS           0
>  #define X86_XCR0_FP               (1ULL << X86_XCR0_FP_POS)
> @@ -95,11 +95,34 @@
>  #define X86_XCR0_ZMM              (1ULL << X86_XCR0_ZMM_POS)
>  #define X86_XCR0_HI_ZMM_POS       7
>  #define X86_XCR0_HI_ZMM           (1ULL << X86_XCR0_HI_ZMM_POS)
> +#define X86_XSS_PROC_TRACE        (_AC(1, ULL) <<  8)
>  #define X86_XCR0_PKRU_POS         9
>  #define X86_XCR0_PKRU             (1ULL << X86_XCR0_PKRU_POS)
> +#define X86_XSS_PASID             (_AC(1, ULL) << 10)
> +#define X86_XSS_CET_U             (_AC(1, ULL) << 11)
> +#define X86_XSS_CET_S             (_AC(1, ULL) << 12)
> +#define X86_XSS_HDC               (_AC(1, ULL) << 13)
> +#define X86_XSS_UINTR             (_AC(1, ULL) << 14)
> +#define X86_XSS_LBR               (_AC(1, ULL) << 15)
> +#define X86_XSS_HWP               (_AC(1, ULL) << 16)
> +#define X86_XCR0_TILE_CFG         (_AC(1, ULL) << 17)
> +#define X86_XCR0_TILE_DATA        (_AC(1, ULL) << 18)

... I'm wondering if you deliberately left out APX (bit 19).

Since you're re-doing some of what I have long had in patches already,
I'd also like to ask whether the last underscores each in the two AMX
names really are useful in your opinion. While rebasing isn't going
to be difficult either way, it would be yet simpler with
X86_XCR0_TILECFG and X86_XCR0_TILEDATA, as I've had it in my patches
for over 3 years.

> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -604,9 +604,156 @@ static bool valid_xcr0(uint64_t xcr0)
>      if ( !(xcr0 & X86_XCR0_BNDREGS) != !(xcr0 & X86_XCR0_BNDCSR) )
>          return false;
>  
> +    /* TILE_CFG and TILE_DATA must be the same. */
> +    if ( !(xcr0 & X86_XCR0_TILE_CFG) != !(xcr0 & X86_XCR0_TILE_DATA) )
> +        return false;
> +
>      return true;
>  }
>  
> +struct xcheck_state {
> +    uint64_t states;
> +    uint32_t uncomp_size;
> +    uint32_t comp_size;
> +};
> +
> +static void __init check_new_xstate(struct xcheck_state *s, uint64_t new)
> +{
> +    uint32_t hw_size;
> +
> +    BUILD_BUG_ON(X86_XCR0_STATES & X86_XSS_STATES);
> +
> +    BUG_ON(s->states & new); /* States only increase. */
> +    BUG_ON(!valid_xcr0(s->states | new)); /* Xen thinks it's a good value. */
> +    BUG_ON(new & ~(X86_XCR0_STATES | X86_XSS_STATES)); /* Known state. */
> +    BUG_ON((new & X86_XCR0_STATES) &&
> +           (new & X86_XSS_STATES)); /* User or supervisor, not both. */
> +
> +    s->states |= new;
> +    if ( new & X86_XCR0_STATES )
> +    {
> +        if ( !set_xcr0(s->states & X86_XCR0_STATES) )
> +            BUG();
> +    }
> +    else
> +        set_msr_xss(s->states & X86_XSS_STATES);
> +
> +    /*
> +     * Check the uncompressed size.  Some XSTATEs are out-of-order and fill 
> in
> +     * prior holes in the state area, so we check that the size doesn't
> +     * decrease.
> +     */
> +    hw_size = cpuid_count_ebx(0xd, 0);
> +
> +    if ( hw_size < s->uncomp_size )
> +        panic("XSTATE 0x%016"PRIx64", new bits {%63pbl}, uncompressed hw 
> size %#x < prev size %#x\n",
> +              s->states, &new, hw_size, s->uncomp_size);
> +
> +    s->uncomp_size = hw_size;
> +
> +    /*
> +     * Check the compressed size, if available.  All components strictly
> +     * appear in index order.  In principle there are no holes, but some
> +     * components have their base address 64-byte aligned for efficiency
> +     * reasons (e.g. AMX-TILE) and there are other components small enough to
> +     * fit in the gap (e.g. PKRU) without increasing the overall length.
> +     */
> +    hw_size = cpuid_count_ebx(0xd, 1);
> +
> +    if ( cpu_has_xsavec )
> +    {
> +        if ( hw_size < s->comp_size )
> +            panic("XSTATE 0x%016"PRIx64", new bits {%63pbl}, compressed hw 
> size %#x < prev size %#x\n",
> +                  s->states, &new, hw_size, s->comp_size);
> +
> +        s->comp_size = hw_size;
> +    }
> +    else
> +        BUG_ON(hw_size); /* Compressed size reported, but no XSAVEC ? */

I'm not quite happy with this being fatal to booting. Maybe just WARN_ON()?

Jan



 


Rackspace

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