* [PATCH 0/2] vpci/msi: fix updating already bound MSI interrupts @ 2018-05-08 9:23 Roger Pau Monne 2018-05-08 9:25 ` [PATCH 1/2] vpci/msi: split code to bind pirq Roger Pau Monne ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Roger Pau Monne @ 2018-05-08 9:23 UTC (permalink / raw) To: xen-devel; +Cc: Roger Pau Monne Hello, There's a bug in current vpci code for MSI emulation when updating an already bound interrupt. The code will disable and enable the interrupt in order to update the binding, which calls unmap_domain_pirq that disables the global MSI enable flag in the control register. In order to fix this incorrect behavior introduce a new update helper that should be used to update the bindings of an already enabled group of MSI interrupts. Thanks, Roger. Roger Pau Monne (2): vpci/msi: split code to bind pirq vpci/msi: fix update of bound MSI interrupts xen/arch/x86/hvm/vmsi.c | 96 +++++++++++++++++++++++++++++------------ xen/drivers/vpci/msi.c | 3 +- xen/include/xen/vpci.h | 2 + 3 files changed, 71 insertions(+), 30 deletions(-) -- 2.17.0 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] vpci/msi: split code to bind pirq 2018-05-08 9:23 [PATCH 0/2] vpci/msi: fix updating already bound MSI interrupts Roger Pau Monne @ 2018-05-08 9:25 ` Roger Pau Monne 2018-05-14 12:24 ` Jan Beulich 2018-05-08 9:25 ` [PATCH 2/2] vpci/msi: fix update of bound MSI interrupts Roger Pau Monne 2018-05-08 9:30 ` [PATCH 0/2] vpci/msi: fix updating already " Juergen Gross 2 siblings, 1 reply; 13+ messages in thread From: Roger Pau Monne @ 2018-05-08 9:25 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monne And put it in a separate update function. This is required in order to improve binding of MSI PIRQs when using vPCI. No functional change. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> --- xen/arch/x86/hvm/vmsi.c | 73 +++++++++++++++++++++++++---------------- 1 file changed, 45 insertions(+), 28 deletions(-) diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c index 900d4f67d4..6e19851439 100644 --- a/xen/arch/x86/hvm/vmsi.c +++ b/xen/arch/x86/hvm/vmsi.c @@ -663,6 +663,42 @@ void vpci_msi_arch_mask(struct vpci_msi *msi, const struct pci_dev *pdev, vpci_mask_pirq(pdev->domain, msi->arch.pirq + entry, mask); } +static int vpci_msi_update(const struct pci_dev *pdev, uint32_t data, + uint64_t address, unsigned int vectors, + unsigned int pirq, uint32_t mask) +{ + unsigned int i; + + ASSERT(pcidevs_locked()); + + for ( i = 0; i < vectors; i++ ) + { + uint8_t vector = MASK_EXTR(data, MSI_DATA_VECTOR_MASK); + uint8_t vector_mask = 0xff >> (8 - fls(vectors) + 1); + struct xen_domctl_bind_pt_irq bind = { + .machine_irq = pirq + i, + .irq_type = PT_IRQ_TYPE_MSI, + .u.msi.gvec = (vector & ~vector_mask) | + ((vector + i) & vector_mask), + .u.msi.gflags = msi_gflags(data, address, (mask >> i) & 1), + }; + int rc = pt_irq_create_bind(pdev->domain, &bind); + + if ( rc ) + { + gdprintk(XENLOG_ERR, + "%04x:%02x:%02x.%u: failed to bind PIRQ %u: %d\n", + pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn), + PCI_FUNC(pdev->devfn), pirq + i, rc); + while ( bind.machine_irq-- ) + pt_irq_destroy_bind(pdev->domain, &bind); + return rc; + } + } + + return 0; +} + static int vpci_msi_enable(const struct pci_dev *pdev, uint32_t data, uint64_t address, unsigned int nr, paddr_t table_base, uint32_t mask) @@ -674,7 +710,7 @@ static int vpci_msi_enable(const struct pci_dev *pdev, uint32_t data, .table_base = table_base, .entry_nr = nr, }; - unsigned int i, vectors = table_base ? 1 : nr; + unsigned vectors = table_base ? 1 : nr; int rc, pirq = INVALID_PIRQ; /* Get a PIRQ. */ @@ -690,36 +726,17 @@ static int vpci_msi_enable(const struct pci_dev *pdev, uint32_t data, return rc; } - for ( i = 0; i < vectors; i++ ) + pcidevs_lock(); + rc = vpci_msi_update(pdev, data, address, vectors, pirq, mask); + if ( rc ) { - uint8_t vector = MASK_EXTR(data, MSI_DATA_VECTOR_MASK); - uint8_t vector_mask = 0xff >> (8 - fls(vectors) + 1); - struct xen_domctl_bind_pt_irq bind = { - .machine_irq = pirq + i, - .irq_type = PT_IRQ_TYPE_MSI, - .u.msi.gvec = (vector & ~vector_mask) | - ((vector + i) & vector_mask), - .u.msi.gflags = msi_gflags(data, address, (mask >> i) & 1), - }; - - pcidevs_lock(); - rc = pt_irq_create_bind(pdev->domain, &bind); - if ( rc ) - { - gdprintk(XENLOG_ERR, - "%04x:%02x:%02x.%u: failed to bind PIRQ %u: %d\n", - pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn), - PCI_FUNC(pdev->devfn), pirq + i, rc); - while ( bind.machine_irq-- ) - pt_irq_destroy_bind(pdev->domain, &bind); - spin_lock(&pdev->domain->event_lock); - unmap_domain_pirq(pdev->domain, pirq); - spin_unlock(&pdev->domain->event_lock); - pcidevs_unlock(); - return rc; - } + spin_lock(&pdev->domain->event_lock); + unmap_domain_pirq(pdev->domain, pirq); + spin_unlock(&pdev->domain->event_lock); pcidevs_unlock(); + return rc; } + pcidevs_unlock(); return pirq; } -- 2.17.0 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] vpci/msi: split code to bind pirq 2018-05-08 9:25 ` [PATCH 1/2] vpci/msi: split code to bind pirq Roger Pau Monne @ 2018-05-14 12:24 ` Jan Beulich 2018-05-14 14:15 ` Roger Pau Monné 0 siblings, 1 reply; 13+ messages in thread From: Jan Beulich @ 2018-05-14 12:24 UTC (permalink / raw) To: Roger Pau Monne; +Cc: Andrew Cooper, xen-devel >>> On 08.05.18 at 11:25, <roger.pau@citrix.com> wrote: > --- a/xen/arch/x86/hvm/vmsi.c > +++ b/xen/arch/x86/hvm/vmsi.c > @@ -663,6 +663,42 @@ void vpci_msi_arch_mask(struct vpci_msi *msi, const struct pci_dev *pdev, > vpci_mask_pirq(pdev->domain, msi->arch.pirq + entry, mask); > } > > +static int vpci_msi_update(const struct pci_dev *pdev, uint32_t data, > + uint64_t address, unsigned int vectors, > + unsigned int pirq, uint32_t mask) > +{ > + unsigned int i; > + > + ASSERT(pcidevs_locked()); > + > + for ( i = 0; i < vectors; i++ ) > + { > + uint8_t vector = MASK_EXTR(data, MSI_DATA_VECTOR_MASK); > + uint8_t vector_mask = 0xff >> (8 - fls(vectors) + 1); > + struct xen_domctl_bind_pt_irq bind = { > + .machine_irq = pirq + i, > + .irq_type = PT_IRQ_TYPE_MSI, > + .u.msi.gvec = (vector & ~vector_mask) | > + ((vector + i) & vector_mask), > + .u.msi.gflags = msi_gflags(data, address, (mask >> i) & 1), > + }; > + int rc = pt_irq_create_bind(pdev->domain, &bind); > + > + if ( rc ) > + { > + gdprintk(XENLOG_ERR, > + "%04x:%02x:%02x.%u: failed to bind PIRQ %u: %d\n", > + pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn), > + PCI_FUNC(pdev->devfn), pirq + i, rc); > + while ( bind.machine_irq-- ) > + pt_irq_destroy_bind(pdev->domain, &bind); I realize this is just code movement, but is this while() correct? I think it can only be correct if pirq (which bind.machine_irq gets initialized from) was always zero, yet that doesn't look to be the case. If you agree, I'd prefer fixed code to be moved (read: wants a prereq patch), or for the fix to be applied while moving the code (suitably reasoned about in the description). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] vpci/msi: split code to bind pirq 2018-05-14 12:24 ` Jan Beulich @ 2018-05-14 14:15 ` Roger Pau Monné 2018-05-14 14:56 ` Jan Beulich 0 siblings, 1 reply; 13+ messages in thread From: Roger Pau Monné @ 2018-05-14 14:15 UTC (permalink / raw) To: Jan Beulich; +Cc: Andrew Cooper, xen-devel On Mon, May 14, 2018 at 06:24:37AM -0600, Jan Beulich wrote: > >>> On 08.05.18 at 11:25, <roger.pau@citrix.com> wrote: > > --- a/xen/arch/x86/hvm/vmsi.c > > +++ b/xen/arch/x86/hvm/vmsi.c > > @@ -663,6 +663,42 @@ void vpci_msi_arch_mask(struct vpci_msi *msi, const struct pci_dev *pdev, > > vpci_mask_pirq(pdev->domain, msi->arch.pirq + entry, mask); > > } > > > > +static int vpci_msi_update(const struct pci_dev *pdev, uint32_t data, > > + uint64_t address, unsigned int vectors, > > + unsigned int pirq, uint32_t mask) > > +{ > > + unsigned int i; > > + > > + ASSERT(pcidevs_locked()); > > + > > + for ( i = 0; i < vectors; i++ ) > > + { > > + uint8_t vector = MASK_EXTR(data, MSI_DATA_VECTOR_MASK); > > + uint8_t vector_mask = 0xff >> (8 - fls(vectors) + 1); > > + struct xen_domctl_bind_pt_irq bind = { > > + .machine_irq = pirq + i, > > + .irq_type = PT_IRQ_TYPE_MSI, > > + .u.msi.gvec = (vector & ~vector_mask) | > > + ((vector + i) & vector_mask), > > + .u.msi.gflags = msi_gflags(data, address, (mask >> i) & 1), > > + }; > > + int rc = pt_irq_create_bind(pdev->domain, &bind); > > + > > + if ( rc ) > > + { > > + gdprintk(XENLOG_ERR, > > + "%04x:%02x:%02x.%u: failed to bind PIRQ %u: %d\n", > > + pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn), > > + PCI_FUNC(pdev->devfn), pirq + i, rc); > > + while ( bind.machine_irq-- ) > > + pt_irq_destroy_bind(pdev->domain, &bind); > > I realize this is just code movement, but is this while() correct? I think it > can only be correct if pirq (which bind.machine_irq gets initialized from) > was always zero, yet that doesn't look to be the case. > > If you agree, I'd prefer fixed code to be moved (read: wants a prereq > patch), or for the fix to be applied while moving the code (suitably > reasoned about in the description). Right, this should be: while ( bind.machine_irq-- >= pirq ) pt_irq_destroy_bind(pdev->domain, &bind); Will fix before moving the code. Thanks, Roger. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] vpci/msi: split code to bind pirq 2018-05-14 14:15 ` Roger Pau Monné @ 2018-05-14 14:56 ` Jan Beulich 2018-05-14 15:00 ` Roger Pau Monné 0 siblings, 1 reply; 13+ messages in thread From: Jan Beulich @ 2018-05-14 14:56 UTC (permalink / raw) To: Roger Pau Monne; +Cc: Andrew Cooper, xen-devel >>> On 14.05.18 at 16:15, <roger.pau@citrix.com> wrote: > On Mon, May 14, 2018 at 06:24:37AM -0600, Jan Beulich wrote: >> >>> On 08.05.18 at 11:25, <roger.pau@citrix.com> wrote: >> > --- a/xen/arch/x86/hvm/vmsi.c >> > +++ b/xen/arch/x86/hvm/vmsi.c >> > @@ -663,6 +663,42 @@ void vpci_msi_arch_mask(struct vpci_msi *msi, const > struct pci_dev *pdev, >> > vpci_mask_pirq(pdev->domain, msi->arch.pirq + entry, mask); >> > } >> > >> > +static int vpci_msi_update(const struct pci_dev *pdev, uint32_t data, >> > + uint64_t address, unsigned int vectors, >> > + unsigned int pirq, uint32_t mask) >> > +{ >> > + unsigned int i; >> > + >> > + ASSERT(pcidevs_locked()); >> > + >> > + for ( i = 0; i < vectors; i++ ) >> > + { >> > + uint8_t vector = MASK_EXTR(data, MSI_DATA_VECTOR_MASK); >> > + uint8_t vector_mask = 0xff >> (8 - fls(vectors) + 1); >> > + struct xen_domctl_bind_pt_irq bind = { >> > + .machine_irq = pirq + i, >> > + .irq_type = PT_IRQ_TYPE_MSI, >> > + .u.msi.gvec = (vector & ~vector_mask) | >> > + ((vector + i) & vector_mask), >> > + .u.msi.gflags = msi_gflags(data, address, (mask >> i) & 1), >> > + }; >> > + int rc = pt_irq_create_bind(pdev->domain, &bind); >> > + >> > + if ( rc ) >> > + { >> > + gdprintk(XENLOG_ERR, >> > + "%04x:%02x:%02x.%u: failed to bind PIRQ %u: %d\n", >> > + pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn), >> > + PCI_FUNC(pdev->devfn), pirq + i, rc); >> > + while ( bind.machine_irq-- ) >> > + pt_irq_destroy_bind(pdev->domain, &bind); >> >> I realize this is just code movement, but is this while() correct? I think > it >> can only be correct if pirq (which bind.machine_irq gets initialized from) >> was always zero, yet that doesn't look to be the case. >> >> If you agree, I'd prefer fixed code to be moved (read: wants a prereq >> patch), or for the fix to be applied while moving the code (suitably >> reasoned about in the description). > > Right, this should be: > > while ( bind.machine_irq-- >= pirq ) > pt_irq_destroy_bind(pdev->domain, &bind); ">" you presumably mean, due to the post-decrement? Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] vpci/msi: split code to bind pirq 2018-05-14 14:56 ` Jan Beulich @ 2018-05-14 15:00 ` Roger Pau Monné 2018-05-14 15:07 ` Jan Beulich 0 siblings, 1 reply; 13+ messages in thread From: Roger Pau Monné @ 2018-05-14 15:00 UTC (permalink / raw) To: Jan Beulich; +Cc: Andrew Cooper, xen-devel On Mon, May 14, 2018 at 08:56:16AM -0600, Jan Beulich wrote: > >>> On 14.05.18 at 16:15, <roger.pau@citrix.com> wrote: > > On Mon, May 14, 2018 at 06:24:37AM -0600, Jan Beulich wrote: > >> >>> On 08.05.18 at 11:25, <roger.pau@citrix.com> wrote: > >> > --- a/xen/arch/x86/hvm/vmsi.c > >> > +++ b/xen/arch/x86/hvm/vmsi.c > >> > @@ -663,6 +663,42 @@ void vpci_msi_arch_mask(struct vpci_msi *msi, const > > struct pci_dev *pdev, > >> > vpci_mask_pirq(pdev->domain, msi->arch.pirq + entry, mask); > >> > } > >> > > >> > +static int vpci_msi_update(const struct pci_dev *pdev, uint32_t data, > >> > + uint64_t address, unsigned int vectors, > >> > + unsigned int pirq, uint32_t mask) > >> > +{ > >> > + unsigned int i; > >> > + > >> > + ASSERT(pcidevs_locked()); > >> > + > >> > + for ( i = 0; i < vectors; i++ ) > >> > + { > >> > + uint8_t vector = MASK_EXTR(data, MSI_DATA_VECTOR_MASK); > >> > + uint8_t vector_mask = 0xff >> (8 - fls(vectors) + 1); > >> > + struct xen_domctl_bind_pt_irq bind = { > >> > + .machine_irq = pirq + i, > >> > + .irq_type = PT_IRQ_TYPE_MSI, > >> > + .u.msi.gvec = (vector & ~vector_mask) | > >> > + ((vector + i) & vector_mask), > >> > + .u.msi.gflags = msi_gflags(data, address, (mask >> i) & 1), > >> > + }; > >> > + int rc = pt_irq_create_bind(pdev->domain, &bind); > >> > + > >> > + if ( rc ) > >> > + { > >> > + gdprintk(XENLOG_ERR, > >> > + "%04x:%02x:%02x.%u: failed to bind PIRQ %u: %d\n", > >> > + pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn), > >> > + PCI_FUNC(pdev->devfn), pirq + i, rc); > >> > + while ( bind.machine_irq-- ) > >> > + pt_irq_destroy_bind(pdev->domain, &bind); > >> > >> I realize this is just code movement, but is this while() correct? I think > > it > >> can only be correct if pirq (which bind.machine_irq gets initialized from) > >> was always zero, yet that doesn't look to be the case. > >> > >> If you agree, I'd prefer fixed code to be moved (read: wants a prereq > >> patch), or for the fix to be applied while moving the code (suitably > >> reasoned about in the description). > > > > Right, this should be: > > > > while ( bind.machine_irq-- >= pirq ) > > pt_irq_destroy_bind(pdev->domain, &bind); > > ">" you presumably mean, due to the post-decrement? Ended up doing --bind.machine_irq >= pirq, because it seemed clearer IMO. Roger. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] vpci/msi: split code to bind pirq 2018-05-14 15:00 ` Roger Pau Monné @ 2018-05-14 15:07 ` Jan Beulich 0 siblings, 0 replies; 13+ messages in thread From: Jan Beulich @ 2018-05-14 15:07 UTC (permalink / raw) To: Roger Pau Monne; +Cc: Andrew Cooper, xen-devel >>> On 14.05.18 at 17:00, <roger.pau@citrix.com> wrote: > On Mon, May 14, 2018 at 08:56:16AM -0600, Jan Beulich wrote: >> >>> On 14.05.18 at 16:15, <roger.pau@citrix.com> wrote: >> > On Mon, May 14, 2018 at 06:24:37AM -0600, Jan Beulich wrote: >> >> >>> On 08.05.18 at 11:25, <roger.pau@citrix.com> wrote: >> >> > --- a/xen/arch/x86/hvm/vmsi.c >> >> > +++ b/xen/arch/x86/hvm/vmsi.c >> >> > @@ -663,6 +663,42 @@ void vpci_msi_arch_mask(struct vpci_msi *msi, const >> > struct pci_dev *pdev, >> >> > vpci_mask_pirq(pdev->domain, msi->arch.pirq + entry, mask); >> >> > } >> >> > >> >> > +static int vpci_msi_update(const struct pci_dev *pdev, uint32_t data, >> >> > + uint64_t address, unsigned int vectors, >> >> > + unsigned int pirq, uint32_t mask) >> >> > +{ >> >> > + unsigned int i; >> >> > + >> >> > + ASSERT(pcidevs_locked()); >> >> > + >> >> > + for ( i = 0; i < vectors; i++ ) >> >> > + { >> >> > + uint8_t vector = MASK_EXTR(data, MSI_DATA_VECTOR_MASK); >> >> > + uint8_t vector_mask = 0xff >> (8 - fls(vectors) + 1); >> >> > + struct xen_domctl_bind_pt_irq bind = { >> >> > + .machine_irq = pirq + i, >> >> > + .irq_type = PT_IRQ_TYPE_MSI, >> >> > + .u.msi.gvec = (vector & ~vector_mask) | >> >> > + ((vector + i) & vector_mask), >> >> > + .u.msi.gflags = msi_gflags(data, address, (mask >> i) & 1), >> >> > + }; >> >> > + int rc = pt_irq_create_bind(pdev->domain, &bind); >> >> > + >> >> > + if ( rc ) >> >> > + { >> >> > + gdprintk(XENLOG_ERR, >> >> > + "%04x:%02x:%02x.%u: failed to bind PIRQ %u: %d\n", >> >> > + pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn), >> >> > + PCI_FUNC(pdev->devfn), pirq + i, rc); >> >> > + while ( bind.machine_irq-- ) >> >> > + pt_irq_destroy_bind(pdev->domain, &bind); >> >> >> >> I realize this is just code movement, but is this while() correct? I think >> > it >> >> can only be correct if pirq (which bind.machine_irq gets initialized from) >> >> was always zero, yet that doesn't look to be the case. >> >> >> >> If you agree, I'd prefer fixed code to be moved (read: wants a prereq >> >> patch), or for the fix to be applied while moving the code (suitably >> >> reasoned about in the description). >> > >> > Right, this should be: >> > >> > while ( bind.machine_irq-- >= pirq ) >> > pt_irq_destroy_bind(pdev->domain, &bind); >> >> ">" you presumably mean, due to the post-decrement? > > Ended up doing --bind.machine_irq >= pirq, because it seemed clearer > IMO. Please don't: Even if in practice pirq can't be zero (I think), your variant would degenerate into an infinite loop in that case. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/2] vpci/msi: fix update of bound MSI interrupts 2018-05-08 9:23 [PATCH 0/2] vpci/msi: fix updating already bound MSI interrupts Roger Pau Monne 2018-05-08 9:25 ` [PATCH 1/2] vpci/msi: split code to bind pirq Roger Pau Monne @ 2018-05-08 9:25 ` Roger Pau Monne 2018-05-14 12:29 ` Jan Beulich 2018-05-08 9:30 ` [PATCH 0/2] vpci/msi: fix updating already " Juergen Gross 2 siblings, 1 reply; 13+ messages in thread From: Roger Pau Monne @ 2018-05-08 9:25 UTC (permalink / raw) To: xen-devel Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Jan Beulich, Roger Pau Monne Current update process of already bound MSI interrupts is wrong because unmap_domain_pirq calls pci_disable_msi, which disables MSI interrupts on the device. On the other hand map_domain_pirq doesn't enable MSI, so the current update process of already enabled MSI entries is wrong because MSI control bit will be disabled by unmap_domain_pirq and not re-enabled by map_domain_pirq. In order to fix this avoid unmapping the PIRQs and just update the binding of the PIRQ. A new arch helper to do that is introduced. Note that MSI-X is not affected because unmap_domain_pirq only disables the MSI enable control bit for the MSI case, for MSI-X the bit is left untouched by unmap_domain_pirq. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: George Dunlap <George.Dunlap@eu.citrix.com> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Julien Grall <julien.grall@arm.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Tim Deegan <tim@xen.org> Cc: Wei Liu <wei.liu2@citrix.com> --- xen/arch/x86/hvm/vmsi.c | 23 +++++++++++++++++++++++ xen/drivers/vpci/msi.c | 3 +-- xen/include/xen/vpci.h | 2 ++ 3 files changed, 26 insertions(+), 2 deletions(-) diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c index 6e19851439..8f9f84a6f3 100644 --- a/xen/arch/x86/hvm/vmsi.c +++ b/xen/arch/x86/hvm/vmsi.c @@ -699,6 +699,29 @@ static int vpci_msi_update(const struct pci_dev *pdev, uint32_t data, return 0; } +int vpci_msi_arch_update(struct vpci_msi *msi, const struct pci_dev *pdev) +{ + int rc; + + ASSERT(msi->arch.pirq != INVALID_PIRQ); + + pcidevs_lock(); + rc = vpci_msi_update(pdev, msi->data, msi->address, msi->vectors, + msi->arch.pirq, msi->mask); + if ( rc ) + { + spin_lock(&pdev->domain->event_lock); + unmap_domain_pirq(pdev->domain, msi->arch.pirq); + spin_unlock(&pdev->domain->event_lock); + pcidevs_unlock(); + msi->arch.pirq = INVALID_PIRQ; + return rc; + } + pcidevs_unlock(); + + return 0; +} + static int vpci_msi_enable(const struct pci_dev *pdev, uint32_t data, uint64_t address, unsigned int nr, paddr_t table_base, uint32_t mask) diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c index ad26c38a92..8f15ad7bf2 100644 --- a/xen/drivers/vpci/msi.c +++ b/xen/drivers/vpci/msi.c @@ -87,8 +87,7 @@ static void update_msi(const struct pci_dev *pdev, struct vpci_msi *msi) if ( !msi->enabled ) return; - vpci_msi_arch_disable(msi, pdev); - if ( vpci_msi_arch_enable(msi, pdev, msi->vectors) ) + if ( vpci_msi_arch_update(msi, pdev) ) msi->enabled = false; } diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h index 72d2225a97..af2b8580ee 100644 --- a/xen/include/xen/vpci.h +++ b/xen/include/xen/vpci.h @@ -159,6 +159,8 @@ int __must_check vpci_msi_arch_enable(struct vpci_msi *msi, const struct pci_dev *pdev, unsigned int vectors); void vpci_msi_arch_disable(struct vpci_msi *msi, const struct pci_dev *pdev); +int __must_check vpci_msi_arch_update(struct vpci_msi *msi, + const struct pci_dev *pdev); void vpci_msi_arch_init(struct vpci_msi *msi); void vpci_msi_arch_print(const struct vpci_msi *msi); -- 2.17.0 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] vpci/msi: fix update of bound MSI interrupts 2018-05-08 9:25 ` [PATCH 2/2] vpci/msi: fix update of bound MSI interrupts Roger Pau Monne @ 2018-05-14 12:29 ` Jan Beulich 2018-05-14 14:27 ` Roger Pau Monné 0 siblings, 1 reply; 13+ messages in thread From: Jan Beulich @ 2018-05-14 12:29 UTC (permalink / raw) To: Roger Pau Monne Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, xen-devel >>> On 08.05.18 at 11:25, <roger.pau@citrix.com> wrote: > --- a/xen/arch/x86/hvm/vmsi.c > +++ b/xen/arch/x86/hvm/vmsi.c > @@ -699,6 +699,29 @@ static int vpci_msi_update(const struct pci_dev *pdev, uint32_t data, > return 0; > } > > +int vpci_msi_arch_update(struct vpci_msi *msi, const struct pci_dev *pdev) > +{ > + int rc; > + > + ASSERT(msi->arch.pirq != INVALID_PIRQ); > + > + pcidevs_lock(); > + rc = vpci_msi_update(pdev, msi->data, msi->address, msi->vectors, > + msi->arch.pirq, msi->mask); > + if ( rc ) > + { > + spin_lock(&pdev->domain->event_lock); > + unmap_domain_pirq(pdev->domain, msi->arch.pirq); This looks quite undesirable - a failed update should leave the interrupt in its prior state rather than unbinding it. Is that overly difficult to achieve? At the very least for single-vector-MSI it looks to me as if nothing needed doing here at all to have just this net effect. If so, that special case is probably worthwhile to have, together with a comment on why the same isn't feasible / desirable for the multi vector case. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] vpci/msi: fix update of bound MSI interrupts 2018-05-14 12:29 ` Jan Beulich @ 2018-05-14 14:27 ` Roger Pau Monné 2018-05-14 14:59 ` Jan Beulich 0 siblings, 1 reply; 13+ messages in thread From: Roger Pau Monné @ 2018-05-14 14:27 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, xen-devel On Mon, May 14, 2018 at 06:29:37AM -0600, Jan Beulich wrote: > >>> On 08.05.18 at 11:25, <roger.pau@citrix.com> wrote: > > --- a/xen/arch/x86/hvm/vmsi.c > > +++ b/xen/arch/x86/hvm/vmsi.c > > @@ -699,6 +699,29 @@ static int vpci_msi_update(const struct pci_dev *pdev, uint32_t data, > > return 0; > > } > > > > +int vpci_msi_arch_update(struct vpci_msi *msi, const struct pci_dev *pdev) > > +{ > > + int rc; > > + > > + ASSERT(msi->arch.pirq != INVALID_PIRQ); > > + > > + pcidevs_lock(); > > + rc = vpci_msi_update(pdev, msi->data, msi->address, msi->vectors, > > + msi->arch.pirq, msi->mask); > > + if ( rc ) > > + { > > + spin_lock(&pdev->domain->event_lock); > > + unmap_domain_pirq(pdev->domain, msi->arch.pirq); > > This looks quite undesirable - a failed update should leave the interrupt in its > prior state rather than unbinding it. Is that overly difficult to achieve? Oh, TBH I would expect that writing an invalid data or address fields will disable MSI instead of keep using the old values. I'm not sure I see the reason to keep using the old values, certainly that could make something else go very wonky inside of the guest itself. Thanks, Roger. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] vpci/msi: fix update of bound MSI interrupts 2018-05-14 14:27 ` Roger Pau Monné @ 2018-05-14 14:59 ` Jan Beulich 0 siblings, 0 replies; 13+ messages in thread From: Jan Beulich @ 2018-05-14 14:59 UTC (permalink / raw) To: Roger Pau Monne Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, xen-devel >>> On 14.05.18 at 16:27, <roger.pau@citrix.com> wrote: > On Mon, May 14, 2018 at 06:29:37AM -0600, Jan Beulich wrote: >> >>> On 08.05.18 at 11:25, <roger.pau@citrix.com> wrote: >> > --- a/xen/arch/x86/hvm/vmsi.c >> > +++ b/xen/arch/x86/hvm/vmsi.c >> > @@ -699,6 +699,29 @@ static int vpci_msi_update(const struct pci_dev *pdev, uint32_t data, >> > return 0; >> > } >> > >> > +int vpci_msi_arch_update(struct vpci_msi *msi, const struct pci_dev *pdev) >> > +{ >> > + int rc; >> > + >> > + ASSERT(msi->arch.pirq != INVALID_PIRQ); >> > + >> > + pcidevs_lock(); >> > + rc = vpci_msi_update(pdev, msi->data, msi->address, msi->vectors, >> > + msi->arch.pirq, msi->mask); >> > + if ( rc ) >> > + { >> > + spin_lock(&pdev->domain->event_lock); >> > + unmap_domain_pirq(pdev->domain, msi->arch.pirq); >> >> This looks quite undesirable - a failed update should leave the interrupt in its >> prior state rather than unbinding it. Is that overly difficult to achieve? > > Oh, TBH I would expect that writing an invalid data or address fields > will disable MSI instead of keep using the old values. I'm not sure I > see the reason to keep using the old values, certainly that could make > something else go very wonky inside of the guest itself. Yeah, true, neither is proper behavior comparing to a simple config space write (which can't really fail). Crashing the guest may be a more appropriate action here then. But with what you say I'd also be fine if you kept it as is. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] vpci/msi: fix updating already bound MSI interrupts 2018-05-08 9:23 [PATCH 0/2] vpci/msi: fix updating already bound MSI interrupts Roger Pau Monne 2018-05-08 9:25 ` [PATCH 1/2] vpci/msi: split code to bind pirq Roger Pau Monne 2018-05-08 9:25 ` [PATCH 2/2] vpci/msi: fix update of bound MSI interrupts Roger Pau Monne @ 2018-05-08 9:30 ` Juergen Gross 2018-05-08 9:40 ` Roger Pau Monné 2 siblings, 1 reply; 13+ messages in thread From: Juergen Gross @ 2018-05-08 9:30 UTC (permalink / raw) To: Roger Pau Monne, xen-devel On 08/05/18 11:23, Roger Pau Monne wrote: > Hello, > > There's a bug in current vpci code for MSI emulation when updating an > already bound interrupt. The code will disable and enable the interrupt > in order to update the binding, which calls unmap_domain_pirq that > disables the global MSI enable flag in the control register. > > In order to fix this incorrect behavior introduce a new update helper > that should be used to update the bindings of an already enabled group > of MSI interrupts. > > Thanks, Roger. > > Roger Pau Monne (2): > vpci/msi: split code to bind pirq > vpci/msi: fix update of bound MSI interrupts > > xen/arch/x86/hvm/vmsi.c | 96 +++++++++++++++++++++++++++++------------ > xen/drivers/vpci/msi.c | 3 +- > xen/include/xen/vpci.h | 2 + > 3 files changed, 71 insertions(+), 30 deletions(-) > I guess this is 4.11 material? Juergen _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] vpci/msi: fix updating already bound MSI interrupts 2018-05-08 9:30 ` [PATCH 0/2] vpci/msi: fix updating already " Juergen Gross @ 2018-05-08 9:40 ` Roger Pau Monné 0 siblings, 0 replies; 13+ messages in thread From: Roger Pau Monné @ 2018-05-08 9:40 UTC (permalink / raw) To: Juergen Gross; +Cc: xen-devel On Tue, May 08, 2018 at 11:30:18AM +0200, Juergen Gross wrote: > On 08/05/18 11:23, Roger Pau Monne wrote: > > Hello, > > > > There's a bug in current vpci code for MSI emulation when updating an > > already bound interrupt. The code will disable and enable the interrupt > > in order to update the binding, which calls unmap_domain_pirq that > > disables the global MSI enable flag in the control register. > > > > In order to fix this incorrect behavior introduce a new update helper > > that should be used to update the bindings of an already enabled group > > of MSI interrupts. > > > > Thanks, Roger. > > > > Roger Pau Monne (2): > > vpci/msi: split code to bind pirq > > vpci/msi: fix update of bound MSI interrupts > > > > xen/arch/x86/hvm/vmsi.c | 96 +++++++++++++++++++++++++++++------------ > > xen/drivers/vpci/msi.c | 3 +- > > xen/include/xen/vpci.h | 2 + > > 3 files changed, 71 insertions(+), 30 deletions(-) > > > > I guess this is 4.11 material? Hm, I think so given PVH Dom0 is experimental. OTOH this changes are to code used exclusively by PVH Dom0, so there's no chance of breaking anything else. Let's see what the maintainers think and I can Cc you later if required. Thanks, Roger. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-05-14 15:07 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-08 9:23 [PATCH 0/2] vpci/msi: fix updating already bound MSI interrupts Roger Pau Monne 2018-05-08 9:25 ` [PATCH 1/2] vpci/msi: split code to bind pirq Roger Pau Monne 2018-05-14 12:24 ` Jan Beulich 2018-05-14 14:15 ` Roger Pau Monné 2018-05-14 14:56 ` Jan Beulich 2018-05-14 15:00 ` Roger Pau Monné 2018-05-14 15:07 ` Jan Beulich 2018-05-08 9:25 ` [PATCH 2/2] vpci/msi: fix update of bound MSI interrupts Roger Pau Monne 2018-05-14 12:29 ` Jan Beulich 2018-05-14 14:27 ` Roger Pau Monné 2018-05-14 14:59 ` Jan Beulich 2018-05-08 9:30 ` [PATCH 0/2] vpci/msi: fix updating already " Juergen Gross 2018-05-08 9:40 ` Roger Pau Monné
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.