| Message ID | 1485861635-3473-4-git-send-email-sergey.dyasli@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 31.01.17 at 12:20, <sergey.dyasli@citrix.com> wrote: > --- a/xen/arch/x86/hvm/vmx/vvmx.c > +++ b/xen/arch/x86/hvm/vmx/vvmx.c > @@ -264,7 +264,7 @@ u64 get_vvmcs_real(const struct vcpu *v, u32 encoding) > return virtual_vmcs_vmread(v, encoding); > } > > -void set_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding, u64 val) > +unsigned long set_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding, u64 val) > { > union vmcs_encoding enc; > u64 *content = (u64 *) vvmcs; > @@ -298,11 +298,13 @@ void set_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding, u64 val) > } > > content[offset] = res; > + > + return 0; > } Is it reasonable for this to never fail? Correct emulation would imo require this to signal invalid fields too. But yes, this could be dealt with in a second step. Jan
On 02/02/17 12:52, Jan Beulich wrote: >>>> On 31.01.17 at 12:20, <sergey.dyasli@citrix.com> wrote: >> --- a/xen/arch/x86/hvm/vmx/vvmx.c >> +++ b/xen/arch/x86/hvm/vmx/vvmx.c >> @@ -264,7 +264,7 @@ u64 get_vvmcs_real(const struct vcpu *v, u32 encoding) >> return virtual_vmcs_vmread(v, encoding); >> } >> >> -void set_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding, u64 val) >> +unsigned long set_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding, u64 val) >> { >> union vmcs_encoding enc; >> u64 *content = (u64 *) vvmcs; >> @@ -298,11 +298,13 @@ void set_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding, u64 val) >> } >> >> content[offset] = res; >> + >> + return 0; >> } > Is it reasonable for this to never fail? No. Fields and values need to be audited against the features offered to the guest in the VT-x MSRs. However, it is not worth trying to fix that now before the MSR levelling work is started, at which point we will have a cpuid_policy-like object to query. ~Andrew
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c index 641e2ef..c1f09c6 100644 --- a/xen/arch/x86/hvm/vmx/vmcs.c +++ b/xen/arch/x86/hvm/vmx/vmcs.c @@ -943,11 +943,16 @@ u64 virtual_vmcs_vmread(const struct vcpu *v, u32 vmcs_encoding) return res; } -void virtual_vmcs_vmwrite(const struct vcpu *v, u32 vmcs_encoding, u64 val) +unsigned long virtual_vmcs_vmwrite(const struct vcpu *v, u32 vmcs_encoding, + u64 val) { + unsigned long ret; + virtual_vmcs_enter(v); - __vmwrite(vmcs_encoding, val); + ret = __vmwrite_safe(vmcs_encoding, val); virtual_vmcs_exit(v); + + return ret; } /* diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c index e0cf0fb..c427a24 100644 --- a/xen/arch/x86/hvm/vmx/vvmx.c +++ b/xen/arch/x86/hvm/vmx/vvmx.c @@ -264,7 +264,7 @@ u64 get_vvmcs_real(const struct vcpu *v, u32 encoding) return virtual_vmcs_vmread(v, encoding); } -void set_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding, u64 val) +unsigned long set_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding, u64 val) { union vmcs_encoding enc; u64 *content = (u64 *) vvmcs; @@ -298,11 +298,13 @@ void set_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding, u64 val) } content[offset] = res; + + return 0; } -void set_vvmcs_real(const struct vcpu *v, u32 encoding, u64 val) +unsigned long set_vvmcs_real(const struct vcpu *v, u32 encoding, u64 val) { - virtual_vmcs_vmwrite(v, encoding, val); + return virtual_vmcs_vmwrite(v, encoding, val); } static unsigned long reg_read(struct cpu_user_regs *regs, @@ -1740,13 +1742,18 @@ int nvmx_handle_vmwrite(struct cpu_user_regs *regs) unsigned long operand; u64 vmcs_encoding; bool_t okay = 1; + unsigned long err; if ( decode_vmx_inst(regs, &decode, &operand, 0) != X86EMUL_OKAY ) return X86EMUL_EXCEPTION; vmcs_encoding = reg_read(regs, decode.reg2); - set_vvmcs(v, vmcs_encoding, operand); + if ( (err = set_vvmcs(v, vmcs_encoding, operand)) ) + { + vmfail(regs, err); + return X86EMUL_OKAY; + } switch ( vmcs_encoding & ~VMCS_HIGH(0) ) { diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h index cb02d53..b30e0f7 100644 --- a/xen/include/asm-x86/hvm/vmx/vmcs.h +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h @@ -541,7 +541,7 @@ int vmx_check_msr_bitmap(unsigned long *msr_bitmap, u32 msr, int access_type); void virtual_vmcs_enter(const struct vcpu *); void virtual_vmcs_exit(const struct vcpu *); u64 virtual_vmcs_vmread(const struct vcpu *, u32 encoding); -void virtual_vmcs_vmwrite(const struct vcpu *, u32 encoding, u64 val); +unsigned long virtual_vmcs_vmwrite(const struct vcpu *, u32 encoding, u64 val); static inline int vmx_add_guest_msr(u32 msr) { diff --git a/xen/include/asm-x86/hvm/vmx/vvmx.h b/xen/include/asm-x86/hvm/vmx/vvmx.h index 242e524..d60e0bb 100644 --- a/xen/include/asm-x86/hvm/vmx/vvmx.h +++ b/xen/include/asm-x86/hvm/vmx/vvmx.h @@ -181,8 +181,8 @@ enum vvmcs_encoding_type { u64 get_vvmcs_virtual(void *vvmcs, u32 encoding); u64 get_vvmcs_real(const struct vcpu *, u32 encoding); -void set_vvmcs_virtual(void *vvmcs, u32 encoding, u64 val); -void set_vvmcs_real(const struct vcpu *, u32 encoding, u64 val); +unsigned long set_vvmcs_virtual(void *vvmcs, u32 encoding, u64 val); +unsigned long set_vvmcs_real(const struct vcpu *, u32 encoding, u64 val); #define get_vvmcs(vcpu, encoding) \ (cpu_has_vmx_vmcs_shadowing ? \
There is an issue with the original __vmwrite() in nested vmx mode: emulation of a guest's VMWRITE with invalid arguments leads to BUG(). Fix this by using __vmwrite_safe() and reporting any kind of VMfail back to the guest. Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> --- xen/arch/x86/hvm/vmx/vmcs.c | 9 +++++++-- xen/arch/x86/hvm/vmx/vvmx.c | 15 +++++++++++---- xen/include/asm-x86/hvm/vmx/vmcs.h | 2 +- xen/include/asm-x86/hvm/vmx/vvmx.h | 4 ++-- 4 files changed, 21 insertions(+), 9 deletions(-)