Re: [Xen-devel] [PATCH v1 4/6] vvmx: add hvm_max_vmx_msr_policy

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.06.17 at 12:44, <sergey.dyasli@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -244,6 +244,8 @@ static u32 adjust_vmx_controls(
>      return ctl;
>  }
>  
> +void calculate_hvm_max_policy(void);

As said for a prior patch, this once again needs to move to a header
which is also being included by the producer.

> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -1941,6 +1941,8 @@ int nvmx_handle_invvpid(struct cpu_user_regs *regs)
>      return X86EMUL_OKAY;
>  }
>  
> +struct vmx_msr_policy __read_mostly hvm_max_vmx_msr_policy;

Wouldn't vvmx_max_msr_policy be unambiguous enough a name,
but shorter to type?

> @@ -1948,6 +1950,134 @@ int nvmx_handle_invvpid(struct cpu_user_regs *regs)
>      (((__emul_value(enable1, default1) & host_value) & (~0ul << 32)) | \
>      ((uint32_t)(__emul_value(enable1, default1) | host_value)))
>  
> +void __init calculate_hvm_max_policy(void)

This is not a suitable name for a VMX specific function. I should
have noticed and said this in patch 2 already, so please consider
it applicable there too.

> +{
> +    struct vmx_msr_policy *p = &hvm_max_vmx_msr_policy;
> +    uint64_t data, *msr;
> +    u32 default1_bits;
> +
> +    *p = raw_vmx_msr_policy;
> +
> +    /* XXX: vmcs_revision_id for nested virt */

There was no such comment (presumably indicating something that
yet needs doing) in the old code - what's this about? Can't this be
implemented instead of such a comment be added?

> +    /* MSR_IA32_VMX_VMCS_ENUM */
> +    /* The max index of VVMCS encoding is 0x1f. */
> +    data = 0x1f << 1;
> +    msr = &p->msr[MSR_IA32_VMX_VMCS_ENUM - MSR_IA32_VMX_BASIC];
> +    *msr = data;
> +
> +    /* MSR_IA32_VMX_CR0_FIXED0 */
> +    /* PG, PE bits must be 1 in VMX operation */
> +    data = X86_CR0_PE | X86_CR0_PG;
> +    msr = &p->msr[MSR_IA32_VMX_CR0_FIXED0 - MSR_IA32_VMX_BASIC];
> +    *msr = data;
> +
> +    /* MSR_IA32_VMX_CR0_FIXED1 */
> +    /* allow 0-settings for all bits */
> +    data = 0xffffffff;
> +    msr = &p->msr[MSR_IA32_VMX_CR0_FIXED1 - MSR_IA32_VMX_BASIC];
> +    *msr = data;
> +
> +    /* MSR_IA32_VMX_CR4_FIXED0 */
> +    /* VMXE bit must be 1 in VMX operation */
> +    data = X86_CR4_VMXE;
> +    msr = &p->msr[MSR_IA32_VMX_CR4_FIXED0 - MSR_IA32_VMX_BASIC];
> +    *msr = data;

I don't see a need for using "data" as an intermediate variable in any
of the three cases above.

> +    /* MSR_IA32_VMX_CR4_FIXED1 */
> +    /* Treated dynamically */
> +
> +    /* MSR_IA32_VMX_MISC */
> +    /* Do not support CR3-target feature now */
> +    msr = &p->msr[MSR_IA32_VMX_MISC - MSR_IA32_VMX_BASIC];
> +    *msr = *msr & ~VMX_MISC_CR3_TARGET;

&=

> +    /* MSR_IA32_VMX_EPT_VPID_CAP */
> +    data = nept_get_ept_vpid_cap();
> +    msr = &p->msr[MSR_IA32_VMX_EPT_VPID_CAP - MSR_IA32_VMX_BASIC];
> +    *msr = data;

No need to use "data" again.

> +    /* MSR_IA32_VMX_VMFUNC is N/A */
> +    p->available &= ~0x20000;

Please use an expression again here.

Jan


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