Re: [Xen-devel] [PATCH] x86/vMSI-X: Fix host crash when shutting down guests with MSI capable devices

[George Dunlap]

On Thu, Jul 21, 2016 at 11:18 AM, Andrew Cooper
 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.

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

[Sander Eikelenboom]

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 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:

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

[Andrew Cooper]

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

[Andrew Cooper]

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

[Sander Eikelenboom]

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

[Sander Eikelenboom]

On July 21, 2016 12:18:37 PM GMT+02:00, Andrew Cooper 
 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?
>---

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