Re: [Xen-devel] [PATCH 2/6] x86/cpuid: Introduce recalculate_xstate()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.01.17 at 12:40, <andrew.cooper3@xxxxxxxxxx> wrote:
> All data in the xstate union, other than the Da1 feature word, is derived from
> other state; either feature bits from other words, or layout information which
> has already been collected by Xen's xstate driver.
> 
> Recalculate the xstate information for each policy object when the feature
> bits may have changed.
> 
> The XSTATE_XSAVES_ONLY define needs extending to a 64bit value to avoid
> problems when taking its converse for masking purposes.

I don't understand this part - plain 0 is a signed quantity, so ~0 would
be sign extended to 64 bits as needed.

> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -80,6 +80,103 @@ static void sanitise_featureset(uint32_t *fs)
>                            (fs[FEATURESET_e1d] & ~CPUID_COMMON_1D_FEATURES));
>  }
>  
> +static void recalculate_xstate(struct cpuid_policy *p)
> +{
> +    uint64_t xstates = XSTATE_FP_SSE;
> +    uint32_t xstate_size = XSTATE_AREA_MIN_SIZE;
> +    unsigned int i, Da1 = p->xstate.Da1;
> +
> +    /*
> +     * The Da1 leaf is the only piece if information preserved.  Everything
> +     * else is derived from other feature state.
> +     */

Almost.

> +    memset(&p->xstate, 0, sizeof(p->xstate));
> +
> +    if ( !p->basic.xsave )
> +        return;

You're clobbering it here (but in all reality it should be zero in this
case).

> @@ -154,6 +152,13 @@ struct cpuid_policy
>              };
>              uint32_t /* b */:32, xss_low, xss_high;
>          };
> +
> +        /* Per-component common state.  Valid for i >= 2. */
> +        struct {
> +            uint32_t size, offset;
> +            bool xss:1, align:1;
> +            uint32_t /* c */:30, /* d */:32;
> +        } comp[CPUID_GUEST_NR_XSTATE];

Hmm, can we rely on this functioning on varying complier variants?
I think the standard doesn't exclude a uint32_t type bitfield to
start on a 4-byte boundary if not following another uint32_t one.
IOW I think we'd be better off giving the same type to all fields we
want to share a storage unit.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel