* [PATCH 0/3] vvmx: fix L1 vmxon
@ 2016-12-13 12:16 Haozhong Zhang
2016-12-13 12:16 ` [PATCH 1/3] vvmx: set vmxon_region_pa of vcpu out of VMX operation to an invalid address Haozhong Zhang
` (2 more replies)
0 siblings, 3 replies; 18+ messages in thread
From: Haozhong Zhang @ 2016-12-13 12:16 UTC (permalink / raw)
To: xen-devel
Cc: Andrew Cooper, Kevin Tian, Jan Beulich, Jun Nakajima, Haozhong Zhang
This patchset fixes bugs and adds missing checks in nvmx_handle_vmxon(),
in order to make it more consistent to Intel SDM (section "VMXON - Enter
VMX Operation" in Vol 3).
Haozhong Zhang (3):
vvmx: set vmxon_region_pa of vcpu out of VMX operation to an invalid address
vvmx: return VMfail to L1 if L1 vmxon is executed in VMX operation
vvmx: check the operand of L1 vmxon
xen/arch/x86/hvm/vmx/vvmx.c | 45 ++++++++++++++++++++++++++++++++++++++-------
1 file changed, 38 insertions(+), 7 deletions(-)
--
2.10.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/3] vvmx: set vmxon_region_pa of vcpu out of VMX operation to an invalid address
2016-12-13 12:16 [PATCH 0/3] vvmx: fix L1 vmxon Haozhong Zhang
@ 2016-12-13 12:16 ` Haozhong Zhang
2016-12-13 14:35 ` Andrew Cooper
` (3 more replies)
2016-12-13 12:16 ` [PATCH 2/3] vvmx: return VMfail to L1 if L1 vmxon is executed in VMX operation Haozhong Zhang
2016-12-13 12:16 ` [PATCH 3/3] vvmx: check the operand of L1 vmxon Haozhong Zhang
2 siblings, 4 replies; 18+ messages in thread
From: Haozhong Zhang @ 2016-12-13 12:16 UTC (permalink / raw)
To: xen-devel
Cc: Andrew Cooper, Kevin Tian, Jan Beulich, Jun Nakajima, Haozhong Zhang
nvmx_handle_vmxon() previously checks whether a vcpu is in VMX
operation by comparing its vmxon_region_pa with GPA 0. However, 0 is
also a valid VMXON region address. If L1 hypervisor had set the VMXON
region address to 0, the check in nvmx_handle_vmxon() will be skipped.
Fix this problem by using an invalid VMXON region address for vcpu
out of VMX operation.
Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
xen/arch/x86/hvm/vmx/vvmx.c | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index e6e9ebd..f5637eb 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -32,6 +32,18 @@ static DEFINE_PER_CPU(u64 *, vvmcs_buf);
static void nvmx_purge_vvmcs(struct vcpu *v);
+/*
+ * When a vcpu is out of VMXON region, set its vmxon_region_pa to
+ * INVALID_VMXON_REGION_PA. We cannot use 0, because 0 is also a valid
+ * VMXON region address.
+ */
+#define INVALID_VMXON_REGION_PA (~0UL)
+
+static bool nvmx_vcpu_in_vmx(struct vcpu *v)
+{
+ return vcpu_2_nvmx(v).vmxon_region_pa != INVALID_VMXON_REGION_PA;
+}
+
#define VMCS_BUF_SIZE 100
int nvmx_cpu_up_prepare(unsigned int cpu)
@@ -107,7 +119,7 @@ int nvmx_vcpu_initialise(struct vcpu *v)
nvmx->ept.enabled = 0;
nvmx->guest_vpid = 0;
- nvmx->vmxon_region_pa = 0;
+ nvmx->vmxon_region_pa = INVALID_VMXON_REGION_PA;
nvcpu->nv_vvmcx = NULL;
nvcpu->nv_vvmcxaddr = VMCX_EADDR;
nvmx->intr.intr_info = 0;
@@ -357,7 +369,7 @@ static int vmx_inst_check_privilege(struct cpu_user_regs *regs, int vmxop_check)
!(v->arch.hvm_vcpu.guest_cr[4] & X86_CR4_VMXE) )
goto invalid_op;
}
- else if ( !vcpu_2_nvmx(v).vmxon_region_pa )
+ else if ( !nvmx_vcpu_in_vmx(v) )
goto invalid_op;
hvm_get_segment_register(v, x86_seg_cs, &cs);
@@ -1384,7 +1396,7 @@ int nvmx_handle_vmxon(struct cpu_user_regs *regs)
if ( rc != X86EMUL_OKAY )
return rc;
- if ( nvmx->vmxon_region_pa )
+ if ( nvmx_vcpu_in_vmx(v) )
gdprintk(XENLOG_WARNING,
"vmxon again: orig %"PRIpaddr" new %lx\n",
nvmx->vmxon_region_pa, gpa);
@@ -1417,7 +1429,7 @@ int nvmx_handle_vmxoff(struct cpu_user_regs *regs)
return rc;
nvmx_purge_vvmcs(v);
- nvmx->vmxon_region_pa = 0;
+ nvmx->vmxon_region_pa = INVALID_VMXON_REGION_PA;
vmreturn(regs, VMSUCCEED);
return X86EMUL_OKAY;
--
2.10.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/3] vvmx: return VMfail to L1 if L1 vmxon is executed in VMX operation
2016-12-13 12:16 [PATCH 0/3] vvmx: fix L1 vmxon Haozhong Zhang
2016-12-13 12:16 ` [PATCH 1/3] vvmx: set vmxon_region_pa of vcpu out of VMX operation to an invalid address Haozhong Zhang
@ 2016-12-13 12:16 ` Haozhong Zhang
2016-12-13 14:46 ` Andrew Cooper
` (2 more replies)
2016-12-13 12:16 ` [PATCH 3/3] vvmx: check the operand of L1 vmxon Haozhong Zhang
2 siblings, 3 replies; 18+ messages in thread
From: Haozhong Zhang @ 2016-12-13 12:16 UTC (permalink / raw)
To: xen-devel
Cc: Andrew Cooper, Kevin Tian, Jan Beulich, Jun Nakajima, Haozhong Zhang
According to Intel SDM, section "VMXON - Enter VMX Operation", a
VMfail should be returned to L1 hypervisor if L1 vmxon is executed in
VMX operation, rather than just print a warning message.
Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
xen/arch/x86/hvm/vmx/vvmx.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index f5637eb..b60d7f0 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1397,9 +1397,12 @@ int nvmx_handle_vmxon(struct cpu_user_regs *regs)
return rc;
if ( nvmx_vcpu_in_vmx(v) )
- gdprintk(XENLOG_WARNING,
- "vmxon again: orig %"PRIpaddr" new %lx\n",
- nvmx->vmxon_region_pa, gpa);
+ {
+ vmreturn(regs,
+ nvcpu->nv_vvmcxaddr != VMCX_EADDR ?
+ VMFAIL_VALID : VMFAIL_INVALID);
+ return X86EMUL_OKAY;
+ }
nvmx->vmxon_region_pa = gpa;
--
2.10.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 3/3] vvmx: check the operand of L1 vmxon
2016-12-13 12:16 [PATCH 0/3] vvmx: fix L1 vmxon Haozhong Zhang
2016-12-13 12:16 ` [PATCH 1/3] vvmx: set vmxon_region_pa of vcpu out of VMX operation to an invalid address Haozhong Zhang
2016-12-13 12:16 ` [PATCH 2/3] vvmx: return VMfail to L1 if L1 vmxon is executed in VMX operation Haozhong Zhang
@ 2016-12-13 12:16 ` Haozhong Zhang
2016-12-13 14:48 ` Andrew Cooper
` (2 more replies)
2 siblings, 3 replies; 18+ messages in thread
From: Haozhong Zhang @ 2016-12-13 12:16 UTC (permalink / raw)
To: xen-devel
Cc: Andrew Cooper, Kevin Tian, Jan Beulich, Jun Nakajima, Haozhong Zhang
Check whether the operand of L1 vmxon is a valid VMXON region address
and whether the VMXON region at that address contains a valid revision
ID.
Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
xen/arch/x86/hvm/vmx/vvmx.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index b60d7f0..7cee307 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1390,6 +1390,7 @@ int nvmx_handle_vmxon(struct cpu_user_regs *regs)
struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v);
struct vmx_inst_decoded decode;
unsigned long gpa = 0;
+ uint32_t nvmcs_revid;
int rc;
rc = decode_vmx_inst(regs, &decode, &gpa, 1);
@@ -1404,6 +1405,21 @@ int nvmx_handle_vmxon(struct cpu_user_regs *regs)
return X86EMUL_OKAY;
}
+ if ( (gpa & ~PAGE_MASK) || (gpa >> v->domain->arch.paging.gfn_bits) )
+ {
+ vmreturn(regs, VMFAIL_INVALID);
+ return X86EMUL_OKAY;
+ }
+
+ rc = hvm_copy_from_guest_phys(&nvmcs_revid, gpa, sizeof(nvmcs_revid));
+ if ( rc != HVMCOPY_okay ||
+ (nvmcs_revid & ~VMX_BASIC_REVISION_MASK) ||
+ ((nvmcs_revid ^ vmx_basic_msr) & VMX_BASIC_REVISION_MASK) )
+ {
+ vmreturn(regs, VMFAIL_INVALID);
+ return X86EMUL_OKAY;
+ }
+
nvmx->vmxon_region_pa = gpa;
/*
--
2.10.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] vvmx: set vmxon_region_pa of vcpu out of VMX operation to an invalid address
2016-12-13 12:16 ` [PATCH 1/3] vvmx: set vmxon_region_pa of vcpu out of VMX operation to an invalid address Haozhong Zhang
@ 2016-12-13 14:35 ` Andrew Cooper
2016-12-13 15:06 ` Konrad Rzeszutek Wilk
2016-12-13 15:19 ` Jan Beulich
` (2 subsequent siblings)
3 siblings, 1 reply; 18+ messages in thread
From: Andrew Cooper @ 2016-12-13 14:35 UTC (permalink / raw)
To: Haozhong Zhang, xen-devel; +Cc: Kevin Tian, Jan Beulich, Jun Nakajima
On 13/12/16 12:16, Haozhong Zhang wrote:
> nvmx_handle_vmxon() previously checks whether a vcpu is in VMX
> operation by comparing its vmxon_region_pa with GPA 0. However, 0 is
> also a valid VMXON region address. If L1 hypervisor had set the VMXON
> region address to 0, the check in nvmx_handle_vmxon() will be skipped.
> Fix this problem by using an invalid VMXON region address for vcpu
> out of VMX operation.
>
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> ---
> xen/arch/x86/hvm/vmx/vvmx.c | 20 ++++++++++++++++----
> 1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> index e6e9ebd..f5637eb 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -32,6 +32,18 @@ static DEFINE_PER_CPU(u64 *, vvmcs_buf);
>
> static void nvmx_purge_vvmcs(struct vcpu *v);
>
> +/*
> + * When a vcpu is out of VMXON region, set its vmxon_region_pa to
> + * INVALID_VMXON_REGION_PA. We cannot use 0, because 0 is also a valid
> + * VMXON region address.
> + */
> +#define INVALID_VMXON_REGION_PA (~0UL)
> +
> +static bool nvmx_vcpu_in_vmx(struct vcpu *v)
const struct vcpu *v.
Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
I can fix this up on commit if no other issues are found by the other
reviewers.
> +{
> + return vcpu_2_nvmx(v).vmxon_region_pa != INVALID_VMXON_REGION_PA;
> +}
> +
> #define VMCS_BUF_SIZE 100
>
> int nvmx_cpu_up_prepare(unsigned int cpu)
> @@ -107,7 +119,7 @@ int nvmx_vcpu_initialise(struct vcpu *v)
>
> nvmx->ept.enabled = 0;
> nvmx->guest_vpid = 0;
> - nvmx->vmxon_region_pa = 0;
> + nvmx->vmxon_region_pa = INVALID_VMXON_REGION_PA;
> nvcpu->nv_vvmcx = NULL;
> nvcpu->nv_vvmcxaddr = VMCX_EADDR;
> nvmx->intr.intr_info = 0;
> @@ -357,7 +369,7 @@ static int vmx_inst_check_privilege(struct cpu_user_regs *regs, int vmxop_check)
> !(v->arch.hvm_vcpu.guest_cr[4] & X86_CR4_VMXE) )
> goto invalid_op;
> }
> - else if ( !vcpu_2_nvmx(v).vmxon_region_pa )
> + else if ( !nvmx_vcpu_in_vmx(v) )
> goto invalid_op;
>
> hvm_get_segment_register(v, x86_seg_cs, &cs);
> @@ -1384,7 +1396,7 @@ int nvmx_handle_vmxon(struct cpu_user_regs *regs)
> if ( rc != X86EMUL_OKAY )
> return rc;
>
> - if ( nvmx->vmxon_region_pa )
> + if ( nvmx_vcpu_in_vmx(v) )
> gdprintk(XENLOG_WARNING,
> "vmxon again: orig %"PRIpaddr" new %lx\n",
> nvmx->vmxon_region_pa, gpa);
> @@ -1417,7 +1429,7 @@ int nvmx_handle_vmxoff(struct cpu_user_regs *regs)
> return rc;
>
> nvmx_purge_vvmcs(v);
> - nvmx->vmxon_region_pa = 0;
> + nvmx->vmxon_region_pa = INVALID_VMXON_REGION_PA;
>
> vmreturn(regs, VMSUCCEED);
> return X86EMUL_OKAY;
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] vvmx: return VMfail to L1 if L1 vmxon is executed in VMX operation
2016-12-13 12:16 ` [PATCH 2/3] vvmx: return VMfail to L1 if L1 vmxon is executed in VMX operation Haozhong Zhang
@ 2016-12-13 14:46 ` Andrew Cooper
2016-12-13 15:16 ` Konrad Rzeszutek Wilk
2016-12-14 5:22 ` Tian, Kevin
2 siblings, 0 replies; 18+ messages in thread
From: Andrew Cooper @ 2016-12-13 14:46 UTC (permalink / raw)
To: Haozhong Zhang, xen-devel; +Cc: Kevin Tian, Jan Beulich, Jun Nakajima
On 13/12/16 12:16, Haozhong Zhang wrote:
> According to Intel SDM, section "VMXON - Enter VMX Operation", a
> VMfail should be returned to L1 hypervisor if L1 vmxon is executed in
> VMX operation, rather than just print a warning message.
>
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, although
> ---
> xen/arch/x86/hvm/vmx/vvmx.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> index f5637eb..b60d7f0 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -1397,9 +1397,12 @@ int nvmx_handle_vmxon(struct cpu_user_regs *regs)
> return rc;
>
> if ( nvmx_vcpu_in_vmx(v) )
> - gdprintk(XENLOG_WARNING,
> - "vmxon again: orig %"PRIpaddr" new %lx\n",
> - nvmx->vmxon_region_pa, gpa);
> + {
> + vmreturn(regs,
> + nvcpu->nv_vvmcxaddr != VMCX_EADDR ?
> + VMFAIL_VALID : VMFAIL_INVALID);
vmreturn really should take a single fail option and internally turn
this into VMFAIL_{VALID,INVALID}, rather than requiring the caller to do so.
~Andrew
> + return X86EMUL_OKAY;
> + }
>
> nvmx->vmxon_region_pa = gpa;
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] vvmx: check the operand of L1 vmxon
2016-12-13 12:16 ` [PATCH 3/3] vvmx: check the operand of L1 vmxon Haozhong Zhang
@ 2016-12-13 14:48 ` Andrew Cooper
2016-12-13 15:18 ` Konrad Rzeszutek Wilk
2016-12-14 5:24 ` Tian, Kevin
2 siblings, 0 replies; 18+ messages in thread
From: Andrew Cooper @ 2016-12-13 14:48 UTC (permalink / raw)
To: Haozhong Zhang, xen-devel; +Cc: Kevin Tian, Jan Beulich, Jun Nakajima
On 13/12/16 12:16, Haozhong Zhang wrote:
> Check whether the operand of L1 vmxon is a valid VMXON region address
> and whether the VMXON region at that address contains a valid revision
> ID.
>
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] vvmx: set vmxon_region_pa of vcpu out of VMX operation to an invalid address
2016-12-13 14:35 ` Andrew Cooper
@ 2016-12-13 15:06 ` Konrad Rzeszutek Wilk
0 siblings, 0 replies; 18+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-12-13 15:06 UTC (permalink / raw)
To: Andrew Cooper
Cc: Haozhong Zhang, Kevin Tian, Jun Nakajima, Jan Beulich, xen-devel
On Tue, Dec 13, 2016 at 02:35:33PM +0000, Andrew Cooper wrote:
> On 13/12/16 12:16, Haozhong Zhang wrote:
> > nvmx_handle_vmxon() previously checks whether a vcpu is in VMX
> > operation by comparing its vmxon_region_pa with GPA 0. However, 0 is
> > also a valid VMXON region address. If L1 hypervisor had set the VMXON
> > region address to 0, the check in nvmx_handle_vmxon() will be skipped.
> > Fix this problem by using an invalid VMXON region address for vcpu
> > out of VMX operation.
> >
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > ---
> > xen/arch/x86/hvm/vmx/vvmx.c | 20 ++++++++++++++++----
> > 1 file changed, 16 insertions(+), 4 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> > index e6e9ebd..f5637eb 100644
> > --- a/xen/arch/x86/hvm/vmx/vvmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> > @@ -32,6 +32,18 @@ static DEFINE_PER_CPU(u64 *, vvmcs_buf);
> >
> > static void nvmx_purge_vvmcs(struct vcpu *v);
> >
> > +/*
> > + * When a vcpu is out of VMXON region, set its vmxon_region_pa to
> > + * INVALID_VMXON_REGION_PA. We cannot use 0, because 0 is also a valid
> > + * VMXON region address.
> > + */
> > +#define INVALID_VMXON_REGION_PA (~0UL)
> > +
> > +static bool nvmx_vcpu_in_vmx(struct vcpu *v)
>
> const struct vcpu *v.
>
> Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] vvmx: return VMfail to L1 if L1 vmxon is executed in VMX operation
2016-12-13 12:16 ` [PATCH 2/3] vvmx: return VMfail to L1 if L1 vmxon is executed in VMX operation Haozhong Zhang
2016-12-13 14:46 ` Andrew Cooper
@ 2016-12-13 15:16 ` Konrad Rzeszutek Wilk
2016-12-14 1:25 ` Haozhong Zhang
2016-12-14 5:22 ` Tian, Kevin
2 siblings, 1 reply; 18+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-12-13 15:16 UTC (permalink / raw)
To: Haozhong Zhang
Cc: Andrew Cooper, Kevin Tian, Jun Nakajima, Jan Beulich, xen-devel
On Tue, Dec 13, 2016 at 08:16:19PM +0800, Haozhong Zhang wrote:
> According to Intel SDM, section "VMXON - Enter VMX Operation", a
> VMfail should be returned to L1 hypervisor if L1 vmxon is executed in
> VMX operation, rather than just print a warning message.
The spec also says to return value 15? But I suppose that means
the TOOD in vmreturn should be implemented?
>
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> ---
> xen/arch/x86/hvm/vmx/vvmx.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> index f5637eb..b60d7f0 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -1397,9 +1397,12 @@ int nvmx_handle_vmxon(struct cpu_user_regs *regs)
> return rc;
>
> if ( nvmx_vcpu_in_vmx(v) )
> - gdprintk(XENLOG_WARNING,
> - "vmxon again: orig %"PRIpaddr" new %lx\n",
> - nvmx->vmxon_region_pa, gpa);
> + {
> + vmreturn(regs,
> + nvcpu->nv_vvmcxaddr != VMCX_EADDR ?
> + VMFAIL_VALID : VMFAIL_INVALID);
> + return X86EMUL_OKAY;
> + }
>
> nvmx->vmxon_region_pa = gpa;
>
> --
> 2.10.1
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] vvmx: check the operand of L1 vmxon
2016-12-13 12:16 ` [PATCH 3/3] vvmx: check the operand of L1 vmxon Haozhong Zhang
2016-12-13 14:48 ` Andrew Cooper
@ 2016-12-13 15:18 ` Konrad Rzeszutek Wilk
2016-12-14 5:24 ` Tian, Kevin
2 siblings, 0 replies; 18+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-12-13 15:18 UTC (permalink / raw)
To: Haozhong Zhang
Cc: Andrew Cooper, Kevin Tian, Jun Nakajima, Jan Beulich, xen-devel
On Tue, Dec 13, 2016 at 08:16:20PM +0800, Haozhong Zhang wrote:
> Check whether the operand of L1 vmxon is a valid VMXON region address
> and whether the VMXON region at that address contains a valid revision
> ID.
>
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Thank you finding these and fixing them!
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] vvmx: set vmxon_region_pa of vcpu out of VMX operation to an invalid address
2016-12-13 12:16 ` [PATCH 1/3] vvmx: set vmxon_region_pa of vcpu out of VMX operation to an invalid address Haozhong Zhang
2016-12-13 14:35 ` Andrew Cooper
@ 2016-12-13 15:19 ` Jan Beulich
2016-12-13 15:21 ` Jan Beulich
2016-12-14 5:18 ` Tian, Kevin
3 siblings, 0 replies; 18+ messages in thread
From: Jan Beulich @ 2016-12-13 15:19 UTC (permalink / raw)
To: Haozhong Zhang; +Cc: Andrew Cooper, Kevin Tian, Jun Nakajima, xen-devel
>>> On 13.12.16 at 13:16, <haozhong.zhang@intel.com> wrote:
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -32,6 +32,18 @@ static DEFINE_PER_CPU(u64 *, vvmcs_buf);
>
> static void nvmx_purge_vvmcs(struct vcpu *v);
>
> +/*
> + * When a vcpu is out of VMXON region, set its vmxon_region_pa to
> + * INVALID_VMXON_REGION_PA. We cannot use 0, because 0 is also a valid
> + * VMXON region address.
> + */
> +#define INVALID_VMXON_REGION_PA (~0UL)
vmxon_region_pa being paddr_t, I think this would better be
explicitly paddr_t, too (rather than unsigned long).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] vvmx: set vmxon_region_pa of vcpu out of VMX operation to an invalid address
2016-12-13 12:16 ` [PATCH 1/3] vvmx: set vmxon_region_pa of vcpu out of VMX operation to an invalid address Haozhong Zhang
2016-12-13 14:35 ` Andrew Cooper
2016-12-13 15:19 ` Jan Beulich
@ 2016-12-13 15:21 ` Jan Beulich
2016-12-14 1:37 ` Haozhong Zhang
2016-12-14 5:18 ` Tian, Kevin
3 siblings, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2016-12-13 15:21 UTC (permalink / raw)
To: Haozhong Zhang; +Cc: Andrew Cooper, Kevin Tian, Jun Nakajima, xen-devel
>>> On 13.12.16 at 13:16, <haozhong.zhang@intel.com> wrote:
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -32,6 +32,18 @@ static DEFINE_PER_CPU(u64 *, vvmcs_buf);
>
> static void nvmx_purge_vvmcs(struct vcpu *v);
>
> +/*
> + * When a vcpu is out of VMXON region, set its vmxon_region_pa to
> + * INVALID_VMXON_REGION_PA. We cannot use 0, because 0 is also a valid
> + * VMXON region address.
> + */
> +#define INVALID_VMXON_REGION_PA (~0UL)
And btw, having looked at patch 2 - any reason you can't simply
re-use VMCX_EADDR here?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] vvmx: return VMfail to L1 if L1 vmxon is executed in VMX operation
2016-12-13 15:16 ` Konrad Rzeszutek Wilk
@ 2016-12-14 1:25 ` Haozhong Zhang
0 siblings, 0 replies; 18+ messages in thread
From: Haozhong Zhang @ 2016-12-14 1:25 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: Andrew Cooper, Kevin Tian, Jun Nakajima, Jan Beulich, xen-devel
On 12/13/16 10:16 -0500, Konrad Rzeszutek Wilk wrote:
>On Tue, Dec 13, 2016 at 08:16:19PM +0800, Haozhong Zhang wrote:
>> According to Intel SDM, section "VMXON - Enter VMX Operation", a
>> VMfail should be returned to L1 hypervisor if L1 vmxon is executed in
>> VMX operation, rather than just print a warning message.
>
>The spec also says to return value 15? But I suppose that means
>the TOOD in vmreturn should be implemented?
>
Yes, it's on my TODO list.
Haozhong
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] vvmx: set vmxon_region_pa of vcpu out of VMX operation to an invalid address
2016-12-13 15:21 ` Jan Beulich
@ 2016-12-14 1:37 ` Haozhong Zhang
2016-12-14 7:08 ` Jan Beulich
0 siblings, 1 reply; 18+ messages in thread
From: Haozhong Zhang @ 2016-12-14 1:37 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Kevin Tian, Jun Nakajima, xen-devel
On 12/13/16 08:21 -0700, Jan Beulich wrote:
>>>> On 13.12.16 at 13:16, <haozhong.zhang@intel.com> wrote:
>> --- a/xen/arch/x86/hvm/vmx/vvmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
>> @@ -32,6 +32,18 @@ static DEFINE_PER_CPU(u64 *, vvmcs_buf);
>>
>> static void nvmx_purge_vvmcs(struct vcpu *v);
>>
>> +/*
>> + * When a vcpu is out of VMXON region, set its vmxon_region_pa to
>> + * INVALID_VMXON_REGION_PA. We cannot use 0, because 0 is also a valid
>> + * VMXON region address.
>> + */
>> +#define INVALID_VMXON_REGION_PA (~0UL)
>
>And btw, having looked at patch 2 - any reason you can't simply
>re-use VMCX_EADDR here?
>
I just find INVALID_PADDR defined along with type paddr_t:
typedef unsigned long paddr_t;
#define INVALID_PADDR (~0UL)
Which one, INVALID_PADDR or VMCX_EADDR, would be better?
Thanks,
Haozhong
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] vvmx: set vmxon_region_pa of vcpu out of VMX operation to an invalid address
2016-12-13 12:16 ` [PATCH 1/3] vvmx: set vmxon_region_pa of vcpu out of VMX operation to an invalid address Haozhong Zhang
` (2 preceding siblings ...)
2016-12-13 15:21 ` Jan Beulich
@ 2016-12-14 5:18 ` Tian, Kevin
3 siblings, 0 replies; 18+ messages in thread
From: Tian, Kevin @ 2016-12-14 5:18 UTC (permalink / raw)
To: Zhang, Haozhong, xen-devel; +Cc: Andrew Cooper, Jan Beulich, Nakajima, Jun
> From: Zhang, Haozhong
> Sent: Tuesday, December 13, 2016 8:16 PM
>
> nvmx_handle_vmxon() previously checks whether a vcpu is in VMX
> operation by comparing its vmxon_region_pa with GPA 0. However, 0 is
> also a valid VMXON region address. If L1 hypervisor had set the VMXON
> region address to 0, the check in nvmx_handle_vmxon() will be skipped.
> Fix this problem by using an invalid VMXON region address for vcpu
> out of VMX operation.
>
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
once you have type thing fixed:
Acked-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] vvmx: return VMfail to L1 if L1 vmxon is executed in VMX operation
2016-12-13 12:16 ` [PATCH 2/3] vvmx: return VMfail to L1 if L1 vmxon is executed in VMX operation Haozhong Zhang
2016-12-13 14:46 ` Andrew Cooper
2016-12-13 15:16 ` Konrad Rzeszutek Wilk
@ 2016-12-14 5:22 ` Tian, Kevin
2 siblings, 0 replies; 18+ messages in thread
From: Tian, Kevin @ 2016-12-14 5:22 UTC (permalink / raw)
To: Zhang, Haozhong, xen-devel; +Cc: Andrew Cooper, Jan Beulich, Nakajima, Jun
> From: Zhang, Haozhong
> Sent: Tuesday, December 13, 2016 8:16 PM
>
> According to Intel SDM, section "VMXON - Enter VMX Operation", a
> VMfail should be returned to L1 hypervisor if L1 vmxon is executed in
> VMX operation, rather than just print a warning message.
>
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
Acked-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] vvmx: check the operand of L1 vmxon
2016-12-13 12:16 ` [PATCH 3/3] vvmx: check the operand of L1 vmxon Haozhong Zhang
2016-12-13 14:48 ` Andrew Cooper
2016-12-13 15:18 ` Konrad Rzeszutek Wilk
@ 2016-12-14 5:24 ` Tian, Kevin
2 siblings, 0 replies; 18+ messages in thread
From: Tian, Kevin @ 2016-12-14 5:24 UTC (permalink / raw)
To: Zhang, Haozhong, xen-devel; +Cc: Andrew Cooper, Jan Beulich, Nakajima, Jun
> From: Zhang, Haozhong
> Sent: Tuesday, December 13, 2016 8:16 PM
>
> Check whether the operand of L1 vmxon is a valid VMXON region address
> and whether the VMXON region at that address contains a valid revision
> ID.
>
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
Acked-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] vvmx: set vmxon_region_pa of vcpu out of VMX operation to an invalid address
2016-12-14 1:37 ` Haozhong Zhang
@ 2016-12-14 7:08 ` Jan Beulich
0 siblings, 0 replies; 18+ messages in thread
From: Jan Beulich @ 2016-12-14 7:08 UTC (permalink / raw)
To: Haozhong Zhang; +Cc: Andrew Cooper, Kevin Tian, Jun Nakajima, xen-devel
>>> On 14.12.16 at 02:37, <haozhong.zhang@intel.com> wrote:
> On 12/13/16 08:21 -0700, Jan Beulich wrote:
>>>>> On 13.12.16 at 13:16, <haozhong.zhang@intel.com> wrote:
>>> --- a/xen/arch/x86/hvm/vmx/vvmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
>>> @@ -32,6 +32,18 @@ static DEFINE_PER_CPU(u64 *, vvmcs_buf);
>>>
>>> static void nvmx_purge_vvmcs(struct vcpu *v);
>>>
>>> +/*
>>> + * When a vcpu is out of VMXON region, set its vmxon_region_pa to
>>> + * INVALID_VMXON_REGION_PA. We cannot use 0, because 0 is also a valid
>>> + * VMXON region address.
>>> + */
>>> +#define INVALID_VMXON_REGION_PA (~0UL)
>>
>>And btw, having looked at patch 2 - any reason you can't simply
>>re-use VMCX_EADDR here?
>>
>
> I just find INVALID_PADDR defined along with type paddr_t:
>
> typedef unsigned long paddr_t;
> #define INVALID_PADDR (~0UL)
>
> Which one, INVALID_PADDR or VMCX_EADDR, would be better?
The former then, I would say. And perhaps ditch VMCX_EADDR in
a separate patch then too, in favor of the more general one.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2016-12-14 7:08 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-13 12:16 [PATCH 0/3] vvmx: fix L1 vmxon Haozhong Zhang
2016-12-13 12:16 ` [PATCH 1/3] vvmx: set vmxon_region_pa of vcpu out of VMX operation to an invalid address Haozhong Zhang
2016-12-13 14:35 ` Andrew Cooper
2016-12-13 15:06 ` Konrad Rzeszutek Wilk
2016-12-13 15:19 ` Jan Beulich
2016-12-13 15:21 ` Jan Beulich
2016-12-14 1:37 ` Haozhong Zhang
2016-12-14 7:08 ` Jan Beulich
2016-12-14 5:18 ` Tian, Kevin
2016-12-13 12:16 ` [PATCH 2/3] vvmx: return VMfail to L1 if L1 vmxon is executed in VMX operation Haozhong Zhang
2016-12-13 14:46 ` Andrew Cooper
2016-12-13 15:16 ` Konrad Rzeszutek Wilk
2016-12-14 1:25 ` Haozhong Zhang
2016-12-14 5:22 ` Tian, Kevin
2016-12-13 12:16 ` [PATCH 3/3] vvmx: check the operand of L1 vmxon Haozhong Zhang
2016-12-13 14:48 ` Andrew Cooper
2016-12-13 15:18 ` Konrad Rzeszutek Wilk
2016-12-14 5:24 ` Tian, Kevin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.