[PATCH v2 0/3] vpci/msi: fix updating already bound MSI interrupts

[PATCH v2 0/3] vpci/msi: fix updating already bound MSI interrupts

[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 (3):
  vpci/msi: fix unbind loop
  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

[PATCH v2 1/3] vpci/msi: fix unbind loop

[Roger Pau Monne]

The current unbind loop on failure in vpci_msi_enable is wrong and
will only work correctly if the initial pirq is 0. Fix this by adding
a proper bound.

Reported-by: Jan Beulich <jbeulich@suse.com>
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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
index 900d4f67d4..5ab7387d78 100644
--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -710,7 +710,7 @@ static int vpci_msi_enable(const struct pci_dev *pdev, uint32_t data,
                      "%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-- )
+            while ( bind.machine_irq-- > pirq )
                 pt_irq_destroy_bind(pdev->domain, &bind);
             spin_lock(&pdev->domain->event_lock);
             unmap_domain_pirq(pdev->domain, pirq);
-- 
2.17.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 2/3] vpci/msi: split code to bind pirq

[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 5ab7387d78..acadc23f8d 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-- > pirq )
+                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-- > pirq )
-                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

[PATCH v2 3/3] vpci/msi: fix update of bound MSI interrupts

[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 acadc23f8d..3001d5c488 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

Re: [PATCH v2 1/3] vpci/msi: fix unbind loop

[Jan Beulich]

>>> On 15.05.18 at 16:10, <roger.pau@citrix.com> wrote:
> The current unbind loop on failure in vpci_msi_enable is wrong and
> will only work correctly if the initial pirq is 0. Fix this by adding
> a proper bound.
> 
> Reported-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jürgen,

any thoughts on taking this one for 4.11?

Jan

> --- a/xen/arch/x86/hvm/vmsi.c
> +++ b/xen/arch/x86/hvm/vmsi.c
> @@ -710,7 +710,7 @@ static int vpci_msi_enable(const struct pci_dev *pdev, 
> uint32_t data,
>                       "%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-- )
> +            while ( bind.machine_irq-- > pirq )
>                  pt_irq_destroy_bind(pdev->domain, &bind);
>              spin_lock(&pdev->domain->event_lock);
>              unmap_domain_pirq(pdev->domain, pirq);
> -- 
> 2.17.0




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 2/3] vpci/msi: split code to bind pirq

[Jan Beulich]

>>> On 15.05.18 at 16:10, <roger.pau@citrix.com> wrote:
> 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>

Reviewed-by: Jan Beulich <jbeulich@suse.com>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 3/3] vpci/msi: fix update of bound MSI interrupts

[Jan Beulich]

>>> On 15.05.18 at 16:10, <roger.pau@citrix.com> wrote:
> 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>

Acked-by: Jan Beulich <jbeulich@suse.com>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 1/3] vpci/msi: fix unbind loop

[Juergen Gross]

On 16/05/18 14:46, Jan Beulich wrote:
>>>> On 15.05.18 at 16:10, <roger.pau@citrix.com> wrote:
>> The current unbind loop on failure in vpci_msi_enable is wrong and
>> will only work correctly if the initial pirq is 0. Fix this by adding
>> a proper bound.
>>
>> Reported-by: Jan Beulich <jbeulich@suse.com>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> Jürgen,
> 
> any thoughts on taking this one for 4.11?

Release-acked-by: Juergen Gross <jgross@suse.com>


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel