Wu, Feng
2013-Nov-21 01:29 UTC
[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@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/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
Wu, Feng
2013-Nov-21 05:59 UTC
Re: [PATCH v2] x86: properly handle MSI-X unmask operation from guests
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@lists.xen.org > [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of Wu, Feng > Sent: Thursday, November 21, 2013 9:29 AM > To: Jan Beulich (JBeulich@suse.com); xen-devel@lists.xen.org > 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@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/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@lists.xen.org > http://lists.xen.org/xen-devel