Re: [PATCH 3/6] x86/xsaves: enable xsaves/xrstors for hvm guest

[Chao Peng <chao.p.peng@linux.intel.com>]

On Thu, Jul 02, 2015 at 10:02:28PM +0800, Shuai Ruan wrote:
> This patch enables xsaves for hvm guest, includes:
> 1.handle xsaves vmcs init and vmexit.
> 2.add logic to write/read the XSS msr.
> 
> Signed-off-by: Shuai Ruan <shuai.ruan@intel.com>
> ---
>  xen/arch/x86/hvm/hvm.c             | 40 ++++++++++++++++++++++++++++++++++++++
>  xen/arch/x86/hvm/vmx/vmcs.c        |  7 ++++++-
>  xen/arch/x86/hvm/vmx/vmx.c         | 18 +++++++++++++++++
>  xen/arch/x86/xstate.c              |  4 ++--
>  xen/include/asm-x86/hvm/vmx/vmcs.h |  5 +++++
>  xen/include/asm-x86/hvm/vmx/vmx.h  |  2 ++
>  xen/include/asm-x86/xstate.h       |  1 +
>  7 files changed, 74 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 535d622..2958e0d 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4269,6 +4269,10 @@ void hvm_hypervisor_cpuid_leaf(uint32_t sub_idx,
>      }
>  }
>  
> +#define XSAVEOPT	(1 << 0)
> +#define XSAVEC		(1 << 1)
> +#define XGETBV		(1 << 2)
> +#define XSAVES		(1 << 3)

Hard tab is used.

>  void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx,
>                                     unsigned int *ecx, unsigned int *edx)
>  {
> @@ -4355,6 +4359,34 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx,
>                      *ebx = _eax + _ebx;
>              }
>          }
> +        if ( count == 1 )
> +        {
> +            if ( cpu_has_xsaves )
> +            {
> +                *ebx = XSTATE_AREA_MIN_SIZE;
> +                if ( v->arch.xcr0 | v->arch.msr_ia32_xss )
> +                    for ( sub_leaf = 2; sub_leaf < 63; sub_leaf++ )
> +                    {
> +                        if ( !((v->arch.xcr0 | v->arch.msr_ia32_xss)
> +			   & (1ULL << sub_leaf)) )

alignment.

> +                            continue;
> +                        domain_cpuid(d, input, sub_leaf, &_eax, &_ebx, &_ecx,
> +                                     &_edx);
> +                        *ebx =  *ebx + _eax;
> +                    }
> +            }
> +            else
> +            {
> +                *eax &= ~XSAVES;
> +                if ( !cpu_has_xgetbv1 )
> +                    *eax &= ~XGETBV;
> +                if ( !cpu_has_xsavec )
> +                    *eax &= ~XSAVEC;
> +                if ( !cpu_has_xsaveopt )
> +                    *eax &= ~XSAVEOPT;

I think these features(XGETBV/XSAVEC/XSAVEOPT) should be independent
of XSAVES, e.g. here they should not be enclosed with 'else' statement.

Chao