Re: [PATCH 2/3] Nested VMX: Clear bit 31 of IA32_VMX_BASIC MSR

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 05/09/13 03:57, Yang Zhang wrote:
> From: Yang Zhang <yang.z.zhang@Intel.com>
>
> The bit 31 of revision_id will set to 1 if vmcs shadowing enabled. And
> according intel SDM, the bit 31 of IA32_VMX_BASIC MSR is always 0. So we
> cannot set low 32 bit of IA32_VMX_BASIC to revision_id directly. Must clear
> the bit 31 to 0.
>
> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> ---
>  xen/arch/x86/hvm/vmx/vvmx.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> index 2e0b7f7..8571002 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -1846,7 +1846,7 @@ int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content)
>      switch (msr) {
>      case MSR_IA32_VMX_BASIC:
>          data = (host_data & (~0ul << 32)) |
> -               ((v->arch.hvm_vmx.vmcs)->vmcs_revision_id);
> +               ((v->arch.hvm_vmx.vmcs)->vmcs_revision_id & ~(1ul << 31));

What are the chances of vmcs_revision_id extending beyond 32 bits?

The SDM states that the bottom 31 bits of IA32_VMX_BASIC shall be the
bottom 31 bits of the revision id, so

(v->arch.hvm_vmx.vmcs->vmcs_revision_id & 0x7fffffff);

would seem more obvious.  Also, the brackets were superfluous.

~Andrew

>          break;
>      case MSR_IA32_VMX_PINBASED_CTLS:
>      case MSR_IA32_VMX_TRUE_PINBASED_CTLS: