* [PATCH v2 1/3] xen/x86: ensure copying to L1 guest in update_runstate_area()
@ 2017-02-23 9:41 Haozhong Zhang
2017-02-23 9:41 ` [PATCH v2 2/3] xen/x86: ensure copying to L1 guest in update_secondary_system_time() Haozhong Zhang
2017-02-23 9:41 ` [PATCH v2 3/3] x86/hvm: check HAP before enabling nested VMX Haozhong Zhang
0 siblings, 2 replies; 5+ messages in thread
From: Haozhong Zhang @ 2017-02-23 9:41 UTC (permalink / raw)
To: xen-devel; +Cc: Haozhong Zhang, Jan Beulich, Andrew Cooper
For a HVM domain, if a vcpu is in the nested guest mode,
__raw_copy_to_guest() and __copy_to_guest() used by
update_runstate_area() will copy data to L2 guest rather than L1
guest.
Besides copying to the wrong address, this bug also causes crash in
the code path:
context_switch(prev, next)
_update_runstate_area(next)
update_runstate_area(next)
__raw_copy_to_guest(...)
...
__hvm_copy(...)
paging_gva_to_gfn(...)
nestedhap_walk_L1_p2m(...)
nvmx_hap_walk_L1_p2m(..)
vmx_vmcs_enter(v) [ v = next ]
vmx_vmcs_try_enter(v) [ v = next ]
if ( likely(v == current) )
return v->arch.hvm_vmx.vmcs_pa == this_cpu(current_vmcs);
vmx_vmcs_try_enter() will fail and trigger the assert in
vmx_vmcs_enter(), if vcpu 'next' is in the nested guest mode and is
being scheduled to another CPU.
This commit temporally clears the nested guest flag before all
__raw_copy_to_guest() and __copy_to_guest() in update_runstate_area(),
and restores the flag after those guest copy operations.
Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
xen/arch/x86/domain.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 7d3071e..6053dcf 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -50,6 +50,7 @@
#include <asm/mpspec.h>
#include <asm/ldt.h>
#include <asm/hvm/hvm.h>
+#include <asm/hvm/nestedhvm.h>
#include <asm/hvm/support.h>
#include <asm/hvm/viridian.h>
#include <asm/debugreg.h>
@@ -1931,10 +1932,29 @@ bool_t update_runstate_area(struct vcpu *v)
bool_t rc;
smap_check_policy_t smap_policy;
void __user *guest_handle = NULL;
+ bool nested_guest_mode = false;
if ( guest_handle_is_null(runstate_guest(v)) )
return 1;
+ /*
+ * Must be before all following __raw_copy_to_guest() and __copy_to_guest().
+ *
+ * Otherwise, if 'v' is in the nested guest mode, paging_gva_to_gfn() called
+ * from __raw_copy_to_guest() and __copy_to_guest() will treat the target
+ * address as L2 gva, and __raw_copy_to_guest() and __copy_to_guest() will
+ * consequently copy runstate to L2 guest rather than L1 guest.
+ *
+ * Therefore, we clear the nested guest flag before __raw_copy_to_guest()
+ * and __copy_to_guest(), and restore the flag after all guest copy.
+ */
+ if ( nestedhvm_enabled(v->domain) )
+ {
+ nested_guest_mode = nestedhvm_is_n2(v);
+ if ( nested_guest_mode )
+ nestedhvm_vcpu_exit_guestmode(v);
+ }
+
smap_policy = smap_policy_change(v, SMAP_CHECK_ENABLED);
if ( VM_ASSIST(v->domain, runstate_update_flag) )
@@ -1971,6 +1991,9 @@ bool_t update_runstate_area(struct vcpu *v)
smap_policy_change(v, smap_policy);
+ if ( unlikely(nested_guest_mode) )
+ nestedhvm_vcpu_enter_guestmode(v);
+
return rc;
}
--
2.10.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v2 2/3] xen/x86: ensure copying to L1 guest in update_secondary_system_time()
2017-02-23 9:41 [PATCH v2 1/3] xen/x86: ensure copying to L1 guest in update_runstate_area() Haozhong Zhang
@ 2017-02-23 9:41 ` Haozhong Zhang
2017-02-24 15:24 ` Jan Beulich
2017-02-23 9:41 ` [PATCH v2 3/3] x86/hvm: check HAP before enabling nested VMX Haozhong Zhang
1 sibling, 1 reply; 5+ messages in thread
From: Haozhong Zhang @ 2017-02-23 9:41 UTC (permalink / raw)
To: xen-devel; +Cc: Haozhong Zhang, Jan Beulich, Andrew Cooper
For a HVM domain, if a vcpu is in the nested guest mode,
__copy_field_to_guest() and __copy_to_guest() used by
update_secondary_system_time() will copy data to L2 guest rather than
L1 guest.
This commit temporally clears the nested guest flag before all
__copy_field_to_guest() and __copy_to_guest() in
update_secondary_system_time(), and restores the flag after those
guest copy operations.
Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
Suggested-by: Jan Beulich <jbeulich@suse.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
xen/arch/x86/time.c | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index 3ad2ab0..cb69dd5 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -24,6 +24,7 @@
#include <xen/symbols.h>
#include <xen/keyhandler.h>
#include <xen/guest_access.h>
+#include <asm/hvm/nestedhvm.h>
#include <asm/io.h>
#include <asm/msr.h>
#include <asm/mpspec.h>
@@ -992,10 +993,30 @@ bool_t update_secondary_system_time(struct vcpu *v,
{
XEN_GUEST_HANDLE(vcpu_time_info_t) user_u = v->arch.time_info_guest;
smap_check_policy_t saved_policy;
+ bool nested_guest_mode = false;
if ( guest_handle_is_null(user_u) )
return 1;
+ /*
+ * Must be before all following __copy_field_to_guest() and
+ * __copy_to_guest().
+ *
+ * Otherwise, if 'v' is in the nested guest mode, paging_gva_to_gfn() called
+ * from __copy_field_to_guest() and __copy_to_guest() will treat the target
+ * address as L2 gva, and __copy_field_to_guest() and __copy_to_guest() will
+ * consequently copy runstate to L2 guest rather than L1 guest.
+ *
+ * Therefore, we clear the nested guest flag before __copy_field_to_guest()
+ * and __copy_to_guest(), and restore the flag after all guest copy.
+ */
+ if ( nestedhvm_enabled(v->domain) )
+ {
+ nested_guest_mode = nestedhvm_is_n2(v);
+ if ( nested_guest_mode )
+ nestedhvm_vcpu_exit_guestmode(v);
+ }
+
saved_policy = smap_policy_change(v, SMAP_CHECK_ENABLED);
/* 1. Update userspace version. */
@@ -1014,6 +1035,9 @@ bool_t update_secondary_system_time(struct vcpu *v,
smap_policy_change(v, saved_policy);
+ if ( unlikely(nested_guest_mode) )
+ nestedhvm_vcpu_enter_guestmode(v);
+
return 1;
}
--
2.10.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v2 3/3] x86/hvm: check HAP before enabling nested VMX
2017-02-23 9:41 [PATCH v2 1/3] xen/x86: ensure copying to L1 guest in update_runstate_area() Haozhong Zhang
2017-02-23 9:41 ` [PATCH v2 2/3] xen/x86: ensure copying to L1 guest in update_secondary_system_time() Haozhong Zhang
@ 2017-02-23 9:41 ` Haozhong Zhang
2017-02-24 15:27 ` Jan Beulich
1 sibling, 1 reply; 5+ messages in thread
From: Haozhong Zhang @ 2017-02-23 9:41 UTC (permalink / raw)
To: xen-devel; +Cc: Haozhong Zhang, Jan Beulich, Andrew Cooper
The current implementation of nested VMX cannot work without HAP.
Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
xen/arch/x86/hvm/hvm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 6621d62..b8db808 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4123,7 +4123,7 @@ static int hvmop_set_param(
* Remove the check below once we have
* shadow-on-shadow.
*/
- if ( cpu_has_svm && !paging_mode_hap(d) && a.value )
+ if ( (cpu_has_svm || cpu_has_vmx) && !paging_mode_hap(d) && a.value )
rc = -EINVAL;
if ( a.value &&
d->arch.hvm_domain.params[HVM_PARAM_ALTP2M] )
--
2.10.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 2/3] xen/x86: ensure copying to L1 guest in update_secondary_system_time()
2017-02-23 9:41 ` [PATCH v2 2/3] xen/x86: ensure copying to L1 guest in update_secondary_system_time() Haozhong Zhang
@ 2017-02-24 15:24 ` Jan Beulich
0 siblings, 0 replies; 5+ messages in thread
From: Jan Beulich @ 2017-02-24 15:24 UTC (permalink / raw)
To: Haozhong Zhang; +Cc: Andrew Cooper, xen-devel
>>> On 23.02.17 at 10:41, <haozhong.zhang@intel.com> wrote:
> @@ -992,10 +993,30 @@ bool_t update_secondary_system_time(struct vcpu *v,
> {
> XEN_GUEST_HANDLE(vcpu_time_info_t) user_u = v->arch.time_info_guest;
> smap_check_policy_t saved_policy;
> + bool nested_guest_mode = false;
>
> if ( guest_handle_is_null(user_u) )
> return 1;
>
> + /*
> + * Must be before all following __copy_field_to_guest() and
> + * __copy_to_guest().
> + *
> + * Otherwise, if 'v' is in the nested guest mode, paging_gva_to_gfn() called
> + * from __copy_field_to_guest() and __copy_to_guest() will treat the target
> + * address as L2 gva, and __copy_field_to_guest() and __copy_to_guest() will
> + * consequently copy runstate to L2 guest rather than L1 guest.
> + *
> + * Therefore, we clear the nested guest flag before __copy_field_to_guest()
> + * and __copy_to_guest(), and restore the flag after all guest copy.
> + */
> + if ( nestedhvm_enabled(v->domain) )
> + {
> + nested_guest_mode = nestedhvm_is_n2(v);
> + if ( nested_guest_mode )
> + nestedhvm_vcpu_exit_guestmode(v);
> + }
> +
> saved_policy = smap_policy_change(v, SMAP_CHECK_ENABLED);
>
> /* 1. Update userspace version. */
There is an early exit path right below here. Taking this together with
the code and comment redundancy with patch 1, this is a pretty clear
sign that you want to rename smap_policy_change() and use the new
function, taking care of both issues, in both code paths.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 3/3] x86/hvm: check HAP before enabling nested VMX
2017-02-23 9:41 ` [PATCH v2 3/3] x86/hvm: check HAP before enabling nested VMX Haozhong Zhang
@ 2017-02-24 15:27 ` Jan Beulich
0 siblings, 0 replies; 5+ messages in thread
From: Jan Beulich @ 2017-02-24 15:27 UTC (permalink / raw)
To: Haozhong Zhang; +Cc: Andrew Cooper, xen-devel
>>> On 23.02.17 at 10:41, <haozhong.zhang@intel.com> wrote:
> The current implementation of nested VMX cannot work without HAP.
Of course the better route would be to fix the actual problem,
the more that - according to other feedback you've got elsewhere -
this apparently is a regression. Nevertheless I can see you perhaps
not having the time to do so, so as a band aid it's likely fine.
However ...
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4123,7 +4123,7 @@ static int hvmop_set_param(
> * Remove the check below once we have
> * shadow-on-shadow.
> */
> - if ( cpu_has_svm && !paging_mode_hap(d) && a.value )
> + if ( (cpu_has_svm || cpu_has_vmx) && !paging_mode_hap(d) && a.value )
... you want to simply drop the cpu_has_svm check here instead of
adding to it, as with neither SVM nor VMX available execution will
never come here.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-02-24 15:27 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-02-23 9:41 [PATCH v2 1/3] xen/x86: ensure copying to L1 guest in update_runstate_area() Haozhong Zhang
2017-02-23 9:41 ` [PATCH v2 2/3] xen/x86: ensure copying to L1 guest in update_secondary_system_time() Haozhong Zhang
2017-02-24 15:24 ` Jan Beulich
2017-02-23 9:41 ` [PATCH v2 3/3] x86/hvm: check HAP before enabling nested VMX Haozhong Zhang
2017-02-24 15:27 ` Jan Beulich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).