* [RESEND] nested EPT: fix the handling of nested EPT.
@ 2015-06-02 21:06 Liang Li
2015-06-04 13:10 ` Tim Deegan
0 siblings, 1 reply; 7+ messages in thread
From: Liang Li @ 2015-06-02 21:06 UTC (permalink / raw)
To: xen-devel
Cc: kevin.tian, keir, andrew.cooper3, Liang Li, tim, jbeulich, Yang Zhang
If the host EPT entry is changed, the nested EPT should be updated.
The current code does not do this, and it's wrong.
Reported-by: Tim Deegan <tim@xen.org>
Signed-off-by: Liang Li <liang.z.li@intel.com>
Signed-off-by: Yang Zhang <yang.z.zhang@intel.com>
---
xen/arch/x86/mm/p2m-ept.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 5133eb6..26293a0 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -26,6 +26,7 @@
#include <asm/p2m.h>
#include <asm/hvm/vmx/vmx.h>
#include <asm/hvm/vmx/vmcs.h>
+#include <asm/hvm/nestedhvm.h>
#include <xen/iommu.h>
#include <asm/mtrr.h>
#include <asm/hvm/cacheattr.h>
@@ -1076,6 +1077,9 @@ void ept_sync_domain(struct p2m_domain *p2m)
ASSERT(local_irq_is_enabled());
+ if ( nestedhvm_enabled(d) )
+ p2m_flush_nestedp2m(d);
+
/*
* Flush active cpus synchronously. Flush others the next time this domain
* is scheduled onto them. We accept the race of other CPUs adding to
--
1.9.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [RESEND] nested EPT: fix the handling of nested EPT.
2015-06-02 21:06 [RESEND] nested EPT: fix the handling of nested EPT Liang Li
@ 2015-06-04 13:10 ` Tim Deegan
2015-06-05 7:41 ` Li, Liang Z
0 siblings, 1 reply; 7+ messages in thread
From: Tim Deegan @ 2015-06-04 13:10 UTC (permalink / raw)
To: Liang Li
Cc: kevin.tian, keir, andrew.cooper3, xen-devel, jbeulich, Yang Zhang
At 05:06 +0800 on 03 Jun (1433307971), Liang Li wrote:
> If the host EPT entry is changed, the nested EPT should be updated.
> The current code does not do this, and it's wrong.
>
> Reported-by: Tim Deegan <tim@xen.org>
> Signed-off-by: Liang Li <liang.z.li@intel.com>
> Signed-off-by: Yang Zhang <yang.z.zhang@intel.com>
> ---
> xen/arch/x86/mm/p2m-ept.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> index 5133eb6..26293a0 100644
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -26,6 +26,7 @@
> #include <asm/p2m.h>
> #include <asm/hvm/vmx/vmx.h>
> #include <asm/hvm/vmx/vmcs.h>
> +#include <asm/hvm/nestedhvm.h>
> #include <xen/iommu.h>
> #include <asm/mtrr.h>
> #include <asm/hvm/cacheattr.h>
> @@ -1076,6 +1077,9 @@ void ept_sync_domain(struct p2m_domain *p2m)
>
> ASSERT(local_irq_is_enabled());
>
> + if ( nestedhvm_enabled(d) )
> + p2m_flush_nestedp2m(d);
It seems like this should trigger when the nested EPT table itself is
being populated, e.g.
hvm_hap_nested_page_fault
--> nestedhvm_hap_nested_page_fault
--> nestedhap_fix_p2m (--> p2m_lock(p2m))
--> p2m_set_entry
--> ept_set_entry
--> ept_sync_domain
--> p2m_flush_nestedp2m
--> p2m_flush_table (--> p2m_lock(p2m))
which would deadlock on the nested table's p2m lock. What have I missed?
Tim.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RESEND] nested EPT: fix the handling of nested EPT.
2015-06-04 13:10 ` Tim Deegan
@ 2015-06-05 7:41 ` Li, Liang Z
2015-06-11 14:20 ` Tim Deegan
0 siblings, 1 reply; 7+ messages in thread
From: Li, Liang Z @ 2015-06-05 7:41 UTC (permalink / raw)
To: Tim Deegan
Cc: Tian, Kevin, keir, andrew.cooper3, xen-devel, jbeulich, Zhang, Yang Z
> -----Original Message-----
> From: Tim Deegan [mailto:tim@xen.org]
> Sent: Thursday, June 04, 2015 9:11 PM
> To: Li, Liang Z
> Cc: xen-devel@lists.xen.org; keir@xen.org; jbeulich@suse.com;
> andrew.cooper3@citrix.com; Tian, Kevin; Zhang, Yang Z
> Subject: Re: [RESEND] nested EPT: fix the handling of nested EPT.
>
> At 05:06 +0800 on 03 Jun (1433307971), Liang Li wrote:
> > If the host EPT entry is changed, the nested EPT should be updated.
> > The current code does not do this, and it's wrong.
> >
> > Reported-by: Tim Deegan <tim@xen.org>
> > Signed-off-by: Liang Li <liang.z.li@intel.com>
> > Signed-off-by: Yang Zhang <yang.z.zhang@intel.com>
> > ---
> > xen/arch/x86/mm/p2m-ept.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> > index 5133eb6..26293a0 100644
> > --- a/xen/arch/x86/mm/p2m-ept.c
> > +++ b/xen/arch/x86/mm/p2m-ept.c
> > @@ -26,6 +26,7 @@
> > #include <asm/p2m.h>
> > #include <asm/hvm/vmx/vmx.h>
> > #include <asm/hvm/vmx/vmcs.h>
> > +#include <asm/hvm/nestedhvm.h>
> > #include <xen/iommu.h>
> > #include <asm/mtrr.h>
> > #include <asm/hvm/cacheattr.h>
> > @@ -1076,6 +1077,9 @@ void ept_sync_domain(struct p2m_domain *p2m)
> >
> > ASSERT(local_irq_is_enabled());
> >
> > + if ( nestedhvm_enabled(d) )
> > + p2m_flush_nestedp2m(d);
>
>
> It seems like this should trigger when the nested EPT table itself is being
> populated, e.g.
>
> hvm_hap_nested_page_fault
> --> nestedhvm_hap_nested_page_fault
> --> nestedhap_fix_p2m (--> p2m_lock(p2m))
> --> p2m_set_entry
> --> ept_set_entry
> --> ept_sync_domain
> --> p2m_flush_nestedp2m
> --> p2m_flush_table (--> p2m_lock(p2m))
>
> which would deadlock on the nested table's p2m lock. What have I missed?
Yes, you are right.
Maybe we should add another condition checking, just like:
struct vcpu *v = current;
If(nestedhvm_enabled(d) && !nestedhvm_vcpu_in_guestmode(v) )
p2m_flush_nestedp2m(d);
Or add a flag(is_nested_p2m) in the struct p2m_domain to mark a nested EPT, and
change the condition to:
If(nestedhvm_enabled(d) && p2m->is_nested_p2m )
to exclude the case of nested EPT, is that enough?
Liang
> Tim.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RESEND] nested EPT: fix the handling of nested EPT.
2015-06-05 7:41 ` Li, Liang Z
@ 2015-06-11 14:20 ` Tim Deegan
2015-06-12 0:57 ` Li, Liang Z
2015-06-19 10:22 ` Li, Liang Z
0 siblings, 2 replies; 7+ messages in thread
From: Tim Deegan @ 2015-06-11 14:20 UTC (permalink / raw)
To: Li, Liang Z
Cc: Tian, Kevin, keir, andrew.cooper3, xen-devel, jbeulich, Zhang, Yang Z
At 07:41 +0000 on 05 Jun (1433490064), Li, Liang Z wrote:
> > -----Original Message-----
> > From: Tim Deegan [mailto:tim@xen.org]
> > At 05:06 +0800 on 03 Jun (1433307971), Liang Li wrote:
> > > If the host EPT entry is changed, the nested EPT should be updated.
> > > The current code does not do this, and it's wrong.
> > >
> > > Reported-by: Tim Deegan <tim@xen.org>
> > > Signed-off-by: Liang Li <liang.z.li@intel.com>
> > > Signed-off-by: Yang Zhang <yang.z.zhang@intel.com>
> > > ---
> > > xen/arch/x86/mm/p2m-ept.c | 4 ++++
> > > 1 file changed, 4 insertions(+)
> > >
> > > diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> > > index 5133eb6..26293a0 100644
> > > --- a/xen/arch/x86/mm/p2m-ept.c
> > > +++ b/xen/arch/x86/mm/p2m-ept.c
> > > @@ -26,6 +26,7 @@
> > > #include <asm/p2m.h>
> > > #include <asm/hvm/vmx/vmx.h>
> > > #include <asm/hvm/vmx/vmcs.h>
> > > +#include <asm/hvm/nestedhvm.h>
> > > #include <xen/iommu.h>
> > > #include <asm/mtrr.h>
> > > #include <asm/hvm/cacheattr.h>
> > > @@ -1076,6 +1077,9 @@ void ept_sync_domain(struct p2m_domain *p2m)
> > >
> > > ASSERT(local_irq_is_enabled());
> > >
> > > + if ( nestedhvm_enabled(d) )
> > > + p2m_flush_nestedp2m(d);
> >
> >
> > It seems like this should trigger when the nested EPT table itself is being
> > populated, e.g.
> >
> > hvm_hap_nested_page_fault
> > --> nestedhvm_hap_nested_page_fault
> > --> nestedhap_fix_p2m (--> p2m_lock(p2m))
> > --> p2m_set_entry
> > --> ept_set_entry
> > --> ept_sync_domain
> > --> p2m_flush_nestedp2m
> > --> p2m_flush_table (--> p2m_lock(p2m))
> >
> > which would deadlock on the nested table's p2m lock. What have I missed?
>
> Yes, you are right.
>
> Maybe we should add another condition checking, just like:
>
> struct vcpu *v = current;
>
> If(nestedhvm_enabled(d) && !nestedhvm_vcpu_in_guestmode(v) )
> p2m_flush_nestedp2m(d);
>
>
> Or add a flag(is_nested_p2m) in the struct p2m_domain to mark a nested EPT, and
> change the condition to:
> If(nestedhvm_enabled(d) && p2m->is_nested_p2m )
Yes, I think this is the right check, but you shouldn't need to add
any new flags: p2m_is_nestedp2m(p2m) will work.
Also, in the next version of the patch, please describe what testing
you've done.
Thanks
Tim.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RESEND] nested EPT: fix the handling of nested EPT.
2015-06-11 14:20 ` Tim Deegan
@ 2015-06-12 0:57 ` Li, Liang Z
2015-06-19 10:22 ` Li, Liang Z
1 sibling, 0 replies; 7+ messages in thread
From: Li, Liang Z @ 2015-06-12 0:57 UTC (permalink / raw)
To: Tim Deegan
Cc: Tian, Kevin, keir, andrew.cooper3, xen-devel, jbeulich, Zhang, Yang Z
> -----Original Message-----
> From: Tim Deegan [mailto:tim@xen.org]
> Sent: Thursday, June 11, 2015 10:20 PM
> To: Li, Liang Z
> Cc: xen-devel@lists.xen.org; keir@xen.org; jbeulich@suse.com;
> andrew.cooper3@citrix.com; Tian, Kevin; Zhang, Yang Z
> Subject: Re: [RESEND] nested EPT: fix the handling of nested EPT.
>
> At 07:41 +0000 on 05 Jun (1433490064), Li, Liang Z wrote:
> > > -----Original Message-----
> > > From: Tim Deegan [mailto:tim@xen.org] At 05:06 +0800 on 03 Jun
> > > (1433307971), Liang Li wrote:
> > > > If the host EPT entry is changed, the nested EPT should be updated.
> > > > The current code does not do this, and it's wrong.
> > > >
> > > > Reported-by: Tim Deegan <tim@xen.org>
> > > > Signed-off-by: Liang Li <liang.z.li@intel.com>
> > > > Signed-off-by: Yang Zhang <yang.z.zhang@intel.com>
> > > > ---
> > > > xen/arch/x86/mm/p2m-ept.c | 4 ++++
> > > > 1 file changed, 4 insertions(+)
> > > >
> > > > diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-
> ept.c
> > > > index 5133eb6..26293a0 100644
> > > > --- a/xen/arch/x86/mm/p2m-ept.c
> > > > +++ b/xen/arch/x86/mm/p2m-ept.c
> > > > @@ -26,6 +26,7 @@
> > > > #include <asm/p2m.h>
> > > > #include <asm/hvm/vmx/vmx.h>
> > > > #include <asm/hvm/vmx/vmcs.h>
> > > > +#include <asm/hvm/nestedhvm.h>
> > > > #include <xen/iommu.h>
> > > > #include <asm/mtrr.h>
> > > > #include <asm/hvm/cacheattr.h>
> > > > @@ -1076,6 +1077,9 @@ void ept_sync_domain(struct p2m_domain
> *p2m)
> > > >
> > > > ASSERT(local_irq_is_enabled());
> > > >
> > > > + if ( nestedhvm_enabled(d) )
> > > > + p2m_flush_nestedp2m(d);
> > >
> > >
> > > It seems like this should trigger when the nested EPT table itself
> > > is being populated, e.g.
> > >
> > > hvm_hap_nested_page_fault
> > > --> nestedhvm_hap_nested_page_fault
> > > --> nestedhap_fix_p2m (--> p2m_lock(p2m))
> > > --> p2m_set_entry
> > > --> ept_set_entry
> > > --> ept_sync_domain
> > > --> p2m_flush_nestedp2m
> > > --> p2m_flush_table (--> p2m_lock(p2m))
> > >
> > > which would deadlock on the nested table's p2m lock. What have I
> missed?
> >
> > Yes, you are right.
> >
> > Maybe we should add another condition checking, just like:
> >
> > struct vcpu *v = current;
> >
> > If(nestedhvm_enabled(d) && !nestedhvm_vcpu_in_guestmode(v) )
> > p2m_flush_nestedp2m(d);
> >
> >
> > Or add a flag(is_nested_p2m) in the struct p2m_domain to mark a nested
> > EPT, and change the condition to:
> > If(nestedhvm_enabled(d) && p2m->is_nested_p2m )
>
> Yes, I think this is the right check, but you shouldn't need to add any new
> flags: p2m_is_nestedp2m(p2m) will work.
>
> Also, in the next version of the patch, please describe what testing you've
> done.
Thanks Tim. I will do test this time ....
> Thanks
> Tim.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RESEND] nested EPT: fix the handling of nested EPT.
2015-06-11 14:20 ` Tim Deegan
2015-06-12 0:57 ` Li, Liang Z
@ 2015-06-19 10:22 ` Li, Liang Z
2015-06-25 9:54 ` Tim Deegan
1 sibling, 1 reply; 7+ messages in thread
From: Li, Liang Z @ 2015-06-19 10:22 UTC (permalink / raw)
To: Tim Deegan
Cc: Tian, Kevin, keir, andrew.cooper3, xen-devel, jbeulich, Zhang, Yang Z
> > > > xen/arch/x86/mm/p2m-ept.c | 4 ++++
> > > > 1 file changed, 4 insertions(+)
> > > >
> > > > diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-
> ept.c
> > > > index 5133eb6..26293a0 100644
> > > > --- a/xen/arch/x86/mm/p2m-ept.c
> > > > +++ b/xen/arch/x86/mm/p2m-ept.c
> > > > @@ -26,6 +26,7 @@
> > > > #include <asm/p2m.h>
> > > > #include <asm/hvm/vmx/vmx.h>
> > > > #include <asm/hvm/vmx/vmcs.h>
> > > > +#include <asm/hvm/nestedhvm.h>
> > > > #include <xen/iommu.h>
> > > > #include <asm/mtrr.h>
> > > > #include <asm/hvm/cacheattr.h>
> > > > @@ -1076,6 +1077,9 @@ void ept_sync_domain(struct p2m_domain
> *p2m)
> > > >
> > > > ASSERT(local_irq_is_enabled());
> > > >
> > > > + if ( nestedhvm_enabled(d) )
> > > > + p2m_flush_nestedp2m(d);
> > >
> > >
> > > It seems like this should trigger when the nested EPT table itself
> > > is being populated, e.g.
> > >
> > > hvm_hap_nested_page_fault
> > > --> nestedhvm_hap_nested_page_fault
> > > --> nestedhap_fix_p2m (--> p2m_lock(p2m))
> > > --> p2m_set_entry
> > > --> ept_set_entry
> > > --> ept_sync_domain
> > > --> p2m_flush_nestedp2m
> > > --> p2m_flush_table (--> p2m_lock(p2m))
> > >
> > > which would deadlock on the nested table's p2m lock. What have I
> missed?
> >
> > Yes, you are right.
> >
> > Maybe we should add another condition checking, just like:
> >
> > struct vcpu *v = current;
> >
> > If(nestedhvm_enabled(d) && !nestedhvm_vcpu_in_guestmode(v) )
> > p2m_flush_nestedp2m(d);
> >
> >
> > Or add a flag(is_nested_p2m) in the struct p2m_domain to mark a nested
> > EPT, and change the condition to:
> > If(nestedhvm_enabled(d) && p2m->is_nested_p2m )
>
> Yes, I think this is the right check, but you shouldn't need to add any new
> flags: p2m_is_nestedp2m(p2m) will work.
>
> Also, in the next version of the patch, please describe what testing you've
> done.
Hi Tim,
I can boot the L2 guest successfully with my new patch, and I want to verify that without the fixed patch,
there are some issue, but I don't know how to trigger the bug. Which case should the test cover?
Could you give some suggestion?
Liang
> Thanks
>
> Tim.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RESEND] nested EPT: fix the handling of nested EPT.
2015-06-19 10:22 ` Li, Liang Z
@ 2015-06-25 9:54 ` Tim Deegan
0 siblings, 0 replies; 7+ messages in thread
From: Tim Deegan @ 2015-06-25 9:54 UTC (permalink / raw)
To: Li, Liang Z
Cc: Tian, Kevin, keir, andrew.cooper3, xen-devel, jbeulich, Zhang, Yang Z
At 10:22 +0000 on 19 Jun (1434709345), Li, Liang Z wrote:
> > > > > xen/arch/x86/mm/p2m-ept.c | 4 ++++
> > > > > 1 file changed, 4 insertions(+)
> > > > >
> > > > > diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-
> > ept.c
> > > > > index 5133eb6..26293a0 100644
> > > > > --- a/xen/arch/x86/mm/p2m-ept.c
> > > > > +++ b/xen/arch/x86/mm/p2m-ept.c
> > > > > @@ -26,6 +26,7 @@
> > > > > #include <asm/p2m.h>
> > > > > #include <asm/hvm/vmx/vmx.h>
> > > > > #include <asm/hvm/vmx/vmcs.h>
> > > > > +#include <asm/hvm/nestedhvm.h>
> > > > > #include <xen/iommu.h>
> > > > > #include <asm/mtrr.h>
> > > > > #include <asm/hvm/cacheattr.h>
> > > > > @@ -1076,6 +1077,9 @@ void ept_sync_domain(struct p2m_domain
> > *p2m)
> > > > >
> > > > > ASSERT(local_irq_is_enabled());
> > > > >
> > > > > + if ( nestedhvm_enabled(d) )
> > > > > + p2m_flush_nestedp2m(d);
> > > >
> > > >
> > > > It seems like this should trigger when the nested EPT table itself
> > > > is being populated, e.g.
> > > >
> > > > hvm_hap_nested_page_fault
> > > > --> nestedhvm_hap_nested_page_fault
> > > > --> nestedhap_fix_p2m (--> p2m_lock(p2m))
> > > > --> p2m_set_entry
> > > > --> ept_set_entry
> > > > --> ept_sync_domain
> > > > --> p2m_flush_nestedp2m
> > > > --> p2m_flush_table (--> p2m_lock(p2m))
> > > >
> > > > which would deadlock on the nested table's p2m lock. What have I
> > missed?
> > >
> > > Yes, you are right.
> > >
> > > Maybe we should add another condition checking, just like:
> > >
> > > struct vcpu *v = current;
> > >
> > > If(nestedhvm_enabled(d) && !nestedhvm_vcpu_in_guestmode(v) )
> > > p2m_flush_nestedp2m(d);
> > >
> > >
> > > Or add a flag(is_nested_p2m) in the struct p2m_domain to mark a nested
> > > EPT, and change the condition to:
> > > If(nestedhvm_enabled(d) && p2m->is_nested_p2m )
> >
> > Yes, I think this is the right check, but you shouldn't need to add any new
> > flags: p2m_is_nestedp2m(p2m) will work.
> >
> > Also, in the next version of the patch, please describe what testing you've
> > done.
>
> Hi Tim,
>
> I can boot the L2 guest successfully with my new patch, and I want to verify that without the fixed patch,
> there are some issue, but I don't know how to trigger the bug. Which case should the test cover?
> Could you give some suggestion?
I'm not sure - I found the bug in the code, and not on a running
system. I cn't even think of a way to provoke it without writing a
malicious L1 guest.
IMO if it doesn't break existing nested support that'll be enough
testing.
Tim.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-06-25 9:54 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-02 21:06 [RESEND] nested EPT: fix the handling of nested EPT Liang Li
2015-06-04 13:10 ` Tim Deegan
2015-06-05 7:41 ` Li, Liang Z
2015-06-11 14:20 ` Tim Deegan
2015-06-12 0:57 ` Li, Liang Z
2015-06-19 10:22 ` Li, Liang Z
2015-06-25 9:54 ` Tim Deegan
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).