Re: [PATCH 05/29] nVMX: Implement reading and writing of VMX MSRs

[Avi Kivity <avi@redhat.com>]

On 01/27/2011 10:32 AM, Nadav Har'El wrote:
> When the guest can use VMX instructions (when the "nested" module option is
> on), it should also be able to read and write VMX MSRs, e.g., to query about
> VMX capabilities. This patch adds this support.
>
>
> +#define CORE2_PINBASED_CTLS_MUST_BE_ONE	0x00000016
> +	case MSR_IA32_VMX_TRUE_PINBASED_CTLS:
> +	case MSR_IA32_VMX_PINBASED_CTLS:
> +		vmx_msr_low  = CORE2_PINBASED_CTLS_MUST_BE_ONE;
> +		vmx_msr_high = CORE2_PINBASED_CTLS_MUST_BE_ONE |
> +				PIN_BASED_EXT_INTR_MASK |
> +				PIN_BASED_NMI_EXITING |
> +				PIN_BASED_VIRTUAL_NMIS;

Can we actually support PIN_BASED_VIRTUAL_NMIs on hosts which don't 
support them?

Maybe better to drop for the initial version.
> +		rdmsr(MSR_IA32_VMX_PROCBASED_CTLS, vmx_msr_low, vmx_msr_high);
> +
> +		vmx_msr_low = 0; /* allow disabling any feature */
> +		vmx_msr_high&= /* do not expose new untested features */
> +			CPU_BASED_HLT_EXITING | CPU_BASED_CR3_LOAD_EXITING |
> +			CPU_BASED_CR3_STORE_EXITING | CPU_BASED_USE_IO_BITMAPS |
> +			CPU_BASED_MOV_DR_EXITING | CPU_BASED_USE_TSC_OFFSETING |
> +			CPU_BASED_MWAIT_EXITING | CPU_BASED_MONITOR_EXITING |
> +			CPU_BASED_INVLPG_EXITING | CPU_BASED_TPR_SHADOW |
> +#ifdef CONFIG_X86_64
> +			CPU_BASED_CR8_LOAD_EXITING |
> +			CPU_BASED_CR8_STORE_EXITING |
> +#endif
> +			CPU_BASED_ACTIVATE_SECONDARY_CONTROLS;

Similarly, I think CPU_BASED_TPR_SHADOW and 
CPU_BASED_ACTIVATE_SECONDARY_CONTROLS are not available on all models.

> +	case MSR_IA32_VMX_PROCBASED_CTLS2:
> +		*pdata = 0;
> +		if (vm_need_virtualize_apic_accesses(vcpu->kvm))
> +			*pdata |= SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES;
> +		break;

Here, at least you qualify support by checking the host.

> +	case MSR_IA32_VMX_EPT_VPID_CAP:
> +		/* Currently, no nested ept or nested vpid */
> +		*pdata = 0;
> +		break;
> +	default:
> +		return 0;
> +	}
> +
> +	return 1;
> +}
> +
> +static int vmx_set_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 data)
> +{
> +	if (!nested_vmx_allowed(vcpu))
> +		return 0;
> +
> +	/*
> +	 * according to the spec, "VMX capability MSRs are read-only; an
> +	 * attempt to write them (with WRMSR) produces a #GP(0).
> +	 */
> +	if (msr_index>= MSR_IA32_VMX_BASIC&&
> +	    msr_index<= MSR_IA32_VMX_TRUE_ENTRY_CTLS) {
> +		kvm_queue_exception_e(vcpu, GP_VECTOR, 0);
> +		return 1;

Can just drop this part, #GP is the default response.

> +	} else if (msr_index == MSR_IA32_FEATURE_CONTROL)
> +		/* TODO: the right thing. */
> +		return 1;
> +	else
> +		return 0;
> +}

-- 
error compiling committee.c: too many arguments to function