Re: [PATCH V7 3/3] x86/xsaves: ebx may return wrong value using CPUID eax=0xdh, ecx =1

[Shuai Ruan <shuai.ruan@linux.intel.com>]

On Tue, Apr 05, 2016 at 02:31:40AM -0600, Jan Beulich wrote:
> >>> On 31.03.16 at 10:57, <shuai.ruan@linux.intel.com> wrote:
> > Refer to SDM 13.4.3 Extended Region of an XSAVE Area. The value return
> 
> No section numbers please - they tend to change.
> 
> > by ecx[1] with cpuid function 0xdh and sub-fucntion i (i>1) indicates
> 
> Either "0xd" or "0dh". And "function".
> 
> > --- a/xen/arch/x86/traps.c
> > +++ b/xen/arch/x86/traps.c
> > @@ -1020,6 +1020,18 @@ void pv_cpuid(struct cpu_user_regs *regs)
> >              a &= (boot_cpu_data.x86_capability[cpufeat_word(X86_FEATURE_XSAVEOPT)] &
> >                    ~cpufeat_mask(X86_FEATURE_XSAVES));
> >              b = c = d = 0;
> > +            if ( cpu_has_xsavec )
> > +            {
> > +                b = XSTATE_AREA_MIN_SIZE;
> 
> Is this really correct namely when curr->arch.xcr0 == 0? If not, the
> if() below should perhaps be combined with the if() above (and then
> the same would apply to hvm_cpuid()).
> 
> > +                if ( curr->arch.xcr0 )
> > +                    for( subleaf = 2; subleaf < 63; subleaf++ )
> > +                        if ( (1ULL << subleaf) & curr->arch.xcr0 )
> 
> The first if() is redundant with this second one. If you really
> mean to avoid the loop, then please also only check bits
> 2..62 in the first if().
> 
Ok for all comments above.

Another question is whether we should add this in pv_cpuid() or not.
(which we have discussed in the previous thread).

Refer to SDM Volume 1 
"13.2 ENUMERATION OF CPU SUPPORT FOR XSAVE INSTRUCTIONS AND XSAVE-
SUPPORTED FEATURES"
— CPUID function 0DH, sub-function 1.
...
"EBX enumerates the size (in bytes) required by the XSAVES instruction
 for an XSAVE area containing all
 the state components corresponding to bits currently set in XCR0 |
 IA32_XSS."

From the descriptions above, EBX only be used when XSAVES is enabled.
So I think we should not deal with pv_cpuid() here. 
Any comments ?

> Jan
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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