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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: Gleb Natapov [mailto:gleb@xxxxxxxxxx]
> Sent: Wednesday, November 21, 2012 9:04 PM
> To: Xu, Dongxiao
> Cc: kvm@xxxxxxxxxxxxxxx; mtosatti@xxxxxxxxxx
> Subject: Re: [PATCH 1/4] nested vmx: clean up for vmcs12 read and write
> 
> 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@xxxxxxxxx>
> > ---
> >  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;
> > +	}
> > +
> Shouldn't vmcs_field_readonly() check be in vmcs12_write() instead of a caller?

Well, you can see from the later patch that, we construct vmcs12 through the vmcs12_write() function. Some fields like the exit reason, exit qualification are needed to be written into the vmcs12 area. Therefore we could not put the read only check into the vmcs12_write() function.

Thanks,
Dongxiao

> 
> > +	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);
> >  	/*
> >  	 * 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.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux