Re: [Xen-devel] [PATCH] x86/vMSI-X: Fix host crash when shutting down guests with MSI capable devices
On Thu, Jul 21, 2016 at 11:18 AM, Andrew Cooperwrote: > c/s 74c6dc2d "x86/vMSI-X: defer intercept handler registration" caused MSI-X > table infrastructure not to always be initialised, but it missed one path > which needed an is-initialised check. > > If a devices is passed through to a domain which is MSI capable but not MSI-X > capable, the call to msixtbl_init() is omitted, but a XEN_DOMCTL_unbind_pt_irq > hypercall still calls into msixtbl_pt_unregister(). This follows the linked > list pointer which is still NULL. > > Introduce an is-initalised check to msixtbl_pt_unregister(). > > Furthermore, the purpose of the open-coded msixtbl_list.next check is rather > subtle. Introduce an msixtbl_initialised() predicate instead, which makes its > purpose far more obvious. Thanks for this bit. > Reported-by: Sander Eikelenboom > Signed-off-by: Andrew Cooper Reviewed-by: George Dunlap ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] x86/vMSI-X: Fix host crash when shutting down guests with MSI capable devices
Monday, July 25, 2016, 12:19:55 PM, you wrote: > On 25/07/16 11:16, Andrew Cooper wrote: >> On 22/07/16 09:50, Sander Eikelenboom wrote: >>> Thursday, July 21, 2016, 12:18:37 PM, you wrote: >>> c/s 74c6dc2d "x86/vMSI-X: defer intercept handler registration" caused MSI-X table infrastructure not to always be initialised, but it missed one path which needed an is-initialised check. If a devices is passed through to a domain which is MSI capable but not MSI-X capable, the call to msixtbl_init() is omitted, but a XEN_DOMCTL_unbind_pt_irq hypercall still calls into msixtbl_pt_unregister(). This follows the linked list pointer which is still NULL. Introduce an is-initalised check to msixtbl_pt_unregister(). Furthermore, the purpose of the open-coded msixtbl_list.next check is rather subtle. Introduce an msixtbl_initialised() predicate instead, which makes its purpose far more obvious. Reported-by: Sander EikelenboomSigned-off-by: Andrew Cooper --- CC: Jan Beulich CC: Sander Eikelenboom Sander - would you mind double checking this patch? --- >>> Hi Andrew, >>> >>> Just got the chance to test and it works for me ! >>> >>> Thanks, >> May I take that as a Test-by: then please? > And of course, I meant Tested-by: Yes, thanks for the quick fix ! -- Sander > ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] x86/vMSI-X: Fix host crash when shutting down guests with MSI capable devices
On 25/07/16 11:16, Andrew Cooper wrote: > On 22/07/16 09:50, Sander Eikelenboom wrote: >> Thursday, July 21, 2016, 12:18:37 PM, you wrote: >> >>> c/s 74c6dc2d "x86/vMSI-X: defer intercept handler registration" caused MSI-X >>> table infrastructure not to always be initialised, but it missed one path >>> which needed an is-initialised check. >>> If a devices is passed through to a domain which is MSI capable but not >>> MSI-X >>> capable, the call to msixtbl_init() is omitted, but a >>> XEN_DOMCTL_unbind_pt_irq >>> hypercall still calls into msixtbl_pt_unregister(). This follows the linked >>> list pointer which is still NULL. >>> Introduce an is-initalised check to msixtbl_pt_unregister(). >>> Furthermore, the purpose of the open-coded msixtbl_list.next check is rather >>> subtle. Introduce an msixtbl_initialised() predicate instead, which makes >>> its >>> purpose far more obvious. >>> Reported-by: Sander Eikelenboom>>> Signed-off-by: Andrew Cooper >>> --- >>> CC: Jan Beulich >>> CC: Sander Eikelenboom >>> Sander - would you mind double checking this patch? >>> --- >> Hi Andrew, >> >> Just got the chance to test and it works for me ! >> >> Thanks, > May I take that as a Test-by: then please? And of course, I meant Tested-by: ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] x86/vMSI-X: Fix host crash when shutting down guests with MSI capable devices
On 22/07/16 09:50, Sander Eikelenboom wrote: > Thursday, July 21, 2016, 12:18:37 PM, you wrote: > >> c/s 74c6dc2d "x86/vMSI-X: defer intercept handler registration" caused MSI-X >> table infrastructure not to always be initialised, but it missed one path >> which needed an is-initialised check. >> If a devices is passed through to a domain which is MSI capable but not MSI-X >> capable, the call to msixtbl_init() is omitted, but a >> XEN_DOMCTL_unbind_pt_irq >> hypercall still calls into msixtbl_pt_unregister(). This follows the linked >> list pointer which is still NULL. >> Introduce an is-initalised check to msixtbl_pt_unregister(). >> Furthermore, the purpose of the open-coded msixtbl_list.next check is rather >> subtle. Introduce an msixtbl_initialised() predicate instead, which makes >> its >> purpose far more obvious. >> Reported-by: Sander Eikelenboom>> Signed-off-by: Andrew Cooper >> --- >> CC: Jan Beulich >> CC: Sander Eikelenboom >> Sander - would you mind double checking this patch? >> --- > Hi Andrew, > > Just got the chance to test and it works for me ! > > Thanks, May I take that as a Test-by: then please? ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] x86/vMSI-X: Fix host crash when shutting down guests with MSI capable devices
Thursday, July 21, 2016, 12:18:37 PM, you wrote: > c/s 74c6dc2d "x86/vMSI-X: defer intercept handler registration" caused MSI-X > table infrastructure not to always be initialised, but it missed one path > which needed an is-initialised check. > If a devices is passed through to a domain which is MSI capable but not MSI-X > capable, the call to msixtbl_init() is omitted, but a XEN_DOMCTL_unbind_pt_irq > hypercall still calls into msixtbl_pt_unregister(). This follows the linked > list pointer which is still NULL. > Introduce an is-initalised check to msixtbl_pt_unregister(). > Furthermore, the purpose of the open-coded msixtbl_list.next check is rather > subtle. Introduce an msixtbl_initialised() predicate instead, which makes its > purpose far more obvious. > Reported-by: Sander Eikelenboom> Signed-off-by: Andrew Cooper > --- > CC: Jan Beulich > CC: Sander Eikelenboom > Sander - would you mind double checking this patch? > --- Hi Andrew, Just got the chance to test and it works for me ! Thanks, Sander > xen/arch/x86/hvm/vmsi.c | 16 +--- > 1 file changed, 13 insertions(+), 3 deletions(-) > diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c > index e418b98..ef1dfff 100644 > --- a/xen/arch/x86/hvm/vmsi.c > +++ b/xen/arch/x86/hvm/vmsi.c > @@ -166,6 +166,16 @@ struct msixtbl_entry > > static DEFINE_RCU_READ_LOCK(msixtbl_rcu_lock); > > +/* > + * MSI-X table infrastructure is dynamically initialised when an MSI-X > capable > + * device is passed through to a domain, rather than unconditionally for all > + * domains. > + */ > +static bool msixtbl_initialised(const struct domain *d) > +{ +return !!d->>arch.hvm_domain.msixtbl_list.next; > +} > + > static struct msixtbl_entry *msixtbl_find_entry( > struct vcpu *v, unsigned long addr) > { > @@ -519,7 +529,7 @@ void msixtbl_pt_unregister(struct domain *d, struct pirq > *pirq) > ASSERT(pcidevs_locked()); > ASSERT(spin_is_locked(>event_lock)); > > -if ( !has_vlapic(d) ) > +if ( !msixtbl_initialised(d) ) > return; > > irq_desc = pirq_spin_lock_irq_desc(pirq, NULL); > @@ -552,7 +562,7 @@ void msixtbl_init(struct domain *d) > struct hvm_io_handler *handler; > > if ( !has_hvm_container_domain(d) || !has_vlapic(d) || - d->>arch.hvm_domain.msixtbl_list.next ) > + msixtbl_initialised(d) ) > return; > > INIT_LIST_HEAD(>arch.hvm_domain.msixtbl_list); > @@ -569,7 +579,7 @@ void msixtbl_pt_cleanup(struct domain *d) > { > struct msixtbl_entry *entry, *temp; > -if ( !d->>arch.hvm_domain.msixtbl_list.next ) > +if ( !msixtbl_initialised(d) ) > return; > > spin_lock(>event_lock); ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] x86/vMSI-X: Fix host crash when shutting down guests with MSI capable devices
On July 21, 2016 12:18:37 PM GMT+02:00, Andrew Cooperwrote: >c/s 74c6dc2d "x86/vMSI-X: defer intercept handler registration" caused >MSI-X >table infrastructure not to always be initialised, but it missed one >path >which needed an is-initialised check. > >If a devices is passed through to a domain which is MSI capable but not >MSI-X >capable, the call to msixtbl_init() is omitted, but a >XEN_DOMCTL_unbind_pt_irq >hypercall still calls into msixtbl_pt_unregister(). This follows the >linked >list pointer which is still NULL. > >Introduce an is-initalised check to msixtbl_pt_unregister(). > >Furthermore, the purpose of the open-coded msixtbl_list.next check is >rather >subtle. Introduce an msixtbl_initialised() predicate instead, which >makes its >purpose far more obvious. > >Reported-by: Sander Eikelenboom >Signed-off-by: Andrew Cooper >--- >CC: Jan Beulich >CC: Sander Eikelenboom > >Sander - would you mind double checking this patch? >--- Sure, will report back tomorrow. -- Sander > xen/arch/x86/hvm/vmsi.c | 16 +--- > 1 file changed, 13 insertions(+), 3 deletions(-) > >diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c >index e418b98..ef1dfff 100644 >--- a/xen/arch/x86/hvm/vmsi.c >+++ b/xen/arch/x86/hvm/vmsi.c >@@ -166,6 +166,16 @@ struct msixtbl_entry > > static DEFINE_RCU_READ_LOCK(msixtbl_rcu_lock); > >+/* >+ * MSI-X table infrastructure is dynamically initialised when an MSI-X >capable >+ * device is passed through to a domain, rather than unconditionally >for all >+ * domains. >+ */ >+static bool msixtbl_initialised(const struct domain *d) >+{ >+return !!d->arch.hvm_domain.msixtbl_list.next; >+} >+ > static struct msixtbl_entry *msixtbl_find_entry( > struct vcpu *v, unsigned long addr) > { >@@ -519,7 +529,7 @@ void msixtbl_pt_unregister(struct domain *d, struct >pirq *pirq) > ASSERT(pcidevs_locked()); > ASSERT(spin_is_locked(>event_lock)); > >-if ( !has_vlapic(d) ) >+if ( !msixtbl_initialised(d) ) > return; > > irq_desc = pirq_spin_lock_irq_desc(pirq, NULL); >@@ -552,7 +562,7 @@ void msixtbl_init(struct domain *d) > struct hvm_io_handler *handler; > > if ( !has_hvm_container_domain(d) || !has_vlapic(d) || >- d->arch.hvm_domain.msixtbl_list.next ) >+ msixtbl_initialised(d) ) > return; > > INIT_LIST_HEAD(>arch.hvm_domain.msixtbl_list); >@@ -569,7 +579,7 @@ void msixtbl_pt_cleanup(struct domain *d) > { > struct msixtbl_entry *entry, *temp; > >-if ( !d->arch.hvm_domain.msixtbl_list.next ) >+if ( !msixtbl_initialised(d) ) > return; > > spin_lock(>event_lock); ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel