Re: [PATCH v3] x86: properly handle MSI-X unmask operation from guests

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 21/11/13 10:51, Wu, Feng wrote:
> patch revision history
> ----------------------
> v1: Initial patch to handle this issue involving changing the hypercall interface
> v2:Totally handled inside hypervisor.
> v3:Change some logics of handling msi-x pending unmask operations.
>
> From 78ae225e6af88b0b850980fc55640d0776aeafbc Mon Sep 17 00:00:00 2001
> From: Feng Wu <feng.wu@intel.com>
> Date: Wed, 13 Nov 2013 21:43:48 -0500
> Subject: [PATCH] x86: properly handle MSI-X unmask operation from guests
>
> For a pass-through device with MSI-x capability, when guest tries
> to unmask the MSI-x interrupt for the passed through device, xen
> doesn't clear the mask bit for MSI-x in hardware in the following
> scenario, which will cause network disconnection:
>
> 1. Guest masks the MSI-x interrupt
> 2. Guest updates the address and data for it
> 3. Guest unmasks the MSI-x interrupt (This is the problematic step)
>
> In the step #3 above, Xen doesn't handle it well. When guest tries
> to unmask MSI-X interrupt, it traps to Xen, Xen just returns to Qemu
> if it notices that address or data has been modified by guest before,
> then Qemu will update Xen with the latest value of address/data by
> hypercall. However, in this whole process, the MSI-X interrupt unmask
> operation is missing, which means Xen doesn't clear the mask bit in
> hardware for the MSI-X interrupt, so it remains disabled, that is why
> it loses the network connection.
>
> This patch fixes this issue.
>
> Signed-off-by: Feng Wu <feng.wu@intel.com>
> ---
>  xen/arch/x86/hvm/io.c        |    3 +++
>  xen/arch/x86/hvm/vmsi.c      |   22 +++++++++++++++++++++-
>  xen/include/asm-x86/domain.h |    7 +++++++
>  xen/include/xen/pci.h        |    1 +
>  4 files changed, 32 insertions(+), 1 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
> index deb7b92..516f0a4 100644
> --- a/xen/arch/x86/hvm/io.c
> +++ b/xen/arch/x86/hvm/io.c
> @@ -297,6 +297,9 @@ void hvm_io_assist(ioreq_t *p)
>          break;
>      }
>
> +    if (msix_post_handler(curr))

Spaces inside the brackets

> +        gdprintk(XENLOG_ERR, ": msix_post_handler error\n");
> +
>      if ( p->state == STATE_IOREQ_NONE )
>          vcpu_end_shutdown_deferral(curr);
> }
> diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
> index 4826b4a..cd97a3b 100644
> --- a/xen/arch/x86/hvm/vmsi.c
> +++ b/xen/arch/x86/hvm/vmsi.c
> @@ -292,8 +292,11 @@ static int msixtbl_write(struct vcpu *v, unsigned long address,
>      }
>
>      /* exit to device model if address/data has been modified */
> -    if ( test_and_clear_bit(nr_entry, &entry->table_flags) )
> +    if ( test_and_clear_bit(nr_entry, &entry->table_flags) ) {

Braces should line up

> +        v->arch.pending_msix_unmask.valid = 1;
> +        v->arch.pending_msix_unmask.ctrl_address = address;
>          goto out;
> +    }
>
>      virt = msixtbl_addr_to_virt(entry, address);
>      if ( !virt )
> @@ -528,3 +531,20 @@ void msixtbl_pt_cleanup(struct domain *d)
>      spin_unlock(&d->arch.hvm_domain.msixtbl_list_lock);
>      local_irq_restore(flags);
>  }
> +
> +int msix_post_handler(struct vcpu *v)
> +{
> +    int rc;
> +
> +    if (v->arch.pending_msix_unmask.valid == 0)

spaces

> +        return 0;
> +
> +    v->arch.pending_msix_unmask.valid = 0;
> +
> +    rc = msixtbl_write(v, v->arch.pending_msix_unmask.ctrl_address, 4, 0);
> +    if (rc != X86EMUL_OKAY)
> +        return -1;
> +    else
> +        return 0;

return rc != X86EMUL_OKAY ? -1 : 0;

> +}
> +
> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> index 9d39061..b3bdfa3 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -375,6 +375,11 @@ struct pv_vcpu
>      spinlock_t shadow_ldt_lock;
>  };
>
> +struct pending_msix_unmask_info {
> +    int valid;
> +    unsigned long ctrl_address;

valid should be boolean, and reeordered for packing purposes.

> +};
> +
>  struct arch_vcpu
>  {
>      /*
> @@ -439,6 +444,8 @@ struct arch_vcpu
>
>      /* A secondary copy of the vcpu time info. */
>      XEN_GUEST_HANDLE(vcpu_time_info_t) time_info_guest;
> +
> +    struct pending_msix_unmask_info pending_msix_unmask;

What happens if multiple msix interrupts are masked, all updated with
addresses, then all unmasked?

~Andrew

>  } __cacheline_aligned;
>
>  /* Shorthands to improve code legibility. */
> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
> index cadb525..ce8f6ff 100644
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -147,5 +147,6 @@ struct pirq;
>  int msixtbl_pt_register(struct domain *, struct pirq *, uint64_t gtable);
>  void msixtbl_pt_unregister(struct domain *, struct pirq *);
>  void msixtbl_pt_cleanup(struct domain *d);
> +int msix_post_handler(struct vcpu *v);
>
>  #endif /* __XEN_PCI_H__ */
> --
> 1.7.1
>
> Thanks,
> Feng
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel