* [PATCH v1 1/6] x86/vvmx: introduce vvmcx_valid()
2018-10-12 15:27 [PATCH v1 0/6] x86/vvmx: various fixes Sergey Dyasli
@ 2018-10-12 15:27 ` Sergey Dyasli
2018-10-12 17:10 ` Wei Liu
` (2 more replies)
2018-10-12 15:27 ` [PATCH v1 2/6] x86/vvmx: correct vmfail() usage for vmptrld and vmclear Sergey Dyasli
` (4 subsequent siblings)
5 siblings, 3 replies; 21+ messages in thread
From: Sergey Dyasli @ 2018-10-12 15:27 UTC (permalink / raw)
To: xen-devel
Cc: Sergey Dyasli, Kevin Tian, Wei Liu, Jan Beulich, Andrew Cooper,
Jun Nakajima
As a convenient helper function and refactor the code to use it.
No functional change.
Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
xen/arch/x86/hvm/vmx/vvmx.c | 17 ++++++++---------
xen/include/asm-x86/hvm/nestedhvm.h | 5 +++++
2 files changed, 13 insertions(+), 9 deletions(-)
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 0e45db83e5..8eee6d0ea8 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -521,8 +521,7 @@ static void vmfail(struct cpu_user_regs *regs, enum vmx_insn_errno errno)
if ( errno == VMX_INSN_SUCCEED )
return;
- if ( vcpu_nestedhvm(current).nv_vvmcxaddr != INVALID_PADDR &&
- errno != VMX_INSN_FAIL_INVALID )
+ if ( vvmcx_valid(current) && errno != VMX_INSN_FAIL_INVALID )
vmfail_valid(regs, errno);
else
vmfail_invalid(regs);
@@ -805,7 +804,7 @@ static void nvmx_purge_vvmcs(struct vcpu *v)
int i;
__clear_current_vvmcs(v);
- if ( nvcpu->nv_vvmcxaddr != INVALID_PADDR )
+ if ( vvmcx_valid(v) )
hvm_unmap_guest_frame(nvcpu->nv_vvmcx, 1);
nvcpu->nv_vvmcx = NULL;
nvcpu->nv_vvmcxaddr = INVALID_PADDR;
@@ -1601,7 +1600,7 @@ static int nvmx_vmresume(struct vcpu *v, struct cpu_user_regs *regs)
struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v);
/* check VMCS is valid and IO BITMAP is set */
- if ( (nvcpu->nv_vvmcxaddr != INVALID_PADDR) &&
+ if ( vvmcx_valid(v) &&
((nvmx->iobitmap[0] && nvmx->iobitmap[1]) ||
!(__n2_exec_control(v) & CPU_BASED_ACTIVATE_IO_BITMAP) ) )
nvcpu->nv_vmentry_pending = 1;
@@ -1622,7 +1621,7 @@ int nvmx_handle_vmresume(struct cpu_user_regs *regs)
if ( rc != X86EMUL_OKAY )
return rc;
- if ( vcpu_nestedhvm(v).nv_vvmcxaddr == INVALID_PADDR )
+ if ( !vvmcx_valid(v) )
{
vmfail_invalid(regs);
return X86EMUL_OKAY;
@@ -1656,7 +1655,7 @@ int nvmx_handle_vmlaunch(struct cpu_user_regs *regs)
if ( rc != X86EMUL_OKAY )
return rc;
- if ( vcpu_nestedhvm(v).nv_vvmcxaddr == INVALID_PADDR )
+ if ( !vvmcx_valid(v) )
{
vmfail_invalid(regs);
return X86EMUL_OKAY;
@@ -1709,7 +1708,7 @@ int nvmx_handle_vmptrld(struct cpu_user_regs *regs)
if ( nvcpu->nv_vvmcxaddr != gpa )
nvmx_purge_vvmcs(v);
- if ( nvcpu->nv_vvmcxaddr == INVALID_PADDR )
+ if ( !vvmcx_valid(v) )
{
bool_t writable;
void *vvmcx = hvm_map_guest_frame_rw(paddr_to_pfn(gpa), 1, &writable);
@@ -1848,7 +1847,7 @@ int nvmx_handle_vmread(struct cpu_user_regs *regs)
if ( rc != X86EMUL_OKAY )
return rc;
- if ( vcpu_nestedhvm(v).nv_vvmcxaddr == INVALID_PADDR )
+ if ( !vvmcx_valid(v) )
{
vmfail_invalid(regs);
return X86EMUL_OKAY;
@@ -1891,7 +1890,7 @@ int nvmx_handle_vmwrite(struct cpu_user_regs *regs)
!= X86EMUL_OKAY )
return X86EMUL_EXCEPTION;
- if ( vcpu_nestedhvm(v).nv_vvmcxaddr == INVALID_PADDR )
+ if ( !vvmcx_valid(v) )
{
vmfail_invalid(regs);
return X86EMUL_OKAY;
diff --git a/xen/include/asm-x86/hvm/nestedhvm.h b/xen/include/asm-x86/hvm/nestedhvm.h
index 9d1c2742b5..e09fa9d47d 100644
--- a/xen/include/asm-x86/hvm/nestedhvm.h
+++ b/xen/include/asm-x86/hvm/nestedhvm.h
@@ -92,4 +92,9 @@ static inline void nestedhvm_set_cr(struct vcpu *v, unsigned int cr,
v->arch.hvm.nvcpu.guest_cr[cr] = value;
}
+static inline bool vvmcx_valid(const struct vcpu *v)
+{
+ return vcpu_nestedhvm(v).nv_vvmcxaddr != INVALID_PADDR;
+}
+
#endif /* _HVM_NESTEDHVM_H */
--
2.17.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v1 1/6] x86/vvmx: introduce vvmcx_valid()
2018-10-12 15:27 ` [PATCH v1 1/6] x86/vvmx: introduce vvmcx_valid() Sergey Dyasli
@ 2018-10-12 17:10 ` Wei Liu
2018-10-30 7:41 ` Tian, Kevin
2018-11-01 11:15 ` Andrew Cooper
2 siblings, 0 replies; 21+ messages in thread
From: Wei Liu @ 2018-10-12 17:10 UTC (permalink / raw)
To: Sergey Dyasli
Cc: Kevin Tian, Wei Liu, Jan Beulich, Andrew Cooper, xen-devel,
Jun Nakajima
On Fri, Oct 12, 2018 at 04:27:54PM +0100, Sergey Dyasli wrote:
> As a convenient helper function and refactor the code to use it.
>
> No functional change.
>
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1 1/6] x86/vvmx: introduce vvmcx_valid()
2018-10-12 15:27 ` [PATCH v1 1/6] x86/vvmx: introduce vvmcx_valid() Sergey Dyasli
2018-10-12 17:10 ` Wei Liu
@ 2018-10-30 7:41 ` Tian, Kevin
2018-10-30 12:41 ` Sergey Dyasli
2018-11-01 11:15 ` Andrew Cooper
2 siblings, 1 reply; 21+ messages in thread
From: Tian, Kevin @ 2018-10-30 7:41 UTC (permalink / raw)
To: Sergey Dyasli, xen-devel@lists.xen.org
Cc: Andrew Cooper, Wei Liu, Nakajima, Jun, Jan Beulich
> From: Sergey Dyasli [mailto:sergey.dyasli@citrix.com]
> Sent: Friday, October 12, 2018 11:28 PM
>
> As a convenient helper function and refactor the code to use it.
>
> No functional change.
>
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
since vmcx is hvm abstracted term, what about using this
helper in svm side too?
> ---
> xen/arch/x86/hvm/vmx/vvmx.c | 17 ++++++++---------
> xen/include/asm-x86/hvm/nestedhvm.h | 5 +++++
> 2 files changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c
> b/xen/arch/x86/hvm/vmx/vvmx.c
> index 0e45db83e5..8eee6d0ea8 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -521,8 +521,7 @@ static void vmfail(struct cpu_user_regs *regs, enum
> vmx_insn_errno errno)
> if ( errno == VMX_INSN_SUCCEED )
> return;
>
> - if ( vcpu_nestedhvm(current).nv_vvmcxaddr != INVALID_PADDR &&
> - errno != VMX_INSN_FAIL_INVALID )
> + if ( vvmcx_valid(current) && errno != VMX_INSN_FAIL_INVALID )
> vmfail_valid(regs, errno);
> else
> vmfail_invalid(regs);
> @@ -805,7 +804,7 @@ static void nvmx_purge_vvmcs(struct vcpu *v)
> int i;
>
> __clear_current_vvmcs(v);
> - if ( nvcpu->nv_vvmcxaddr != INVALID_PADDR )
> + if ( vvmcx_valid(v) )
> hvm_unmap_guest_frame(nvcpu->nv_vvmcx, 1);
> nvcpu->nv_vvmcx = NULL;
> nvcpu->nv_vvmcxaddr = INVALID_PADDR;
> @@ -1601,7 +1600,7 @@ static int nvmx_vmresume(struct vcpu *v, struct
> cpu_user_regs *regs)
> struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v);
>
> /* check VMCS is valid and IO BITMAP is set */
> - if ( (nvcpu->nv_vvmcxaddr != INVALID_PADDR) &&
> + if ( vvmcx_valid(v) &&
> ((nvmx->iobitmap[0] && nvmx->iobitmap[1]) ||
> !(__n2_exec_control(v) & CPU_BASED_ACTIVATE_IO_BITMAP) ) )
> nvcpu->nv_vmentry_pending = 1;
> @@ -1622,7 +1621,7 @@ int nvmx_handle_vmresume(struct
> cpu_user_regs *regs)
> if ( rc != X86EMUL_OKAY )
> return rc;
>
> - if ( vcpu_nestedhvm(v).nv_vvmcxaddr == INVALID_PADDR )
> + if ( !vvmcx_valid(v) )
> {
> vmfail_invalid(regs);
> return X86EMUL_OKAY;
> @@ -1656,7 +1655,7 @@ int nvmx_handle_vmlaunch(struct cpu_user_regs
> *regs)
> if ( rc != X86EMUL_OKAY )
> return rc;
>
> - if ( vcpu_nestedhvm(v).nv_vvmcxaddr == INVALID_PADDR )
> + if ( !vvmcx_valid(v) )
> {
> vmfail_invalid(regs);
> return X86EMUL_OKAY;
> @@ -1709,7 +1708,7 @@ int nvmx_handle_vmptrld(struct cpu_user_regs
> *regs)
> if ( nvcpu->nv_vvmcxaddr != gpa )
> nvmx_purge_vvmcs(v);
>
> - if ( nvcpu->nv_vvmcxaddr == INVALID_PADDR )
> + if ( !vvmcx_valid(v) )
> {
> bool_t writable;
> void *vvmcx = hvm_map_guest_frame_rw(paddr_to_pfn(gpa), 1,
> &writable);
> @@ -1848,7 +1847,7 @@ int nvmx_handle_vmread(struct cpu_user_regs
> *regs)
> if ( rc != X86EMUL_OKAY )
> return rc;
>
> - if ( vcpu_nestedhvm(v).nv_vvmcxaddr == INVALID_PADDR )
> + if ( !vvmcx_valid(v) )
> {
> vmfail_invalid(regs);
> return X86EMUL_OKAY;
> @@ -1891,7 +1890,7 @@ int nvmx_handle_vmwrite(struct cpu_user_regs
> *regs)
> != X86EMUL_OKAY )
> return X86EMUL_EXCEPTION;
>
> - if ( vcpu_nestedhvm(v).nv_vvmcxaddr == INVALID_PADDR )
> + if ( !vvmcx_valid(v) )
> {
> vmfail_invalid(regs);
> return X86EMUL_OKAY;
> diff --git a/xen/include/asm-x86/hvm/nestedhvm.h b/xen/include/asm-
> x86/hvm/nestedhvm.h
> index 9d1c2742b5..e09fa9d47d 100644
> --- a/xen/include/asm-x86/hvm/nestedhvm.h
> +++ b/xen/include/asm-x86/hvm/nestedhvm.h
> @@ -92,4 +92,9 @@ static inline void nestedhvm_set_cr(struct vcpu *v,
> unsigned int cr,
> v->arch.hvm.nvcpu.guest_cr[cr] = value;
> }
>
> +static inline bool vvmcx_valid(const struct vcpu *v)
> +{
> + return vcpu_nestedhvm(v).nv_vvmcxaddr != INVALID_PADDR;
> +}
> +
> #endif /* _HVM_NESTEDHVM_H */
> --
> 2.17.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v1 1/6] x86/vvmx: introduce vvmcx_valid()
2018-10-30 7:41 ` Tian, Kevin
@ 2018-10-30 12:41 ` Sergey Dyasli
2018-11-01 2:22 ` Tian, Kevin
0 siblings, 1 reply; 21+ messages in thread
From: Sergey Dyasli @ 2018-10-30 12:41 UTC (permalink / raw)
To: Tian, Kevin, xen-devel@lists.xen.org,
sergey.dyasli@citrix.com >> Sergey Dyasli
Cc: Andrew Cooper, Wei Liu, Nakajima, Jun, Jan Beulich
On 30/10/2018 07:41, Tian, Kevin wrote:
>> From: Sergey Dyasli [mailto:sergey.dyasli@citrix.com]
>> Sent: Friday, October 12, 2018 11:28 PM
>>
>> As a convenient helper function and refactor the code to use it.
>>
>> No functional change.
>>
>> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
>
> since vmcx is hvm abstracted term, what about using this
> helper in svm side too?
I couldn't find any code in nestedsvm.c that would benefit from this new
helper.
--
Thanks,
Sergey
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1 1/6] x86/vvmx: introduce vvmcx_valid()
2018-10-30 12:41 ` Sergey Dyasli
@ 2018-11-01 2:22 ` Tian, Kevin
2018-11-01 8:52 ` Sergey Dyasli
0 siblings, 1 reply; 21+ messages in thread
From: Tian, Kevin @ 2018-11-01 2:22 UTC (permalink / raw)
To: Sergey Dyasli, xen-devel@lists.xen.org
Cc: Andrew Cooper, Wei Liu, Nakajima, Jun, Jan Beulich
> From: Sergey Dyasli [mailto:sergey.dyasli@citrix.com]
> Sent: Tuesday, October 30, 2018 8:41 PM
>
> On 30/10/2018 07:41, Tian, Kevin wrote:
> >> From: Sergey Dyasli [mailto:sergey.dyasli@citrix.com]
> >> Sent: Friday, October 12, 2018 11:28 PM
> >>
> >> As a convenient helper function and refactor the code to use it.
> >>
> >> No functional change.
> >>
> >> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
> >
> > since vmcx is hvm abstracted term, what about using this
> > helper in svm side too?
>
> I couldn't find any code in nestedsvm.c that would benefit from this new
> helper.
>
just a quick check:
if (nv->nv_vvmcx != NULL && nv->nv_vvmcxaddr != vmcbaddr) {
ASSERT(nv->nv_vvmcxaddr != INVALID_PADDR);
hvm_unmap_guest_frame(nv->nv_vvmcx, 1);
nv->nv_vvmcx = NULL;
nv->nv_vvmcxaddr = INVALID_PADDR;
}
Thanks
Kevin
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v1 1/6] x86/vvmx: introduce vvmcx_valid()
2018-11-01 2:22 ` Tian, Kevin
@ 2018-11-01 8:52 ` Sergey Dyasli
0 siblings, 0 replies; 21+ messages in thread
From: Sergey Dyasli @ 2018-11-01 8:52 UTC (permalink / raw)
To: Tian, Kevin, xen-devel@lists.xen.org
Cc: Andrew Cooper, Wei Liu, Nakajima, Jun, Jan Beulich
On 01/11/2018 02:22, Tian, Kevin wrote:
>> From: Sergey Dyasli [mailto:sergey.dyasli@citrix.com]
>> Sent: Tuesday, October 30, 2018 8:41 PM
>>
>> On 30/10/2018 07:41, Tian, Kevin wrote:
>>>> From: Sergey Dyasli [mailto:sergey.dyasli@citrix.com]
>>>> Sent: Friday, October 12, 2018 11:28 PM
>>>>
>>>> As a convenient helper function and refactor the code to use it.
>>>>
>>>> No functional change.
>>>>
>>>> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
>>>
>>> since vmcx is hvm abstracted term, what about using this
>>> helper in svm side too?
>>
>> I couldn't find any code in nestedsvm.c that would benefit from this new
>> helper.
>>
>
> just a quick check:
>
> if (nv->nv_vvmcx != NULL && nv->nv_vvmcxaddr != vmcbaddr) {
> ASSERT(nv->nv_vvmcxaddr != INVALID_PADDR);
> hvm_unmap_guest_frame(nv->nv_vvmcx, 1);
> nv->nv_vvmcx = NULL;
> nv->nv_vvmcxaddr = INVALID_PADDR;
> }
Yes, that's the only usage and it looks better open coded IMHO. But I
can use the new helper here also, if you insist.
Thanks,
Sergey
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1 1/6] x86/vvmx: introduce vvmcx_valid()
2018-10-12 15:27 ` [PATCH v1 1/6] x86/vvmx: introduce vvmcx_valid() Sergey Dyasli
2018-10-12 17:10 ` Wei Liu
2018-10-30 7:41 ` Tian, Kevin
@ 2018-11-01 11:15 ` Andrew Cooper
2 siblings, 0 replies; 21+ messages in thread
From: Andrew Cooper @ 2018-11-01 11:15 UTC (permalink / raw)
To: Sergey Dyasli, xen-devel; +Cc: Kevin Tian, Wei Liu, Jun Nakajima, Jan Beulich
On 12/10/18 16:27, Sergey Dyasli wrote:
> diff --git a/xen/include/asm-x86/hvm/nestedhvm.h b/xen/include/asm-x86/hvm/nestedhvm.h
> index 9d1c2742b5..e09fa9d47d 100644
> --- a/xen/include/asm-x86/hvm/nestedhvm.h
> +++ b/xen/include/asm-x86/hvm/nestedhvm.h
> @@ -92,4 +92,9 @@ static inline void nestedhvm_set_cr(struct vcpu *v, unsigned int cr,
> v->arch.hvm.nvcpu.guest_cr[cr] = value;
> }
>
> +static inline bool vvmcx_valid(const struct vcpu *v)
> +{
> + return vcpu_nestedhvm(v).nv_vvmcxaddr != INVALID_PADDR;
> +}
> +
Now that XSA-278 is public, I can explain why this predicate is a little
broken.
nv_vvmcxaddr doesn't get initialised until HVM_PARAM_NESTED_VIRT is set
to 1.
As with c/s 18cef4df8, please adjust the vcpu construction path to make
this predicate unconditionally safe to use.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v1 2/6] x86/vvmx: correct vmfail() usage for vmptrld and vmclear
2018-10-12 15:27 [PATCH v1 0/6] x86/vvmx: various fixes Sergey Dyasli
2018-10-12 15:27 ` [PATCH v1 1/6] x86/vvmx: introduce vvmcx_valid() Sergey Dyasli
@ 2018-10-12 15:27 ` Sergey Dyasli
2018-10-12 17:10 ` Wei Liu
2018-10-30 7:45 ` Tian, Kevin
2018-10-12 15:27 ` [PATCH v1 3/6] x86/vvmx: add VMX_INSN_VMPTRLD_WITH_VMXON_PTR errno Sergey Dyasli
` (3 subsequent siblings)
5 siblings, 2 replies; 21+ messages in thread
From: Sergey Dyasli @ 2018-10-12 15:27 UTC (permalink / raw)
To: xen-devel
Cc: Sergey Dyasli, Kevin Tian, Wei Liu, Jan Beulich, Andrew Cooper,
Jun Nakajima
Calling vmfail_valid() is correct only if vvmcx is valid. Modify
functions to use vmfail() instead which performs the necessary check.
Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
xen/arch/x86/hvm/vmx/vvmx.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 8eee6d0ea8..26b7d72660 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1744,7 +1744,7 @@ int nvmx_handle_vmptrld(struct cpu_user_regs *regs)
!map_io_bitmap_all(v) ||
!_map_msr_bitmap(v) )
{
- vmfail_valid(regs, VMX_INSN_VMPTRLD_INVALID_PHYADDR);
+ vmfail(regs, VMX_INSN_VMPTRLD_INVALID_PHYADDR);
goto out;
}
}
@@ -1828,7 +1828,7 @@ int nvmx_handle_vmclear(struct cpu_user_regs *regs)
if ( rc == VMSUCCEED )
vmsucceed(regs);
else if ( rc == VMFAIL_VALID )
- vmfail_valid(regs, VMX_INSN_VMCLEAR_INVALID_PHYADDR);
+ vmfail(regs, VMX_INSN_VMCLEAR_INVALID_PHYADDR);
else
vmfail_invalid(regs);
--
2.17.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v1 2/6] x86/vvmx: correct vmfail() usage for vmptrld and vmclear
2018-10-12 15:27 ` [PATCH v1 2/6] x86/vvmx: correct vmfail() usage for vmptrld and vmclear Sergey Dyasli
@ 2018-10-12 17:10 ` Wei Liu
2018-10-30 7:45 ` Tian, Kevin
1 sibling, 0 replies; 21+ messages in thread
From: Wei Liu @ 2018-10-12 17:10 UTC (permalink / raw)
To: Sergey Dyasli
Cc: Kevin Tian, Wei Liu, Jan Beulich, Andrew Cooper, xen-devel,
Jun Nakajima
On Fri, Oct 12, 2018 at 04:27:55PM +0100, Sergey Dyasli wrote:
> Calling vmfail_valid() is correct only if vvmcx is valid. Modify
> functions to use vmfail() instead which performs the necessary check.
>
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
I think the code is a bit convoluted because what fields get access is
hidden behind layers of macros and functions. In order to catch future
issues, may I suggest you add some assertions to vmfail_{in,}valid?
Like
ASSERT(!cpu_has_vmx_vmcs_shadowing && vvmcx_invalid(v));
in vmfail_invalid. Something similar can be added in vmfail_valid as
well.
> ---
> xen/arch/x86/hvm/vmx/vvmx.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> index 8eee6d0ea8..26b7d72660 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -1744,7 +1744,7 @@ int nvmx_handle_vmptrld(struct cpu_user_regs *regs)
> !map_io_bitmap_all(v) ||
> !_map_msr_bitmap(v) )
> {
> - vmfail_valid(regs, VMX_INSN_VMPTRLD_INVALID_PHYADDR);
> + vmfail(regs, VMX_INSN_VMPTRLD_INVALID_PHYADDR);
> goto out;
> }
> }
> @@ -1828,7 +1828,7 @@ int nvmx_handle_vmclear(struct cpu_user_regs *regs)
> if ( rc == VMSUCCEED )
> vmsucceed(regs);
> else if ( rc == VMFAIL_VALID )
> - vmfail_valid(regs, VMX_INSN_VMCLEAR_INVALID_PHYADDR);
> + vmfail(regs, VMX_INSN_VMCLEAR_INVALID_PHYADDR);
> else
> vmfail_invalid(regs);
>
> --
> 2.17.1
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v1 2/6] x86/vvmx: correct vmfail() usage for vmptrld and vmclear
2018-10-12 15:27 ` [PATCH v1 2/6] x86/vvmx: correct vmfail() usage for vmptrld and vmclear Sergey Dyasli
2018-10-12 17:10 ` Wei Liu
@ 2018-10-30 7:45 ` Tian, Kevin
1 sibling, 0 replies; 21+ messages in thread
From: Tian, Kevin @ 2018-10-30 7:45 UTC (permalink / raw)
To: Sergey Dyasli, xen-devel@lists.xen.org
Cc: Andrew Cooper, Wei Liu, Nakajima, Jun, Jan Beulich
> From: Sergey Dyasli [mailto:sergey.dyasli@citrix.com]
> Sent: Friday, October 12, 2018 11:28 PM
>
> Calling vmfail_valid() is correct only if vvmcx is valid. Modify
> functions to use vmfail() instead which performs the necessary check.
>
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
Acked-by: Kevin Tian <kevin.tian@intel.com>. btw Wei's suggestion
on adding assert makes sense to me
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v1 3/6] x86/vvmx: add VMX_INSN_VMPTRLD_WITH_VMXON_PTR errno
2018-10-12 15:27 [PATCH v1 0/6] x86/vvmx: various fixes Sergey Dyasli
2018-10-12 15:27 ` [PATCH v1 1/6] x86/vvmx: introduce vvmcx_valid() Sergey Dyasli
2018-10-12 15:27 ` [PATCH v1 2/6] x86/vvmx: correct vmfail() usage for vmptrld and vmclear Sergey Dyasli
@ 2018-10-12 15:27 ` Sergey Dyasli
2018-10-30 7:49 ` Tian, Kevin
2018-10-12 15:27 ` [PATCH v1 4/6] x86/vvmx: add VMX_INSN_VMCLEAR_WITH_VMXON_PTR errno Sergey Dyasli
` (2 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: Sergey Dyasli @ 2018-10-12 15:27 UTC (permalink / raw)
To: xen-devel
Cc: Sergey Dyasli, Kevin Tian, Wei Liu, Jan Beulich, Andrew Cooper,
Jun Nakajima
And make nvmx_handle_vmptrld() return the new errno in case the provided
address is the same as vmxon region address.
While at it, correct the return value for not-4KB-aligned case.
Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
xen/arch/x86/hvm/vmx/vvmx.c | 10 ++++++++--
xen/include/asm-x86/hvm/vmx/vmcs.h | 1 +
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 26b7d72660..4caa5811a1 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1699,9 +1699,15 @@ int nvmx_handle_vmptrld(struct cpu_user_regs *regs)
if ( rc != X86EMUL_OKAY )
return rc;
- if ( gpa == vcpu_2_nvmx(v).vmxon_region_pa || gpa & 0xfff )
+ if ( gpa & 0xfff )
{
- vmfail_invalid(regs);
+ vmfail(regs, VMX_INSN_VMPTRLD_INVALID_PHYADDR);
+ goto out;
+ }
+
+ if ( gpa == vcpu_2_nvmx(v).vmxon_region_pa )
+ {
+ vmfail(regs, VMX_INSN_VMPTRLD_WITH_VMXON_PTR);
goto out;
}
diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
index 76dd04a72d..cae1861610 100644
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -534,6 +534,7 @@ enum vmx_insn_errno
VMX_INSN_INVALID_CONTROL_STATE = 7,
VMX_INSN_INVALID_HOST_STATE = 8,
VMX_INSN_VMPTRLD_INVALID_PHYADDR = 9,
+ VMX_INSN_VMPTRLD_WITH_VMXON_PTR = 10,
VMX_INSN_VMPTRLD_INCORRECT_VMCS_ID = 11,
VMX_INSN_UNSUPPORTED_VMCS_COMPONENT = 12,
VMX_INSN_VMXON_IN_VMX_ROOT = 15,
--
2.17.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v1 3/6] x86/vvmx: add VMX_INSN_VMPTRLD_WITH_VMXON_PTR errno
2018-10-12 15:27 ` [PATCH v1 3/6] x86/vvmx: add VMX_INSN_VMPTRLD_WITH_VMXON_PTR errno Sergey Dyasli
@ 2018-10-30 7:49 ` Tian, Kevin
0 siblings, 0 replies; 21+ messages in thread
From: Tian, Kevin @ 2018-10-30 7:49 UTC (permalink / raw)
To: Sergey Dyasli, xen-devel@lists.xen.org
Cc: Andrew Cooper, Wei Liu, Nakajima, Jun, Jan Beulich
> From: Sergey Dyasli [mailto:sergey.dyasli@citrix.com]
> Sent: Friday, October 12, 2018 11:28 PM
>
> And make nvmx_handle_vmptrld() return the new errno in case the
> provided
> address is the same as vmxon region address.
>
> While at it, correct the return value for not-4KB-aligned case.
>
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
Acked-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v1 4/6] x86/vvmx: add VMX_INSN_VMCLEAR_WITH_VMXON_PTR errno
2018-10-12 15:27 [PATCH v1 0/6] x86/vvmx: various fixes Sergey Dyasli
` (2 preceding siblings ...)
2018-10-12 15:27 ` [PATCH v1 3/6] x86/vvmx: add VMX_INSN_VMPTRLD_WITH_VMXON_PTR errno Sergey Dyasli
@ 2018-10-12 15:27 ` Sergey Dyasli
2018-10-30 7:53 ` Tian, Kevin
2018-10-12 15:27 ` [PATCH v1 5/6] x86/vvmx: correctly report vvmcs size Sergey Dyasli
2018-10-12 15:27 ` [PATCH v1 6/6] x86/vvmx: fix I/O and MSR bitmaps mapping Sergey Dyasli
5 siblings, 1 reply; 21+ messages in thread
From: Sergey Dyasli @ 2018-10-12 15:27 UTC (permalink / raw)
To: xen-devel
Cc: Sergey Dyasli, Kevin Tian, Wei Liu, Jan Beulich, Andrew Cooper,
Jun Nakajima
And make nvmx_handle_vmclear() return the new errno in case the provided
address is the same as vmxon region address.
While at it, correct the return value for not-4KB-aligned case and for
invalid physaddr.
Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
xen/arch/x86/hvm/vmx/vvmx.c | 23 ++++++++++++++++++-----
xen/include/asm-x86/hvm/vmx/vmcs.h | 1 +
2 files changed, 19 insertions(+), 5 deletions(-)
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 4caa5811a1..8b691bfc04 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1804,9 +1804,20 @@ int nvmx_handle_vmclear(struct cpu_user_regs *regs)
return rc;
BUILD_BUG_ON(X86EMUL_OKAY != VMSUCCEED); /* rc = VMSUCCEED; */
+
+ if ( gpa == vcpu_2_nvmx(v).vmxon_region_pa )
+ {
+ vmfail(regs, VMX_INSN_VMCLEAR_WITH_VMXON_PTR);
+ goto out;
+ }
+
if ( gpa & 0xfff )
- rc = VMFAIL_INVALID;
- else if ( gpa == nvcpu->nv_vvmcxaddr )
+ {
+ vmfail(regs, VMX_INSN_VMCLEAR_INVALID_PHYADDR);
+ goto out;
+ }
+
+ if ( gpa == nvcpu->nv_vvmcxaddr )
{
if ( cpu_has_vmx_vmcs_shadowing )
nvmx_clear_vmcs_pointer(v, nvcpu->nv_vvmcx);
@@ -1820,7 +1831,10 @@ int nvmx_handle_vmclear(struct cpu_user_regs *regs)
bool_t writable;
vvmcs = hvm_map_guest_frame_rw(paddr_to_pfn(gpa), 0, &writable);
- if ( vvmcs )
+
+ if ( !vvmcs )
+ rc = VMFAIL_VALID;
+ else
{
if ( writable )
clear_vvmcs_launched(&nvmx->launched_list,
@@ -1835,9 +1849,8 @@ int nvmx_handle_vmclear(struct cpu_user_regs *regs)
vmsucceed(regs);
else if ( rc == VMFAIL_VALID )
vmfail(regs, VMX_INSN_VMCLEAR_INVALID_PHYADDR);
- else
- vmfail_invalid(regs);
+out:
return X86EMUL_OKAY;
}
diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
index cae1861610..e84d2e482b 100644
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -529,6 +529,7 @@ enum vmx_insn_errno
{
VMX_INSN_SUCCEED = 0,
VMX_INSN_VMCLEAR_INVALID_PHYADDR = 2,
+ VMX_INSN_VMCLEAR_WITH_VMXON_PTR = 3,
VMX_INSN_VMLAUNCH_NONCLEAR_VMCS = 4,
VMX_INSN_VMRESUME_NONLAUNCHED_VMCS = 5,
VMX_INSN_INVALID_CONTROL_STATE = 7,
--
2.17.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v1 4/6] x86/vvmx: add VMX_INSN_VMCLEAR_WITH_VMXON_PTR errno
2018-10-12 15:27 ` [PATCH v1 4/6] x86/vvmx: add VMX_INSN_VMCLEAR_WITH_VMXON_PTR errno Sergey Dyasli
@ 2018-10-30 7:53 ` Tian, Kevin
0 siblings, 0 replies; 21+ messages in thread
From: Tian, Kevin @ 2018-10-30 7:53 UTC (permalink / raw)
To: Sergey Dyasli, xen-devel@lists.xen.org
Cc: Andrew Cooper, Wei Liu, Nakajima, Jun, Jan Beulich
> From: Sergey Dyasli [mailto:sergey.dyasli@citrix.com]
> Sent: Friday, October 12, 2018 11:28 PM
>
> And make nvmx_handle_vmclear() return the new errno in case the
> provided
> address is the same as vmxon region address.
>
> While at it, correct the return value for not-4KB-aligned case and for
> invalid physaddr.
>
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
> ---
> xen/arch/x86/hvm/vmx/vvmx.c | 23 ++++++++++++++++++-----
> xen/include/asm-x86/hvm/vmx/vmcs.h | 1 +
> 2 files changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c
> b/xen/arch/x86/hvm/vmx/vvmx.c
> index 4caa5811a1..8b691bfc04 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -1804,9 +1804,20 @@ int nvmx_handle_vmclear(struct cpu_user_regs
> *regs)
> return rc;
>
> BUILD_BUG_ON(X86EMUL_OKAY != VMSUCCEED); /* rc = VMSUCCEED;
> */
> +
> + if ( gpa == vcpu_2_nvmx(v).vmxon_region_pa )
> + {
> + vmfail(regs, VMX_INSN_VMCLEAR_WITH_VMXON_PTR);
> + goto out;
> + }
> +
> if ( gpa & 0xfff )
> - rc = VMFAIL_INVALID;
> - else if ( gpa == nvcpu->nv_vvmcxaddr )
> + {
> + vmfail(regs, VMX_INSN_VMCLEAR_INVALID_PHYADDR);
> + goto out;
> + }
> +
> + if ( gpa == nvcpu->nv_vvmcxaddr )
> {
> if ( cpu_has_vmx_vmcs_shadowing )
> nvmx_clear_vmcs_pointer(v, nvcpu->nv_vvmcx);
> @@ -1820,7 +1831,10 @@ int nvmx_handle_vmclear(struct cpu_user_regs
> *regs)
> bool_t writable;
>
> vvmcs = hvm_map_guest_frame_rw(paddr_to_pfn(gpa), 0, &writable);
> - if ( vvmcs )
> +
> + if ( !vvmcs )
> + rc = VMFAIL_VALID;
> + else
> {
> if ( writable )
> clear_vvmcs_launched(&nvmx->launched_list,
> @@ -1835,9 +1849,8 @@ int nvmx_handle_vmclear(struct cpu_user_regs
> *regs)
> vmsucceed(regs);
> else if ( rc == VMFAIL_VALID )
> vmfail(regs, VMX_INSN_VMCLEAR_INVALID_PHYADDR);
> - else
> - vmfail_invalid(regs);
>
there is only one place poking rc now. clearer to replace rc with direct
vmfail too. then above rc checks can be removed.
> +out:
> return X86EMUL_OKAY;
> }
>
> diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-
> x86/hvm/vmx/vmcs.h
> index cae1861610..e84d2e482b 100644
> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
> @@ -529,6 +529,7 @@ enum vmx_insn_errno
> {
> VMX_INSN_SUCCEED = 0,
> VMX_INSN_VMCLEAR_INVALID_PHYADDR = 2,
> + VMX_INSN_VMCLEAR_WITH_VMXON_PTR = 3,
> VMX_INSN_VMLAUNCH_NONCLEAR_VMCS = 4,
> VMX_INSN_VMRESUME_NONLAUNCHED_VMCS = 5,
> VMX_INSN_INVALID_CONTROL_STATE = 7,
> --
> 2.17.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v1 5/6] x86/vvmx: correctly report vvmcs size
2018-10-12 15:27 [PATCH v1 0/6] x86/vvmx: various fixes Sergey Dyasli
` (3 preceding siblings ...)
2018-10-12 15:27 ` [PATCH v1 4/6] x86/vvmx: add VMX_INSN_VMCLEAR_WITH_VMXON_PTR errno Sergey Dyasli
@ 2018-10-12 15:27 ` Sergey Dyasli
2018-10-30 8:06 ` Tian, Kevin
2018-10-12 15:27 ` [PATCH v1 6/6] x86/vvmx: fix I/O and MSR bitmaps mapping Sergey Dyasli
5 siblings, 1 reply; 21+ messages in thread
From: Sergey Dyasli @ 2018-10-12 15:27 UTC (permalink / raw)
To: xen-devel
Cc: Sergey Dyasli, Kevin Tian, Wei Liu, Jan Beulich, Andrew Cooper,
Jun Nakajima
The size of Xen's virtual vmcs region is 4096 bytes. Correctly report
it to the guest in case when VMCS shadowing is not available instead of
providing H/W value (which is usually smaller).
Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
xen/arch/x86/hvm/vmx/vvmx.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 8b691bfc04..2c2ba36d94 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -2064,6 +2064,14 @@ int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content)
data = (host_data & (~0ul << 32)) |
(vmcs->vmcs_revision_id & 0x7fffffff);
unmap_domain_page(vmcs);
+
+ if ( !cpu_has_vmx_vmcs_shadowing )
+ {
+ /* Report vmcs_region_size as 4096 */
+ data &= ~VMX_BASIC_VMCS_SIZE_MASK;
+ data |= 1ULL << 44;
+ }
+
break;
}
case MSR_IA32_VMX_PINBASED_CTLS:
--
2.17.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v1 5/6] x86/vvmx: correctly report vvmcs size
2018-10-12 15:27 ` [PATCH v1 5/6] x86/vvmx: correctly report vvmcs size Sergey Dyasli
@ 2018-10-30 8:06 ` Tian, Kevin
2018-10-30 12:36 ` Sergey Dyasli
0 siblings, 1 reply; 21+ messages in thread
From: Tian, Kevin @ 2018-10-30 8:06 UTC (permalink / raw)
To: Sergey Dyasli, xen-devel@lists.xen.org
Cc: Andrew Cooper, Wei Liu, Nakajima, Jun, Jan Beulich
> From: Sergey Dyasli [mailto:sergey.dyasli@citrix.com]
> Sent: Friday, October 12, 2018 11:28 PM
>
> The size of Xen's virtual vmcs region is 4096 bytes. Correctly report
> it to the guest in case when VMCS shadowing is not available instead of
> providing H/W value (which is usually smaller).
what is the problem of reporting smaller size even when actual
size is 4096? is L1 expected to access the portion beyond h/w
reported size?
>
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
> ---
> xen/arch/x86/hvm/vmx/vvmx.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c
> b/xen/arch/x86/hvm/vmx/vvmx.c
> index 8b691bfc04..2c2ba36d94 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -2064,6 +2064,14 @@ int nvmx_msr_read_intercept(unsigned int msr,
> u64 *msr_content)
> data = (host_data & (~0ul << 32)) |
> (vmcs->vmcs_revision_id & 0x7fffffff);
> unmap_domain_page(vmcs);
> +
> + if ( !cpu_has_vmx_vmcs_shadowing )
> + {
> + /* Report vmcs_region_size as 4096 */
> + data &= ~VMX_BASIC_VMCS_SIZE_MASK;
> + data |= 1ULL << 44;
> + }
> +
> break;
> }
> case MSR_IA32_VMX_PINBASED_CTLS:
> --
> 2.17.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v1 5/6] x86/vvmx: correctly report vvmcs size
2018-10-30 8:06 ` Tian, Kevin
@ 2018-10-30 12:36 ` Sergey Dyasli
2018-11-01 2:29 ` Tian, Kevin
0 siblings, 1 reply; 21+ messages in thread
From: Sergey Dyasli @ 2018-10-30 12:36 UTC (permalink / raw)
To: Tian, Kevin, xen-devel@lists.xen.org,
sergey.dyasli@citrix.com >> Sergey Dyasli
Cc: Andrew Cooper, Wei Liu, Nakajima, Jun, Jan Beulich
On 30/10/2018 08:06, Tian, Kevin wrote:
>> From: Sergey Dyasli [mailto:sergey.dyasli@citrix.com]
>> Sent: Friday, October 12, 2018 11:28 PM
>>
>> The size of Xen's virtual vmcs region is 4096 bytes. Correctly report
>> it to the guest in case when VMCS shadowing is not available instead of
>> providing H/W value (which is usually smaller).
>
> what is the problem of reporting smaller size even when actual
> size is 4096? is L1 expected to access the portion beyond h/w
> reported size?
>
Here's the code snippet from kvm-unit-tests:
vmcs[0]->hdr.revision_id = basic.revision;
assert(!vmcs_clear(vmcs[0]));
assert(!make_vmcs_current(vmcs[0]));
set_all_vmcs_fields(0x86);
assert(!vmcs_clear(vmcs[0]));
memcpy(vmcs[1], vmcs[0], basic.size);
assert(!make_vmcs_current(vmcs[1]));
report("test vmclear flush (current VMCS)", check_all_vmcs_fields(0x86));
set_all_vmcs_fields() vmwrites almost 4k, but memcpy() copies only 1024
bytes and vmreads get incorrect values.
>>
>> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
>> ---
>> xen/arch/x86/hvm/vmx/vvmx.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c
>> b/xen/arch/x86/hvm/vmx/vvmx.c
>> index 8b691bfc04..2c2ba36d94 100644
>> --- a/xen/arch/x86/hvm/vmx/vvmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
>> @@ -2064,6 +2064,14 @@ int nvmx_msr_read_intercept(unsigned int msr,
>> u64 *msr_content)
>> data = (host_data & (~0ul << 32)) |
>> (vmcs->vmcs_revision_id & 0x7fffffff);
>> unmap_domain_page(vmcs);
>> +
>> + if ( !cpu_has_vmx_vmcs_shadowing )
>> + {
>> + /* Report vmcs_region_size as 4096 */
>> + data &= ~VMX_BASIC_VMCS_SIZE_MASK;
>> + data |= 1ULL << 44;
>> + }
>> +
>> break;
>> }
>> case MSR_IA32_VMX_PINBASED_CTLS:
>> --
>> 2.17.1
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v1 5/6] x86/vvmx: correctly report vvmcs size
2018-10-30 12:36 ` Sergey Dyasli
@ 2018-11-01 2:29 ` Tian, Kevin
2018-11-01 9:22 ` Sergey Dyasli
0 siblings, 1 reply; 21+ messages in thread
From: Tian, Kevin @ 2018-11-01 2:29 UTC (permalink / raw)
To: Sergey Dyasli, xen-devel@lists.xen.org
Cc: Andrew Cooper, Wei Liu, Nakajima, Jun, Jan Beulich
> From: Sergey Dyasli [mailto:sergey.dyasli@citrix.com]
> Sent: Tuesday, October 30, 2018 8:36 PM
>
> On 30/10/2018 08:06, Tian, Kevin wrote:
> >> From: Sergey Dyasli [mailto:sergey.dyasli@citrix.com]
> >> Sent: Friday, October 12, 2018 11:28 PM
> >>
> >> The size of Xen's virtual vmcs region is 4096 bytes. Correctly report
> >> it to the guest in case when VMCS shadowing is not available instead of
> >> providing H/W value (which is usually smaller).
> >
> > what is the problem of reporting smaller size even when actual
> > size is 4096? is L1 expected to access the portion beyond h/w
> > reported size?
> >
>
> Here's the code snippet from kvm-unit-tests:
>
> vmcs[0]->hdr.revision_id = basic.revision;
> assert(!vmcs_clear(vmcs[0]));
> assert(!make_vmcs_current(vmcs[0]));
> set_all_vmcs_fields(0x86);
>
> assert(!vmcs_clear(vmcs[0]));
> memcpy(vmcs[1], vmcs[0], basic.size);
> assert(!make_vmcs_current(vmcs[1]));
> report("test vmclear flush (current VMCS)",
> check_all_vmcs_fields(0x86));
>
> set_all_vmcs_fields() vmwrites almost 4k, but memcpy() copies only 1024
> bytes and vmreads get incorrect values.
>
I didn't understand why set_all_vmcs_fields blindly touch 4k instead of
following reported size. Also I didn't get the reason of this patch - whatever
size reported, xen just needs to emulate hw behavior according to spec,
i.e. do proper emulation if offset < size, otherwise just vmfail. Guest is
not aware of shadow vmcs. why do we want to report different vmcs
size based on presence of shadow vmcs?
Thanks
Kevin
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v1 5/6] x86/vvmx: correctly report vvmcs size
2018-11-01 2:29 ` Tian, Kevin
@ 2018-11-01 9:22 ` Sergey Dyasli
0 siblings, 0 replies; 21+ messages in thread
From: Sergey Dyasli @ 2018-11-01 9:22 UTC (permalink / raw)
To: Tian, Kevin, xen-devel@lists.xen.org,
sergey.dyasli@citrix.com >> Sergey Dyasli
Cc: Andrew Cooper, Wei Liu, Nakajima, Jun, Jan Beulich
On 01/11/2018 02:29, Tian, Kevin wrote:
>> From: Sergey Dyasli [mailto:sergey.dyasli@citrix.com]
>> Sent: Tuesday, October 30, 2018 8:36 PM
>>
>> On 30/10/2018 08:06, Tian, Kevin wrote:
>>>> From: Sergey Dyasli [mailto:sergey.dyasli@citrix.com]
>>>> Sent: Friday, October 12, 2018 11:28 PM
>>>>
>>>> The size of Xen's virtual vmcs region is 4096 bytes. Correctly report
>>>> it to the guest in case when VMCS shadowing is not available instead of
>>>> providing H/W value (which is usually smaller).
>>>
>>> what is the problem of reporting smaller size even when actual
>>> size is 4096? is L1 expected to access the portion beyond h/w
>>> reported size?
>>>
>>
>> Here's the code snippet from kvm-unit-tests:
>>
>> vmcs[0]->hdr.revision_id = basic.revision;
>> assert(!vmcs_clear(vmcs[0]));
>> assert(!make_vmcs_current(vmcs[0]));
>> set_all_vmcs_fields(0x86);
>>
>> assert(!vmcs_clear(vmcs[0]));
>> memcpy(vmcs[1], vmcs[0], basic.size);
>> assert(!make_vmcs_current(vmcs[1]));
>> report("test vmclear flush (current VMCS)",
>> check_all_vmcs_fields(0x86));
>>
>> set_all_vmcs_fields() vmwrites almost 4k, but memcpy() copies only 1024
>> bytes and vmreads get incorrect values.
>>
>
> I didn't understand why set_all_vmcs_fields blindly touch 4k instead of
> following reported size. Also I didn't get the reason of this patch - whatever
> size reported, xen just needs to emulate hw behavior according to spec,
> i.e. do proper emulation if offset < size, otherwise just vmfail. Guest is
> not aware of shadow vmcs. why do we want to report different vmcs
> size based on presence of shadow vmcs?
Here's the detailed explanation (for when vmcs shadowing is not
available in H/W):
1. Guest reads vmcs region size as 1024 (from H/W), allocates it and
does vmptrld
2. Xen maps provided guest's memory and uses it as virtual vmcs (vmcs12)
3. Guest uses vmwrites to set up the vmcs
4. During emulation (set_vvmcs_virtual()), Xen writes values into
virtual vmcs but the resulting offset can be larger than 1024 (when
multiplied by sizeof(u64)).
There is even a comment in include/asm-x86/hvm/vmx/vvmx.h:
/*
* Virtual VMCS layout
*
* Since physical VMCS layout is unknown, a custom layout is used
* for virtual VMCS seen by guest. It occupies a 4k page, and the
* field is offset by an 9-bit offset into u64[], The offset is as
* follow, which means every <width, type> pair has a max of 32
* fields available.
*
* 9 7 5 0
* --------------------------------
* offset: | width | type | index |
* --------------------------------
*
* Also, since the lower range <width=0, type={0,1}> has only one
* field: VPID, it is moved to a higher offset (63), and leaves the
* lower range to non-indexed field like VMCS revision.
*
*/
--
Thanks,
Sergey
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v1 6/6] x86/vvmx: fix I/O and MSR bitmaps mapping
2018-10-12 15:27 [PATCH v1 0/6] x86/vvmx: various fixes Sergey Dyasli
` (4 preceding siblings ...)
2018-10-12 15:27 ` [PATCH v1 5/6] x86/vvmx: correctly report vvmcs size Sergey Dyasli
@ 2018-10-12 15:27 ` Sergey Dyasli
5 siblings, 0 replies; 21+ messages in thread
From: Sergey Dyasli @ 2018-10-12 15:27 UTC (permalink / raw)
To: xen-devel
Cc: Sergey Dyasli, Kevin Tian, Wei Liu, Jan Beulich, Andrew Cooper,
Jun Nakajima
Currently Xen tries to map bitmaps during emulation of vmptrld and
vmwrite. This is wrong: the guest can store arbitrary values in those
fields.
Make bitmaps mapping happen only during a nested vmentry and only if
the appropriate execution controls are turned on by L1 hypervisor.
Mapping happens only:
1. During the first nested vmentry
2. After L1 has changed an appropriate vmcs field
3. After nvmx_purge_vvmcs() was previously called
Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
xen/arch/x86/hvm/vmx/vvmx.c | 104 +++++++++++++++++++++++-------------
1 file changed, 67 insertions(+), 37 deletions(-)
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 2c2ba36d94..e10ed79f66 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -750,6 +750,17 @@ static void __clear_current_vvmcs(struct vcpu *v)
__vmpclear(nvcpu->nv_n2vmcx_pa);
}
+static void unmap_msr_bitmap(struct vcpu *v)
+{
+ struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
+
+ if ( nvmx->msrbitmap )
+ {
+ hvm_unmap_guest_frame(nvmx->msrbitmap, 1);
+ nvmx->msrbitmap = NULL;
+ }
+}
+
/*
* Refreshes the MSR bitmap mapping for the current nested vcpu. Returns true
* for a successful mapping, and returns false for MSR_BITMAP parameter errors
@@ -760,12 +771,7 @@ static bool __must_check _map_msr_bitmap(struct vcpu *v)
struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
uint64_t gpa;
- if ( nvmx->msrbitmap )
- {
- hvm_unmap_guest_frame(nvmx->msrbitmap, 1);
- nvmx->msrbitmap = NULL;
- }
-
+ unmap_msr_bitmap(v);
gpa = get_vvmcs(v, MSR_BITMAP);
if ( !IS_ALIGNED(gpa, PAGE_SIZE) )
@@ -776,6 +782,17 @@ static bool __must_check _map_msr_bitmap(struct vcpu *v)
return nvmx->msrbitmap != NULL;
}
+static void unmap_io_bitmap(struct vcpu *v, unsigned int idx)
+{
+ struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
+
+ if ( nvmx->iobitmap[idx] )
+ {
+ hvm_unmap_guest_frame(nvmx->iobitmap[idx], 1);
+ nvmx->iobitmap[idx] = NULL;
+ }
+}
+
static bool_t __must_check _map_io_bitmap(struct vcpu *v, u64 vmcs_reg)
{
struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
@@ -783,8 +800,7 @@ static bool_t __must_check _map_io_bitmap(struct vcpu *v, u64 vmcs_reg)
int index;
index = vmcs_reg == IO_BITMAP_A ? 0 : 1;
- if (nvmx->iobitmap[index])
- hvm_unmap_guest_frame(nvmx->iobitmap[index], 1);
+ unmap_io_bitmap(v, index);
gpa = get_vvmcs(v, vmcs_reg);
nvmx->iobitmap[index] = hvm_map_guest_frame_ro(gpa >> PAGE_SHIFT, 1);
@@ -799,7 +815,6 @@ static inline bool_t __must_check map_io_bitmap_all(struct vcpu *v)
static void nvmx_purge_vvmcs(struct vcpu *v)
{
- struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v);
int i;
@@ -809,16 +824,11 @@ static void nvmx_purge_vvmcs(struct vcpu *v)
nvcpu->nv_vvmcx = NULL;
nvcpu->nv_vvmcxaddr = INVALID_PADDR;
v->arch.hvm.vmx.vmcs_shadow_maddr = 0;
- for (i=0; i<2; i++) {
- if ( nvmx->iobitmap[i] ) {
- hvm_unmap_guest_frame(nvmx->iobitmap[i], 1);
- nvmx->iobitmap[i] = NULL;
- }
- }
- if ( nvmx->msrbitmap ) {
- hvm_unmap_guest_frame(nvmx->msrbitmap, 1);
- nvmx->msrbitmap = NULL;
- }
+
+ for ( i = 0; i < 2; i++ )
+ unmap_io_bitmap(v, i);
+
+ unmap_msr_bitmap(v);
}
u64 nvmx_get_tsc_offset(struct vcpu *v)
@@ -1594,20 +1604,34 @@ static void clear_vvmcs_launched(struct list_head *launched_list,
}
}
-static int nvmx_vmresume(struct vcpu *v, struct cpu_user_regs *regs)
+static enum vmx_insn_errno nvmx_vmresume(struct vcpu *v)
{
struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v);
+ unsigned int exec_ctrl;
- /* check VMCS is valid and IO BITMAP is set */
- if ( vvmcx_valid(v) &&
- ((nvmx->iobitmap[0] && nvmx->iobitmap[1]) ||
- !(__n2_exec_control(v) & CPU_BASED_ACTIVATE_IO_BITMAP) ) )
- nvcpu->nv_vmentry_pending = 1;
- else
- vmfail_invalid(regs);
+ ASSERT(vvmcx_valid(v));
+ exec_ctrl = __n2_exec_control(v);
- return X86EMUL_OKAY;
+ if ( exec_ctrl & CPU_BASED_ACTIVATE_IO_BITMAP )
+ {
+ if ( (nvmx->iobitmap[0] == NULL || nvmx->iobitmap[1] == NULL) &&
+ !map_io_bitmap_all(v) )
+ goto invalid_control_state;
+ }
+
+ if ( exec_ctrl & CPU_BASED_ACTIVATE_MSR_BITMAP )
+ {
+ if ( nvmx->msrbitmap == NULL && !_map_msr_bitmap(v) )
+ goto invalid_control_state;
+ }
+
+ nvcpu->nv_vmentry_pending = 1;
+
+ return VMX_INSN_SUCCEED;
+
+invalid_control_state:
+ return VMX_INSN_INVALID_CONTROL_STATE;
}
int nvmx_handle_vmresume(struct cpu_user_regs *regs)
@@ -1641,7 +1665,12 @@ int nvmx_handle_vmresume(struct cpu_user_regs *regs)
vmfail_valid(regs, VMX_INSN_VMRESUME_NONLAUNCHED_VMCS);
return X86EMUL_OKAY;
}
- return nvmx_vmresume(v,regs);
+
+ rc = nvmx_vmresume(v);
+ if ( rc )
+ vmfail_valid(regs, rc);
+
+ return X86EMUL_OKAY;
}
int nvmx_handle_vmlaunch(struct cpu_user_regs *regs)
@@ -1676,13 +1705,16 @@ int nvmx_handle_vmlaunch(struct cpu_user_regs *regs)
return X86EMUL_OKAY;
}
else {
- rc = nvmx_vmresume(v,regs);
- if ( rc == X86EMUL_OKAY )
+ rc = nvmx_vmresume(v);
+ if ( rc )
+ vmfail_valid(regs, rc);
+ else
{
if ( set_vvmcs_launched(&nvmx->launched_list,
PFN_DOWN(v->arch.hvm.vmx.vmcs_shadow_maddr)) < 0 )
return X86EMUL_UNHANDLEABLE;
}
+ rc = X86EMUL_OKAY;
}
return rc;
}
@@ -1746,9 +1778,7 @@ int nvmx_handle_vmptrld(struct cpu_user_regs *regs)
vvmcx = NULL;
}
}
- if ( !vvmcx ||
- !map_io_bitmap_all(v) ||
- !_map_msr_bitmap(v) )
+ else
{
vmfail(regs, VMX_INSN_VMPTRLD_INVALID_PHYADDR);
goto out;
@@ -1926,13 +1956,13 @@ int nvmx_handle_vmwrite(struct cpu_user_regs *regs)
switch ( vmcs_encoding & ~VMCS_HIGH(0) )
{
case IO_BITMAP_A:
- okay = _map_io_bitmap(v, IO_BITMAP_A);
+ unmap_io_bitmap(v, 0);
break;
case IO_BITMAP_B:
- okay = _map_io_bitmap(v, IO_BITMAP_B);
+ unmap_io_bitmap(v, 1);
break;
case MSR_BITMAP:
- okay = _map_msr_bitmap(v);
+ unmap_msr_bitmap(v);
break;
}
--
2.17.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 21+ messages in thread