Re: [Xen-devel] [PATCH V4 3/4] x86/xsaves: enable xsaves/xrstors for hvm guest

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.08.15 at 12:54, <shuai.ruan@xxxxxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4540,6 +4540,33 @@ 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)) )
> +                            continue;
> +                        _eax = xstate_sizes[sub_leaf];
> +                        *ebx =  *ebx + _eax;

Why not just "*ebx += xstate_sizes[sub_leaf]"?

> +                    }
> +            }
> +            else
> +            {
> +                *eax &= ~XSAVES;
> +                *ebx = *ecx = *edx = 0;
> +            }
> +            if ( !cpu_has_xgetbv1 )
> +                *eax &= ~XGETBV1;
> +            if ( !cpu_has_xsavec )
> +                *eax &= ~XSAVEC;
> +            if ( !cpu_has_xsaveopt )
> +                *eax &= ~XSAVEOPT;

Are all these really necessary? And is they are, why isn't the
XSAVEOPT one needed already before this series?

> @@ -1238,7 +1239,8 @@ static int construct_vmcs(struct vcpu *v)
>          __vmwrite(HOST_PAT, host_pat);
>          __vmwrite(GUEST_PAT, guest_pat);
>      }
> -
> +    if ( cpu_has_vmx_xsaves )
> +        __vmwrite(XSS_EXIT_BITMAP, VMX_XSS_EXIT_BITMAP);

This reads as if you were writing an architecture mandated value
into an architecture defined VMCS slot. I.e. the second constant
would better be renamed imo.

> @@ -365,6 +370,8 @@ enum vmcs_field {
>      VMREAD_BITMAP                   = 0x00002026,
>      VMWRITE_BITMAP                  = 0x00002028,
>      VIRT_EXCEPTION_INFO             = 0x0000202a,
> +    XSS_EXIT_BITMAP                 = 0x0000202c,
> +    XSS_EXIT_BITMAP_HIGH            = 0x0000202d,

Did you not notice that all the ..._HIGH value are gone in favor of
using VMCS_HIGH() in the few cases where a 64-bit hypervisor really
needs to access (or rather, emulate) them?

Jan


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