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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.09.15 at 13:34, <shuai.ruan@xxxxxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4550,6 +4550,23 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, 
> unsigned int *ebx,
>                      *ebx = _eax + _ebx;
>              }
>          }
> +        if ( count == 1 )
> +        {
> +            if ( cpu_has_xsaves )

Doesn't this also depend on the respective VMX capability (which
of course you shouldn't check directly here)?

> +            {
> +                *ebx = XSTATE_AREA_MIN_SIZE;
> +                if ( v->arch.xcr0 | v->arch.hvm_vcpu.msr_xss )
> +                    for ( sub_leaf = 2; sub_leaf < 63; sub_leaf++ )
> +                    {
> +                        if ( !((v->arch.xcr0 | v->arch.hvm_vcpu.msr_xss)
> +                           & (1ULL << sub_leaf)) )

Indentation.

> +                            continue;

Please invert the condition and drop the continue.

> @@ -4781,6 +4804,13 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t 
> msr_content,
>             return X86EMUL_EXCEPTION;
>          break;
>  
> +    case MSR_IA32_XSS:
> +      /* No XSS features currently supported for guests. */

Hard tab. Also - is the patch of much use then?

> @@ -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, 0);
>      vmx_vmcs_exit(v);

Instead of removing a blank line I'd rather see you add another one
before the call to vmx_vmcs_exit().

> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -14,8 +14,8 @@
>  #include <asm/xstate.h>
>  #include <asm/asm_defns.h>
>  
> -static bool_t __read_mostly cpu_has_xsaveopt;
> -static bool_t __read_mostly cpu_has_xsavec;
> +bool_t __read_mostly cpu_has_xsaveopt;
> +bool_t __read_mostly cpu_has_xsavec;

Why? (But iirc this will go away anyway once re-based on Andrew's
changes.)

> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
> @@ -225,6 +225,7 @@ extern u32 vmx_vmentry_control;
>  #define SECONDARY_EXEC_ENABLE_VMCS_SHADOWING    0x00004000
>  #define SECONDARY_EXEC_ENABLE_PML               0x00020000
>  #define SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS   0x00040000
> +#define SECONDARY_EXEC_XSAVES                   0x00100000
>  extern u32 vmx_secondary_exec_control;
>  
>  #define VMX_EPT_EXEC_ONLY_SUPPORTED             0x00000001
> @@ -240,6 +241,8 @@ extern u32 vmx_secondary_exec_control;
>  
>  #define VMX_MISC_VMWRITE_ALL                    0x20000000
>  
> +#define VMX_XSS_EXIT_BITMAP                     0

What use is this addition without it having any user? (And without
any user judging whether this is a meaningful definition is also
impossible.)

Jan


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