Wu, Feng
2013-Nov-26 02:05 UTC
[PATCH v5] 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. v3:Change some logics of handling msi-x pending unmask operations. v4:Some changes related to coding style according to Andrew Cooper''s comments v5:Some changes according to Jan''s comments, including a) remove "valid" field, use "ctrl_address" itself to judge if there is a valid pending msix unmask operation b) Call msix_post_handler() when (p->state == STATE_IOREQ_NONE), which means it gets executed only upon completion of I/O From de586050612f2fbe6fbb46065c2a84069c52009e 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 v5] 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 | 4 ++++ xen/arch/x86/hvm/vmsi.c | 15 +++++++++++++++ xen/include/asm-x86/domain.h | 7 +++++++ xen/include/xen/pci.h | 1 + 4 files changed, 27 insertions(+), 0 deletions(-) diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c index deb7b92..bd751d1 100644 --- a/xen/arch/x86/hvm/io.c +++ b/xen/arch/x86/hvm/io.c @@ -298,7 +298,11 @@ void hvm_io_assist(ioreq_t *p) } if ( p->state == STATE_IOREQ_NONE ) + { + if ( !msix_post_handler(curr) ) + gdprintk(XENLOG_WARNING, "msix_post_handler error\n"); vcpu_end_shutdown_deferral(curr); + } } static int dpci_ioport_read(uint32_t mport, ioreq_t *p) diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c index 4826b4a..7178de9 100644 --- a/xen/arch/x86/hvm/vmsi.c +++ b/xen/arch/x86/hvm/vmsi.c @@ -293,7 +293,10 @@ 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) ) + { + v->arch.pending_msix_unmask.ctrl_address = address; goto out; + } virt = msixtbl_addr_to_virt(entry, address); if ( !virt ) @@ -528,3 +531,15 @@ void msixtbl_pt_cleanup(struct domain *d) spin_unlock(&d->arch.hvm_domain.msixtbl_list_lock); local_irq_restore(flags); } + +bool_t msix_post_handler(struct vcpu *v) +{ + unsigned long ctrl_address = v->arch.pending_msix_unmask.ctrl_address; + + if ( ctrl_address == 0 ) + return 1; + + v->arch.pending_msix_unmask.ctrl_address = 0; + return msixtbl_write(v, ctrl_address, 4, 0) == X86EMUL_OKAY; +} + diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h index 9d39061..8292fdb 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 +{ + unsigned long ctrl_address; +}; + 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; } __cacheline_aligned; /* Shorthands to improve code legibility. */ diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h index cadb525..15050c8 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); +bool_t msix_post_handler(struct vcpu *v); #endif /* __XEN_PCI_H__ */ -- 1.7.1
Jan Beulich
2013-Nov-26 08:48 UTC
Re: [PATCH v5] x86: properly handle MSI-X unmask operation from guests
>>> On 26.11.13 at 03:05, "Wu, Feng" <feng.wu@intel.com> 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. > v4:Some changes related to coding style according to Andrew Cooper''s > comments > v5:Some changes according to Jan''s comments, including > a) remove "valid" field, use "ctrl_address" itself to judge if there is a valid > pending msix unmask operation > b) Call msix_post_handler() when (p->state == STATE_IOREQ_NONE), which > means it gets executed only upon completion of I/OI''m fine with this now, except for some cosmetic things which I would take the liberty of changing on the fly while committing, assuming there aren''t going to be other comments requiring another revision.> --- a/xen/arch/x86/hvm/io.c > +++ b/xen/arch/x86/hvm/io.c > @@ -298,7 +298,11 @@ void hvm_io_assist(ioreq_t *p) > } > > if ( p->state == STATE_IOREQ_NONE ) > + { > + if ( !msix_post_handler(curr) ) > + gdprintk(XENLOG_WARNING, "msix_post_handler error\n");There''s really no point in issuing the warning here rather than in the function itself; the function could therefore return "void". Further, the function name is bogus: It suggests to deal with some "post" operation. I''d rename it to msix_write_completion() or some such.> @@ -528,3 +531,15 @@ void msixtbl_pt_cleanup(struct domain *d) > spin_unlock(&d->arch.hvm_domain.msixtbl_list_lock); > local_irq_restore(flags); > } > + > +bool_t msix_post_handler(struct vcpu *v) > +{ > + unsigned long ctrl_address = v->arch.pending_msix_unmask.ctrl_address; > + > + if ( ctrl_address == 0 ) > + return 1; > + > + v->arch.pending_msix_unmask.ctrl_address = 0; > + return msixtbl_write(v, ctrl_address, 4, 0) == X86EMUL_OKAY; > +} > +Stray blank line.> --- 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 > +{ > + unsigned long ctrl_address; > +};Hardly worth a separate structure. Jan
Wu, Feng
2013-Nov-26 08:58 UTC
Re: [PATCH v5] x86: properly handle MSI-X unmask operation from guests
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Tuesday, November 26, 2013 4:48 PM > To: Wu, Feng > Cc: Nakajima, Jun; Auld, Will; Zhang, Xiantao; xen-devel > Subject: Re: [PATCH v5] x86: properly handle MSI-X unmask operation from > guests > > >>> On 26.11.13 at 03:05, "Wu, Feng" <feng.wu@intel.com> 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. > > v4:Some changes related to coding style according to Andrew Cooper''s > > comments > > v5:Some changes according to Jan''s comments, including > > a) remove "valid" field, use "ctrl_address" itself to judge if there is a valid > > pending msix unmask operation > > b) Call msix_post_handler() when (p->state == STATE_IOREQ_NONE), > which > > means it gets executed only upon completion of I/O > > I''m fine with this now, except for some cosmetic things which I > would take the liberty of changing on the fly while committing, > assuming there aren''t going to be other comments requiring > another revision.Jan, thank you very much for your review of this patch! Also thank Andrew!> > > --- a/xen/arch/x86/hvm/io.c > > +++ b/xen/arch/x86/hvm/io.c > > @@ -298,7 +298,11 @@ void hvm_io_assist(ioreq_t *p) > > } > > > > if ( p->state == STATE_IOREQ_NONE ) > > + { > > + if ( !msix_post_handler(curr) ) > > + gdprintk(XENLOG_WARNING, "msix_post_handler error\n"); > > There''s really no point in issuing the warning here rather than in > the function itself; the function could therefore return "void". > > Further, the function name is bogus: It suggests to deal with > some "post" operation. I''d rename it to msix_write_completion() > or some such. > > > @@ -528,3 +531,15 @@ void msixtbl_pt_cleanup(struct domain *d) > > spin_unlock(&d->arch.hvm_domain.msixtbl_list_lock); > > local_irq_restore(flags); > > } > > + > > +bool_t msix_post_handler(struct vcpu *v) > > +{ > > + unsigned long ctrl_address > v->arch.pending_msix_unmask.ctrl_address; > > + > > + if ( ctrl_address == 0 ) > > + return 1; > > + > > + v->arch.pending_msix_unmask.ctrl_address = 0; > > + return msixtbl_write(v, ctrl_address, 4, 0) == X86EMUL_OKAY; > > +} > > + > > Stray blank line. > > > --- 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 > > +{ > > + unsigned long ctrl_address; > > +}; > > Hardly worth a separate structure. > > Jan
Jan Beulich
2013-Nov-26 09:31 UTC
[PATCH v6] 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> Only latch the address if the guest really is unmasking the entry. Clean up the entire change. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/hvm/io.c +++ b/xen/arch/x86/hvm/io.c @@ -300,7 +300,10 @@ void hvm_io_assist(ioreq_t *p) } if ( p->state == STATE_IOREQ_NONE ) + { + msix_write_completion(curr); vcpu_end_shutdown_deferral(curr); + } } static int dpci_ioport_read(uint32_t mport, ioreq_t *p) --- a/xen/arch/x86/hvm/vmsi.c +++ b/xen/arch/x86/hvm/vmsi.c @@ -293,7 +293,11 @@ static int msixtbl_write(struct vcpu *v, /* exit to device model if address/data has been modified */ if ( test_and_clear_bit(nr_entry, &entry->table_flags) ) + { + if ( !(val & PCI_MSIX_VECTOR_BITMASK) ) + v->arch.hvm_vcpu.hvm_io.msix_unmask_address = address; goto out; + } virt = msixtbl_addr_to_virt(entry, address); if ( !virt ) @@ -528,3 +532,15 @@ void msixtbl_pt_cleanup(struct domain *d spin_unlock(&d->arch.hvm_domain.msixtbl_list_lock); local_irq_restore(flags); } + +void msix_write_completion(struct vcpu *v) +{ + unsigned long ctrl_address = v->arch.hvm_vcpu.hvm_io.msix_unmask_address; + + if ( !ctrl_address ) + return; + + v->arch.hvm_vcpu.hvm_io.msix_unmask_address = 0; + if ( msixtbl_write(v, ctrl_address, 4, 0) != X86EMUL_OKAY ) + gdprintk(XENLOG_WARNING, "MSI-X write completion failure\n"); +} --- a/xen/include/asm-x86/hvm/vcpu.h +++ b/xen/include/asm-x86/hvm/vcpu.h @@ -74,6 +74,8 @@ struct hvm_vcpu_io { * necessary retry through other than function return codes. */ bool_t mmio_retry, mmio_retrying; + + unsigned long msix_unmask_address; }; #define VMCX_EADDR (~0ULL) --- a/xen/include/asm-x86/hvm/io.h +++ b/xen/include/asm-x86/hvm/io.h @@ -124,6 +124,7 @@ void hvm_interrupt_post(struct vcpu *v, void hvm_io_assist(ioreq_t *p); void hvm_dpci_eoi(struct domain *d, unsigned int guest_irq, union vioapic_redir_entry *ent); +void msix_write_completion(struct vcpu *); struct hvm_hw_stdvga { uint8_t sr_index; _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Jan Beulich
2013-Nov-26 09:34 UTC
Re: [PATCH v5] x86: properly handle MSI-X unmask operation from guests
>>> On 26.11.13 at 09:58, "Wu, Feng" <feng.wu@intel.com> wrote: >> I''m fine with this now, except for some cosmetic things which I >> would take the liberty of changing on the fly while committing, >> assuming there aren''t going to be other comments requiring >> another revision. > > Jan, thank you very much for your review of this patch!Actually, as you may have seen already, I just posted v6, which - apart from having all the intended cleanup - fixes a bug I just spotted. Jan
Wu, Feng
2013-Nov-26 09:53 UTC
Re: [PATCH v5] x86: properly handle MSI-X unmask operation from guests
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Tuesday, November 26, 2013 5:35 PM > To: Wu, Feng > Cc: Andrew Cooper (andrew.cooper3@citrix.com); Nakajima, Jun; Auld, Will; > Zhang, Xiantao; xen-devel > Subject: RE: [PATCH v5] x86: properly handle MSI-X unmask operation from > guests > > >>> On 26.11.13 at 09:58, "Wu, Feng" <feng.wu@intel.com> wrote: > >> I''m fine with this now, except for some cosmetic things which I > >> would take the liberty of changing on the fly while committing, > >> assuming there aren''t going to be other comments requiring > >> another revision. > > > > Jan, thank you very much for your review of this patch! > > Actually, as you may have seen already, I just posted v6, > which - apart from having all the intended cleanup - fixes a > bug I just spotted.Yes, I see your changes, the check is needed. Normally OS follows the sequences below to operate interrupt: 1) mask 2) change msix data / address 3) unmask In this case, patch v5 works fine. However, If guest operate it like this way: 1) mask 2) change msix data / address 3) mask 4) change msix data /address again 3) unmask Patch v5 will latch the address the step #3, which will cause problem. Thanks a lot for this finding.> > Jan
Andrew Cooper
2013-Nov-26 10:01 UTC
Re: [PATCH v6] x86: properly handle MSI-X unmask operation from guests
On 26/11/13 09:31, Jan Beulich wrote:> 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> > > Only latch the address if the guest really is unmasking the entry. > > Clean up the entire change. > > Signed-off-by: Jan Beulich <jbeulich@suse.com>Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>> > --- a/xen/arch/x86/hvm/io.c > +++ b/xen/arch/x86/hvm/io.c > @@ -300,7 +300,10 @@ void hvm_io_assist(ioreq_t *p) > } > > if ( p->state == STATE_IOREQ_NONE ) > + { > + msix_write_completion(curr); > vcpu_end_shutdown_deferral(curr); > + } > } > > static int dpci_ioport_read(uint32_t mport, ioreq_t *p) > --- a/xen/arch/x86/hvm/vmsi.c > +++ b/xen/arch/x86/hvm/vmsi.c > @@ -293,7 +293,11 @@ static int msixtbl_write(struct vcpu *v, > > /* exit to device model if address/data has been modified */ > if ( test_and_clear_bit(nr_entry, &entry->table_flags) ) > + { > + if ( !(val & PCI_MSIX_VECTOR_BITMASK) ) > + v->arch.hvm_vcpu.hvm_io.msix_unmask_address = address; > goto out; > + } > > virt = msixtbl_addr_to_virt(entry, address); > if ( !virt ) > @@ -528,3 +532,15 @@ void msixtbl_pt_cleanup(struct domain *d > spin_unlock(&d->arch.hvm_domain.msixtbl_list_lock); > local_irq_restore(flags); > } > + > +void msix_write_completion(struct vcpu *v) > +{ > + unsigned long ctrl_address = v->arch.hvm_vcpu.hvm_io.msix_unmask_address; > + > + if ( !ctrl_address ) > + return; > + > + v->arch.hvm_vcpu.hvm_io.msix_unmask_address = 0; > + if ( msixtbl_write(v, ctrl_address, 4, 0) != X86EMUL_OKAY ) > + gdprintk(XENLOG_WARNING, "MSI-X write completion failure\n"); > +} > --- a/xen/include/asm-x86/hvm/vcpu.h > +++ b/xen/include/asm-x86/hvm/vcpu.h > @@ -74,6 +74,8 @@ struct hvm_vcpu_io { > * necessary retry through other than function return codes. > */ > bool_t mmio_retry, mmio_retrying; > + > + unsigned long msix_unmask_address; > }; > > #define VMCX_EADDR (~0ULL) > --- a/xen/include/asm-x86/hvm/io.h > +++ b/xen/include/asm-x86/hvm/io.h > @@ -124,6 +124,7 @@ void hvm_interrupt_post(struct vcpu *v, > void hvm_io_assist(ioreq_t *p); > void hvm_dpci_eoi(struct domain *d, unsigned int guest_irq, > union vioapic_redir_entry *ent); > +void msix_write_completion(struct vcpu *); > > struct hvm_hw_stdvga { > uint8_t sr_index; > > >
Reasonably Related Threads
- [PATCH v4] x86: properly handle MSI-X unmask operation from guests
- [PATCH] x86/vtsc: update vcpu_time after hvm_set_guest_time
- [PATCH 00/15] RFC xen device model support
- ioemu: make various functions in i386-dm/helper2.c static
- [PATCH][2/10] Extend the VMX intercept mechanism to include mmio as well as portio.