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

[Sander Eikelenboom <linux@xxxxxxxxxxxxxx>]

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 <linux@xxxxxxxxxxxxxx>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Sander Eikelenboom <linux@xxxxxxxxxxxxxx>

> 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(&d->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(&d->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(&d->event_lock);


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel