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

[PATCH 0/2] 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 (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

[PATCH 1/2] 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 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

[PATCH 2/2] 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 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

Re: [PATCH 0/2] vpci/msi: fix updating already bound MSI interrupts

[Juergen Gross]

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

Re: [PATCH 0/2] vpci/msi: fix updating already bound MSI interrupts

[Roger Pau Monné]

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

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

[Jan Beulich]

>>> 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

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

[Jan Beulich]

>>> 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

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

[Roger Pau Monné]

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

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

[Roger Pau Monné]

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

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

[Jan Beulich]

>>> 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

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

[Jan Beulich]

>>> 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

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

[Roger Pau Monné]

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

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

[Jan Beulich]

>>> 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