Re: [Xen-devel] [PATCH v2] x86: properly handle MSI-X unmask operation from guests

["Wu, Feng" <feng.wu@xxxxxxxxx>]

There are some issues in this version, please ignore this one, I will send out 
a new version soon!

> -----Original Message-----
> From: xen-devel-bounces@xxxxxxxxxxxxx
> [mailto:xen-devel-bounces@xxxxxxxxxxxxx] On Behalf Of Wu, Feng
> Sent: Thursday, November 21, 2013 9:29 AM
> To: Jan Beulich (JBeulich@xxxxxxxx); xen-devel@xxxxxxxxxxxxx
> Cc: Auld, Will; Zhang, Xiantao; Nakajima, Jun
> Subject: [Xen-devel] [PATCH v2] x86: properly handle MSI-X unmask operation
> from guests
> 
> patch revision history
> ----------------------
> v1: Initial patch to handle this issue involving changing the hypercall 
> interface
> v2: Totally handled inside hypervisor, need to add a MSI-X specific handler 
> in the
> general I/O emulation path.
> 
> From fb9b39754da87c895a8d00efebd5aa557cfc1216 Mon Sep 17 00:00:00
> 2001
> From: Feng Wu <feng.wu@xxxxxxxxx>
> 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@xxxxxxxxx>
> ---
>  xen/arch/x86/domain.c        |    2 ++
>  xen/arch/x86/hvm/io.c        |    3 +++
>  xen/arch/x86/hvm/vmsi.c      |   40
> +++++++++++++++++++++++++++++++++++++++-
>  xen/include/asm-x86/domain.h |    8 ++++++++
>  xen/include/xen/pci.h        |    1 +
>  5 files changed, 53 insertions(+), 1 deletions(-)
> 
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index b67fcb8..eb3de90 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -374,6 +374,8 @@ int vcpu_initialise(struct vcpu *v)
> 
>      v->arch.flags = TF_kernel_mode;
> 
> +    spin_lock_init(&v->arch.pending_msix_unmask.lock);
> +
>      rc = mapcache_vcpu_init(v);
>      if ( rc )
>      if ( rc )
>          return rc;
> diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
> index 6e344e4..06fcd43 100644
> --- a/xen/arch/x86/hvm/io.c
> +++ b/xen/arch/x86/hvm/io.c
> @@ -288,6 +288,9 @@ void hvm_io_assist(void)
>          break;
>      }
> 
> +    if (msix_post_handler(curr->domain))
> +        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..36f27fc 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) ) {
> +        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,38 @@ 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 domain *d)
> +{
> +    int rc;
> +    unsigned long flags;
> +    struct vcpu *v;
> +
> +    for_each_vcpu(d, v) {
> +
> +        /*
> +         * v->arch.pending_msix_unmask.valid may be checked on other
> cpus
> +         * at the same time, we need to add spinlock to protect it
> +         */
> +
> +        spin_lock_irqsave(&v->arch.pending_msix_unmask.lock, flags);
> +
> +        if (v->arch.pending_msix_unmask.valid == 0) {
> +            spin_unlock_irqrestore(&v->arch.pending_msix_unmask.lock,
> flags);
> +            return 0;
> +        }
> +
> +        v->arch.pending_msix_unmask.valid = 0;
> +
> +        spin_unlock_irqrestore(&v->arch.pending_msix_unmask.lock,
> flags);
> +
> +        rc = msixtbl_write(v, v->arch.pending_msix_unmask.ctrl_address, 4,
> 0);
> +        if (rc != X86EMUL_OKAY)
> +            return -1;
> +        else
> +            return 0;
> +    }
> +
> +    return 0;
> +}
> +
> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> index e42651e..844aadc 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -376,6 +376,12 @@ struct pv_vcpu
>      spinlock_t shadow_ldt_lock;
>  };
> 
> +struct pending_msix_unmask_info {
> +    int valid;
> +    unsigned long ctrl_address;
> +    spinlock_t lock;
> +};
> +
>  struct arch_vcpu
>  {
>      /*
> @@ -440,6 +446,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;
>  } __cacheline_aligned;
> 
>  /* Shorthands to improve code legibility. */
> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
> index cadb525..649b869 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 domain *d);
> 
>  #endif /* __XEN_PCI_H__ */
> --
> 1.7.1
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel

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