Re: [PATCH 1/4] nested vmx: clean up for vmcs12 read and write

[Gleb Natapov <gleb@redhat.com>]

On Wed, Nov 21, 2012 at 05:04:34PM +0800, Dongxiao Xu wrote:
> abstract vmcs12_read and vmcs12_write functions to do the vmcs12
> read/write operations.
> 
> Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
> ---
>  arch/x86/kvm/vmx.c |   86 +++++++++++++++++++++++++++-------------------------
>  1 files changed, 45 insertions(+), 41 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index f858159..d8670e4 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -5407,32 +5407,67 @@ static inline int vmcs_field_readonly(unsigned long field)
>   * some of the bits we return here (e.g., on 32-bit guests, only 32 bits of
>   * 64-bit fields are to be returned).
>   */
> -static inline bool vmcs12_read_any(struct kvm_vcpu *vcpu,
> -					unsigned long field, u64 *ret)
> +static inline u64 vmcs12_read(struct kvm_vcpu *vcpu, unsigned long field)
>  {
>  	short offset = vmcs_field_to_offset(field);
>  	char *p;
>  
> -	if (offset < 0)
> +	if (offset < 0) {
> +		nested_vmx_failValid(vcpu, VMXERR_UNSUPPORTED_VMCS_COMPONENT);
> +		skip_emulated_instruction(vcpu);
>  		return 0;
> +	}
>  
>  	p = ((char *)(get_vmcs12(vcpu))) + offset;
>  
>  	switch (vmcs_field_type(field)) {
>  	case VMCS_FIELD_TYPE_NATURAL_WIDTH:
> -		*ret = *((natural_width *)p);
> +		return *((natural_width *)p);
> +	case VMCS_FIELD_TYPE_U16:
> +		return *((u16 *)p);
> +	case VMCS_FIELD_TYPE_U32:
> +		return *((u32 *)p);
> +	case VMCS_FIELD_TYPE_U64:
> +		return *((u64 *)p);
> +	default:
> +		nested_vmx_failValid(vcpu, VMXERR_UNSUPPORTED_VMCS_COMPONENT);
> +		skip_emulated_instruction(vcpu);
> +		return 0; /* can never happen. */
> +	}
> +}
> +
> +static inline int vmcs12_write(struct kvm_vcpu *vcpu,
> +			unsigned long field,
> +			u64 value)
> +{
> +	short offset = vmcs_field_to_offset(field);
> +	char *p;
> +
> +	if (offset < 0) {
> +		nested_vmx_failValid(vcpu, VMXERR_UNSUPPORTED_VMCS_COMPONENT);
> +		skip_emulated_instruction(vcpu);
> +		return 0;
> +	}
> +
> +	p = ((char *)(get_vmcs12(vcpu))) + offset;
> +
> +	switch (vmcs_field_type(field)) {
> +	case VMCS_FIELD_TYPE_NATURAL_WIDTH:
> +		*(natural_width *)p = value;
>  		return 1;
>  	case VMCS_FIELD_TYPE_U16:
> -		*ret = *((u16 *)p);
> +		*(u16 *)p = value;
>  		return 1;
>  	case VMCS_FIELD_TYPE_U32:
> -		*ret = *((u32 *)p);
> +		*(u32 *)p = value;
>  		return 1;
>  	case VMCS_FIELD_TYPE_U64:
> -		*ret = *((u64 *)p);
> +		*(u64 *)p = value;
>  		return 1;
>  	default:
> -		return 0; /* can never happen. */
> +		nested_vmx_failValid(vcpu, VMXERR_UNSUPPORTED_VMCS_COMPONENT);
> +		skip_emulated_instruction(vcpu);
> +		return 0;
>  	}
>  }
>  
> @@ -5466,11 +5501,7 @@ static int handle_vmread(struct kvm_vcpu *vcpu)
>  	/* Decode instruction info and find the field to read */
>  	field = kvm_register_read(vcpu, (((vmx_instruction_info) >> 28) & 0xf));
>  	/* Read the field, zero-extended to a u64 field_value */
> -	if (!vmcs12_read_any(vcpu, field, &field_value)) {
> -		nested_vmx_failValid(vcpu, VMXERR_UNSUPPORTED_VMCS_COMPONENT);
> -		skip_emulated_instruction(vcpu);
> -		return 1;
> -	}
> +	field_value = vmcs12_read(vcpu, field);
You do not handle failure here and always write back field_value even if
vmcs12_read() failed. Actually now it is impossible to detect a
failure. Call to nested_vmx_failValid() in vmcs12_read() will be
overwritten by call to nested_vmx_succeed() at the end of
handle_vmread() and skip_emulated_instruction() will be called twice.

> +             skip_emulated_instruction(vcpu);

>  	/*
>  	 * Now copy part of this value to register or memory, as requested.
>  	 * Note that the number of bits actually copied is 32 or 64 depending
> @@ -5500,8 +5531,6 @@ static int handle_vmwrite(struct kvm_vcpu *vcpu)
>  	gva_t gva;
>  	unsigned long exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
>  	u32 vmx_instruction_info = vmcs_read32(VMX_INSTRUCTION_INFO);
> -	char *p;
> -	short offset;
>  	/* The value to write might be 32 or 64 bits, depending on L1's long
>  	 * mode, and eventually we need to write that into a field of several
>  	 * possible lengths. The code below first zero-extends the value to 64
> @@ -5537,33 +5566,8 @@ static int handle_vmwrite(struct kvm_vcpu *vcpu)
>  		skip_emulated_instruction(vcpu);
>  		return 1;
>  	}
> -
> -	offset = vmcs_field_to_offset(field);
> -	if (offset < 0) {
> -		nested_vmx_failValid(vcpu, VMXERR_UNSUPPORTED_VMCS_COMPONENT);
> -		skip_emulated_instruction(vcpu);
> -		return 1;
> -	}
> -	p = ((char *) get_vmcs12(vcpu)) + offset;
> -
> -	switch (vmcs_field_type(field)) {
> -	case VMCS_FIELD_TYPE_U16:
> -		*(u16 *)p = field_value;
> -		break;
> -	case VMCS_FIELD_TYPE_U32:
> -		*(u32 *)p = field_value;
> -		break;
> -	case VMCS_FIELD_TYPE_U64:
> -		*(u64 *)p = field_value;
> -		break;
> -	case VMCS_FIELD_TYPE_NATURAL_WIDTH:
> -		*(natural_width *)p = field_value;
> -		break;
> -	default:
> -		nested_vmx_failValid(vcpu, VMXERR_UNSUPPORTED_VMCS_COMPONENT);
> -		skip_emulated_instruction(vcpu);
> +	if (!vmcs12_write(vcpu, field, field_value))
>  		return 1;
> -	}
>  
>  	nested_vmx_succeed(vcpu);
>  	skip_emulated_instruction(vcpu);
> -- 
> 1.7.1

--
			Gleb.