[PATCH 0/3] x86/intr: interrupt related fixes

[PATCH 0/3] x86/intr: interrupt related fixes

[Roger Pau Monne]

Hello,

This series has originated from the failure discovered by osstest in:

https://lore.kernel.org/xen-devel/E1l1vyr-00074y-8j@osstest.test-lab.xenproject.org/

The commit pointed by osstest is correct, but it triggers an underlying
shortcoming of the vPCI MSI handling which is fixed in patch 3.
Patches 1 and 2 are small bugfixes found in the process of diagnosing
the issue.

Thanks, Roger.

Roger Pau Monne (3):
  x86/irq: remove duplicated clear_domain_irq_pirq calls
  x86/irq: don't disable domain MSI IRQ on force unbind
  x86/vmsi: tolerate unsupported MSI address/data fields

 xen/arch/x86/hvm/vmsi.c      | 93 +++++++++++++++++++-----------------
 xen/arch/x86/irq.c           |  8 +---
 xen/drivers/vpci/msi.c       |  3 +-
 xen/include/asm-x86/hvm/io.h |  1 +
 xen/include/xen/vpci.h       |  3 +-
 5 files changed, 52 insertions(+), 56 deletions(-)

-- 
2.29.2

[PATCH 1/3] x86/irq: remove duplicated clear_domain_irq_pirq calls

[Roger Pau Monne]

There are two duplicated calls to cleanup_domain_irq_pirq in
unmap_domain_pirq.

The first one in the for loop will be called with exactly the same
arguments as the call placed closer to the loop start.

The second one will only be executed when desc != NULL, and that's
already covered by the first call in the for loop above, as any
attempt to unmap a multi vector MSI range will have nr != 1 and thus
always exit the loop with desc == NULL.

Note that those calls are harmless, but make the code harder to read.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
The logic used in the loop seems extremely complex to follow IMO,
there are several breaks for exiting the loop, and the index (i) is
also updated in different places.
---
 xen/arch/x86/irq.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 032fe82167..49849bd7d3 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -2383,9 +2383,6 @@ int unmap_domain_pirq(struct domain *d, int pirq)
 
         spin_unlock_irqrestore(&desc->lock, flags);
 
-        if ( !forced_unbind )
-           cleanup_domain_irq_pirq(d, irq, info);
-
         rc = irq_deny_access(d, irq);
         if ( rc )
         {
@@ -2419,9 +2416,6 @@ int unmap_domain_pirq(struct domain *d, int pirq)
     {
         spin_unlock_irqrestore(&desc->lock, flags);
 
-        if ( !forced_unbind )
-            cleanup_domain_irq_pirq(d, irq, info);
-
         rc = irq_deny_access(d, irq);
         if ( rc )
         {
-- 
2.29.2

[PATCH 2/3] x86/irq: don't disable domain MSI IRQ on force unbind

[Roger Pau Monne]

When force unbinding a MSI the used IRQ would get added to the domain
irq-pirq tree as -irq -pirq, thus preventing the same IRQ to be used
by the domain. Doing so for MSI allocated IRQs prevents the same IRQ
to be used for any other device, since MSI IRQs are allocated and
deallocated on demand unlike legacy PCI IRQs.

Ignore forced unbinds for MSI IRQs and just cleanup the irq-pirq
information as normal.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
It's not clear to me why we add the irq as -irq -pirq to the irq-pirq
domain tree on forced unbind, as allowing to bind the irq again should
just work regardless of whether it has been previously forcefully
unbound?
---
 xen/arch/x86/irq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 49849bd7d3..c8e5277eb1 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -2362,7 +2362,7 @@ int unmap_domain_pirq(struct domain *d, int pirq)
     {
         BUG_ON(irq != domain_pirq_to_irq(d, pirq + i));
 
-        if ( !forced_unbind )
+        if ( !forced_unbind || msi_desc )
             clear_domain_irq_pirq(d, irq, info);
         else
         {
-- 
2.29.2

[PATCH 3/3] x86/vmsi: tolerate unsupported MSI address/data fields

[Roger Pau Monne]

Plain MSI doesn't allow caching the MSI address and data fields while
the capability is enabled and not masked, hence we need to allow any
changes to those fields to update the binding of the interrupt. For
reference, the same doesn't apply to MSI-X that is allowed to cache
the data and address fields while the entry is unmasked, see section
6.8.3.5 of the PCI Local Bus Specification 3.0.

Allowing such updates means that a guest can write an invalid address
(ie: all zeros) and then a valid one, so the PIRQs shouldn't be
unmapped when the interrupt cannot be bound to the guest, since
further updates to the address or data fields can result in the
binding succeeding.

Modify the vPCI MSI arch helpers to track whether the interrupt is
bound, and make failures in vpci_msi_update not unmap the PIRQ, so
that further calls can attempt to bind the PIRQ again.

Note this requires some modifications to the MSI-X handlers, but there
shouldn't be any functional changes in that area.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/vmsi.c      | 93 +++++++++++++++++++-----------------
 xen/drivers/vpci/msi.c       |  3 +-
 xen/include/asm-x86/hvm/io.h |  1 +
 xen/include/xen/vpci.h       |  3 +-
 4 files changed, 51 insertions(+), 49 deletions(-)

diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
index a2ac82c95c..13e2a190b4 100644
--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -715,32 +715,37 @@ 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)
+void vpci_msi_arch_update(struct vpci_msi *msi, const struct pci_dev *pdev)
 {
+    unsigned int i;
     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 )
+    for ( i = 0; i < msi->vectors && msi->arch.bound; i++ )
     {
-        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;
+        struct xen_domctl_bind_pt_irq unbind = {
+            .machine_irq = msi->arch.pirq + i,
+            .irq_type = PT_IRQ_TYPE_MSI,
+        };
+
+        rc = pt_irq_destroy_bind(pdev->domain, &unbind);
+        if ( rc )
+        {
+            ASSERT_UNREACHABLE();
+            domain_crash(pdev->domain);
+            return;
+        }
     }
-    pcidevs_unlock();
 
-    return 0;
+    msi->arch.bound = !vpci_msi_update(pdev, msi->data, msi->address,
+                                       msi->vectors, msi->arch.pirq, msi->mask);
+    pcidevs_unlock();
 }
 
-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)
+static int vpci_msi_enable(const struct pci_dev *pdev, unsigned int nr,
+                           paddr_t table_base)
 {
     struct msi_info msi_info = {
         .seg = pdev->seg,
@@ -749,7 +754,6 @@ static int vpci_msi_enable(const struct pci_dev *pdev, uint32_t data,
         .table_base = table_base,
         .entry_nr = nr,
     };
-    unsigned vectors = table_base ? 1 : nr;
     int rc, pirq = INVALID_PIRQ;
 
     /* Get a PIRQ. */
@@ -763,18 +767,6 @@ static int vpci_msi_enable(const struct pci_dev *pdev, uint32_t data,
         return rc;
     }
 
-    pcidevs_lock();
-    rc = vpci_msi_update(pdev, data, address, vectors, pirq, mask);
-    if ( 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;
 }
 
@@ -784,25 +776,28 @@ int vpci_msi_arch_enable(struct vpci_msi *msi, const struct pci_dev *pdev,
     int rc;
 
     ASSERT(msi->arch.pirq == INVALID_PIRQ);
-    rc = vpci_msi_enable(pdev, msi->data, msi->address, vectors, 0, msi->mask);
-    if ( rc >= 0 )
-    {
-        msi->arch.pirq = rc;
-        rc = 0;
-    }
+    rc = vpci_msi_enable(pdev, vectors, 0);
+    if ( rc < 0 )
+        return rc;
+    msi->arch.pirq = rc;
 
-    return rc;
+    pcidevs_lock();
+    msi->arch.bound = !vpci_msi_update(pdev, msi->data, msi->address, vectors,
+                                       msi->arch.pirq, msi->mask);
+    pcidevs_unlock();
+
+    return 0;
 }
 
 static void vpci_msi_disable(const struct pci_dev *pdev, int pirq,
-                             unsigned int nr)
+                             unsigned int nr, bool bound)
 {
     unsigned int i;
 
     ASSERT(pirq != INVALID_PIRQ);
 
     pcidevs_lock();
-    for ( i = 0; i < nr; i++ )
+    for ( i = 0; i < nr && bound; i++ )
     {
         struct xen_domctl_bind_pt_irq bind = {
             .machine_irq = pirq + i,
@@ -822,7 +817,7 @@ static void vpci_msi_disable(const struct pci_dev *pdev, int pirq,
 
 void vpci_msi_arch_disable(struct vpci_msi *msi, const struct pci_dev *pdev)
 {
-    vpci_msi_disable(pdev, msi->arch.pirq, msi->vectors);
+    vpci_msi_disable(pdev, msi->arch.pirq, msi->vectors, msi->arch.bound);
     msi->arch.pirq = INVALID_PIRQ;
 }
 
@@ -857,14 +852,22 @@ int vpci_msix_arch_enable_entry(struct vpci_msix_entry *entry,
     int rc;
 
     ASSERT(entry->arch.pirq == INVALID_PIRQ);
-    rc = vpci_msi_enable(pdev, entry->data, entry->addr,
-                         vmsix_entry_nr(pdev->vpci->msix, entry),
-                         table_base, entry->masked);
-    if ( rc >= 0 )
+    rc = vpci_msi_enable(pdev, vmsix_entry_nr(pdev->vpci->msix, entry),
+                         table_base);
+    if ( rc < 0 )
+        return rc;
+
+    entry->arch.pirq = rc;
+
+    pcidevs_lock();
+    rc = vpci_msi_update(pdev, entry->data, entry->addr, 1, entry->arch.pirq,
+                         entry->masked);
+    if ( rc )
     {
-        entry->arch.pirq = rc;
-        rc = 0;
+        vpci_msi_disable(pdev, entry->arch.pirq, 1, false);
+        entry->arch.pirq = INVALID_PIRQ;
     }
+    pcidevs_unlock();
 
     return rc;
 }
@@ -875,7 +878,7 @@ int vpci_msix_arch_disable_entry(struct vpci_msix_entry *entry,
     if ( entry->arch.pirq == INVALID_PIRQ )
         return -ENOENT;
 
-    vpci_msi_disable(pdev, entry->arch.pirq, 1);
+    vpci_msi_disable(pdev, entry->arch.pirq, 1, true);
     entry->arch.pirq = INVALID_PIRQ;
 
     return 0;
diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c
index 65db438d24..5757a7aed2 100644
--- a/xen/drivers/vpci/msi.c
+++ b/xen/drivers/vpci/msi.c
@@ -85,8 +85,7 @@ static void update_msi(const struct pci_dev *pdev, struct vpci_msi *msi)
     if ( !msi->enabled )
         return;
 
-    if ( vpci_msi_arch_update(msi, pdev) )
-        msi->enabled = false;
+    vpci_msi_arch_update(msi, pdev);
 }
 
 /* Handlers for the address field (32bit or low part of a 64bit address). */
diff --git a/xen/include/asm-x86/hvm/io.h b/xen/include/asm-x86/hvm/io.h
index 9453b9b2b7..3d2e877110 100644
--- a/xen/include/asm-x86/hvm/io.h
+++ b/xen/include/asm-x86/hvm/io.h
@@ -130,6 +130,7 @@ static inline void msixtbl_init(struct domain *d) {}
 /* Arch-specific MSI data for vPCI. */
 struct vpci_arch_msi {
     int pirq;
+    bool bound;
 };
 
 /* Arch-specific MSI-X entry data for vPCI. */
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index 5295d4c990..9f5b5d52e1 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -160,8 +160,7 @@ 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_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.29.2

Re: [PATCH 1/3] x86/irq: remove duplicated clear_domain_irq_pirq calls

[Jan Beulich]

On 26.01.2021 12:06, Roger Pau Monne wrote:
> There are two duplicated calls to cleanup_domain_irq_pirq in
> unmap_domain_pirq.
> 
> The first one in the for loop will be called with exactly the same
> arguments as the call placed closer to the loop start.

I'm having trouble figuring out which two instances you refer
to: To me "first one" and "closer to the start" are two ways
of expressing the same thing. You don't refer to the call to
clear_domain_irq_pirq(), do you, when the two calls you
remove are to cleanup_domain_irq_pirq()? Admittedly quite
similar names, but entirely different functions.

> The logic used in the loop seems extremely complex to follow IMO,
> there are several breaks for exiting the loop, and the index (i) is
> also updated in different places.

Indeed, and it didn't feel well already back at the time when
I much extended this to support multi-vector MSI. I simply
didn't have any good idea how to streamline all of this
(short of rewriting it altogether, which would have made
patch review quite difficult). If you have thoughts, I'm all
ears.

Jan

Re: [PATCH 2/3] x86/irq: don't disable domain MSI IRQ on force unbind

[Jan Beulich]

On 26.01.2021 12:06, Roger Pau Monne wrote:
> When force unbinding a MSI the used IRQ would get added to the domain
> irq-pirq tree as -irq -pirq,

I think it's -pirq at index irq, i.e. I don't think IRQ gets
negated as far as the radix tree goes. info->arch.irq gets a
negative value stored, yes.

> thus preventing the same IRQ to be used by the domain.

Iirc this (answering your post-commit-message question here)
is for cleaning up _after_ the domain, i.e. there's no goal
to allow re-use of this IRQ. The comment ahead of
unmap_domain_pirq() validly says "The pirq should have been
unbound before this call." The only time we can't make
ourselves dependent upon this is when the guest is being
cleaned up. During normal operation I think we actually
_want_ to enforce correct behavior of the guest here.

> It's not clear to me why we add the irq as -irq -pirq to the irq-pirq
> domain tree on forced unbind, as allowing to bind the irq again should
> just work regardless of whether it has been previously forcefully
> unbound?

To continue from the above, see pirq_guest_unbind() where
we have

    if ( desc == NULL )
    {
        irq = -pirq->arch.irq;
        BUG_ON(irq <= 0);
        desc = irq_to_desc(irq);
        spin_lock_irq(&desc->lock);
        clear_domain_irq_pirq(d, irq, pirq);
    }

as the alternative to going the normal path through
__pirq_guest_unbind(). Again iirc that's to cover for the
unbind request arriving after the unmap one (i.e. having
caused the unmap to force-unbind the IRQ).

Jan

Re: [PATCH 3/3] x86/vmsi: tolerate unsupported MSI address/data fields

[Jan Beulich]

On 26.01.2021 12:06, Roger Pau Monne wrote:
> Plain MSI doesn't allow caching the MSI address and data fields while
> the capability is enabled and not masked, hence we need to allow any
> changes to those fields to update the binding of the interrupt. For
> reference, the same doesn't apply to MSI-X that is allowed to cache
> the data and address fields while the entry is unmasked, see section
> 6.8.3.5 of the PCI Local Bus Specification 3.0.
> 
> Allowing such updates means that a guest can write an invalid address
> (ie: all zeros) and then a valid one, so the PIRQs shouldn't be
> unmapped when the interrupt cannot be bound to the guest, since
> further updates to the address or data fields can result in the
> binding succeeding.

IOW the breakage from the other patch was because rubbish was
written first, and suitable data was written later on? I didn't
think core PCI code in Linux would do such, which would make me
suspect a driver having custom MSI handling code ...

> Modify the vPCI MSI arch helpers to track whether the interrupt is
> bound, and make failures in vpci_msi_update not unmap the PIRQ, so
> that further calls can attempt to bind the PIRQ again.
> 
> Note this requires some modifications to the MSI-X handlers, but there
> shouldn't be any functional changes in that area.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

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

Am I understanding correctly that this change is independent of
the initial 2 patches (where I have reservations), and hence
it could go in ahead of them (or all alone)?

Jan

Re: [PATCH 3/3] x86/vmsi: tolerate unsupported MSI address/data fields

[Roger Pau Monné]

On Tue, Jan 26, 2021 at 04:17:59PM +0100, Jan Beulich wrote:
> On 26.01.2021 12:06, Roger Pau Monne wrote:
> > Plain MSI doesn't allow caching the MSI address and data fields while
> > the capability is enabled and not masked, hence we need to allow any
> > changes to those fields to update the binding of the interrupt. For
> > reference, the same doesn't apply to MSI-X that is allowed to cache
> > the data and address fields while the entry is unmasked, see section
> > 6.8.3.5 of the PCI Local Bus Specification 3.0.
> > 
> > Allowing such updates means that a guest can write an invalid address
> > (ie: all zeros) and then a valid one, so the PIRQs shouldn't be
> > unmapped when the interrupt cannot be bound to the guest, since
> > further updates to the address or data fields can result in the
> > binding succeeding.
> 
> IOW the breakage from the other patch was because rubbish was
> written first, and suitable data was written later on? I didn't
> think core PCI code in Linux would do such, which would make me
> suspect a driver having custom MSI handling code ...

So it seems that specific Linux driver will write 0s to the address
field at some point during initialization, but it also doesn't end up
using MSI interrupts anyway, so I assume it's somehow broken. FTR it's
the snd_hda_intel driver.

However it seems like Linux likes to zero all addresses fields on
shutdown for MSI (not MSI-X) with the capability enabled, and I do
see:

vmsi.c:688:d0v2 0000:00:1c.3: PIRQ 643: unsupported address 0
vmsi.c:688:d0v2 0000:00:1c.3: PIRQ 643: unsupported address 0
vmsi.c:688:d0v2 0000:00:1c.0: PIRQ 644: unsupported address 0
vmsi.c:688:d0v2 0000:00:1c.0: PIRQ 644: unsupported address 0
vmsi.c:688:d0v2 0000:00:14.0: PIRQ 641: unsupported address 0
vmsi.c:688:d0v2 0000:00:14.0: PIRQ 641: unsupported address 0
vmsi.c:688:d0v2 0000:00:14.0: PIRQ 641: unsupported address 0
vmsi.c:688:d0v2 0000:00:01.2: PIRQ 645: unsupported address 0
vmsi.c:688:d0v2 0000:00:01.2: PIRQ 645: unsupported address 0
vmsi.c:688:d0v2 0000:00:01.1: PIRQ 646: unsupported address 0
vmsi.c:688:d0v2 0000:00:01.1: PIRQ 646: unsupported address 0
vmsi.c:688:d0v2 0000:00:01.0: PIRQ 647: unsupported address 0
vmsi.c:688:d0v2 0000:00:01.0: PIRQ 647: unsupported address 0

When dom0 is shutting down. That's with the 5.4 kernel, maybe other
versions won't do it.

> > Modify the vPCI MSI arch helpers to track whether the interrupt is
> > bound, and make failures in vpci_msi_update not unmap the PIRQ, so
> > that further calls can attempt to bind the PIRQ again.
> > 
> > Note this requires some modifications to the MSI-X handlers, but there
> > shouldn't be any functional changes in that area.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> Am I understanding correctly that this change is independent of
> the initial 2 patches (where I have reservations), and hence
> it could go in ahead of them (or all alone)?

Yes, it's fully independent.

Thanks, Roger.

Re: [PATCH 1/3] x86/irq: remove duplicated clear_domain_irq_pirq calls

[Roger Pau Monné]

On Tue, Jan 26, 2021 at 03:38:27PM +0100, Jan Beulich wrote:
> On 26.01.2021 12:06, Roger Pau Monne wrote:
> > There are two duplicated calls to cleanup_domain_irq_pirq in
> > unmap_domain_pirq.
> > 
> > The first one in the for loop will be called with exactly the same
> > arguments as the call placed closer to the loop start.
> 
> I'm having trouble figuring out which two instances you refer
> to: To me "first one" and "closer to the start" are two ways
> of expressing the same thing. You don't refer to the call to
> clear_domain_irq_pirq(), do you, when the two calls you
> remove are to cleanup_domain_irq_pirq()? Admittedly quite
> similar names, but entirely different functions.

Urg, yes, that's impossible to parse sensibly, sorry.

Also the subject is wrong, should be cleanup_domain_irq_pirq, not
clear_domain_irq_pirq.

This should instead be:

"There are two duplicated calls to cleanup_domain_irq_pirq in
unmap_domain_pirq.

The first removed call to cleanup_domain_irq_pirq will be called with
exactly the same arguments as the previous call placed above it."

It's hard to explain this in a commit message.

Do you agree that the removed calls are duplicated though? I might have
as well missed part of the logic here and be wrong about this.

> > The logic used in the loop seems extremely complex to follow IMO,
> > there are several breaks for exiting the loop, and the index (i) is
> > also updated in different places.
> 
> Indeed, and it didn't feel well already back at the time when
> I much extended this to support multi-vector MSI. I simply
> didn't have any good idea how to streamline all of this
> (short of rewriting it altogether, which would have made
> patch review quite difficult). If you have thoughts, I'm all
> ears.

I would just rewrite it altogether. Code like this is very prone to
cause mistakes in the future IMO. If you want I can try to.

I guess the problem with this is that we would lose the history of the
code for no functional change.

Thanks, Roger.

Re: [PATCH 1/3] x86/irq: remove duplicated clear_domain_irq_pirq calls

[Jan Beulich]

On 26.01.2021 17:08, Roger Pau Monné wrote:
> On Tue, Jan 26, 2021 at 03:38:27PM +0100, Jan Beulich wrote:
>> On 26.01.2021 12:06, Roger Pau Monne wrote:
>>> There are two duplicated calls to cleanup_domain_irq_pirq in
>>> unmap_domain_pirq.
>>>
>>> The first one in the for loop will be called with exactly the same
>>> arguments as the call placed closer to the loop start.
>>
>> I'm having trouble figuring out which two instances you refer
>> to: To me "first one" and "closer to the start" are two ways
>> of expressing the same thing. You don't refer to the call to
>> clear_domain_irq_pirq(), do you, when the two calls you
>> remove are to cleanup_domain_irq_pirq()? Admittedly quite
>> similar names, but entirely different functions.
> 
> Urg, yes, that's impossible to parse sensibly, sorry.
> 
> Also the subject is wrong, should be cleanup_domain_irq_pirq, not
> clear_domain_irq_pirq.
> 
> This should instead be:
> 
> "There are two duplicated calls to cleanup_domain_irq_pirq in
> unmap_domain_pirq.
> 
> The first removed call to cleanup_domain_irq_pirq will be called with
> exactly the same arguments as the previous call placed above it."

And which one is "the previous call"? I'm still getting the
impression you're mixing up cleanup_domain_irq_pirq() and
clear_domain_irq_pirq(). (I guess we need to resort to line
numbers in current staging or master, to avoid
misunderstandings. Not for the commit message [if any], but
for the discussion.)

> It's hard to explain this in a commit message.
> 
> Do you agree that the removed calls are duplicated though? I might have
> as well missed part of the logic here and be wrong about this.

No, for the moment I don't agree yet, because I don't see
the redundancy so far.

>>> The logic used in the loop seems extremely complex to follow IMO,
>>> there are several breaks for exiting the loop, and the index (i) is
>>> also updated in different places.
>>
>> Indeed, and it didn't feel well already back at the time when
>> I much extended this to support multi-vector MSI. I simply
>> didn't have any good idea how to streamline all of this
>> (short of rewriting it altogether, which would have made
>> patch review quite difficult). If you have thoughts, I'm all
>> ears.
> 
> I would just rewrite it altogether. Code like this is very prone to
> cause mistakes in the future IMO. If you want I can try to.

I wouldn't mind, but yes, besides review difficulties ...

> I guess the problem with this is that we would lose the history of the
> code for no functional change.

... this also wouldn't be overly nice (but can be dealt with
via a few more steps wading through git history).

Jan

Re: [PATCH 2/3] x86/irq: don't disable domain MSI IRQ on force unbind

[Roger Pau Monné]

On Tue, Jan 26, 2021 at 03:52:54PM +0100, Jan Beulich wrote:
> On 26.01.2021 12:06, Roger Pau Monne wrote:
> > When force unbinding a MSI the used IRQ would get added to the domain
> > irq-pirq tree as -irq -pirq,
> 
> I think it's -pirq at index irq, i.e. I don't think IRQ gets
> negated as far as the radix tree goes. info->arch.irq gets a
> negative value stored, yes.

Right, and this then prevents the IRQ to be used at all by the domain.
Doiong a domain_irq_to_pirq with that IRQ will get -pirq, but that
seems pretty arbitrary for MSI IRQs, that get allocated on demand.

At the end of unmap_domain_pirq the IRQ will get freed if it was
assigned to an MSI source, and hence it seem pointless to add irq ->
-pirq to the domain irq tree.

> > thus preventing the same IRQ to be used by the domain.
> 
> Iirc this (answering your post-commit-message question here)
> is for cleaning up _after_ the domain, i.e. there's no goal
> to allow re-use of this IRQ. The comment ahead of
> unmap_domain_pirq() validly says "The pirq should have been
> unbound before this call." The only time we can't make
> ourselves dependent upon this is when the guest is being
> cleaned up. During normal operation I think we actually
> _want_ to enforce correct behavior of the guest here.

OK, so that might be fine for legacy PCI IRQs, that are fixed, but
quite pointless for allocated on demand MSI IRQs that can change
between allocations.

> > It's not clear to me why we add the irq as -irq -pirq to the irq-pirq
> > domain tree on forced unbind, as allowing to bind the irq again should
> > just work regardless of whether it has been previously forcefully
> > unbound?
> 
> To continue from the above, see pirq_guest_unbind() where
> we have
> 
>     if ( desc == NULL )
>     {
>         irq = -pirq->arch.irq;
>         BUG_ON(irq <= 0);
>         desc = irq_to_desc(irq);
>         spin_lock_irq(&desc->lock);
>         clear_domain_irq_pirq(d, irq, pirq);
>     }
> 
> as the alternative to going the normal path through
> __pirq_guest_unbind(). Again iirc that's to cover for the
> unbind request arriving after the unmap one (i.e. having
> caused the unmap to force-unbind the IRQ).

Oh, so that's the point. Do you think you could add some comments to
explain the indented behaviour? I think I get it now, but it's hard to
follow without some pointers.

Thanks, Roger.

Re: [PATCH 2/3] x86/irq: don't disable domain MSI IRQ on force unbind

[Jan Beulich]

On 26.01.2021 17:21, Roger Pau Monné wrote:
> On Tue, Jan 26, 2021 at 03:52:54PM +0100, Jan Beulich wrote:
>> On 26.01.2021 12:06, Roger Pau Monne wrote:
>>> When force unbinding a MSI the used IRQ would get added to the domain
>>> irq-pirq tree as -irq -pirq,
>>
>> I think it's -pirq at index irq, i.e. I don't think IRQ gets
>> negated as far as the radix tree goes. info->arch.irq gets a
>> negative value stored, yes.
> 
> Right, and this then prevents the IRQ to be used at all by the domain.
> Doiong a domain_irq_to_pirq with that IRQ will get -pirq, but that
> seems pretty arbitrary for MSI IRQs, that get allocated on demand.
> 
> At the end of unmap_domain_pirq the IRQ will get freed if it was
> assigned to an MSI source,

This is a good point, but ...

> and hence it seem pointless to add irq ->
> -pirq to the domain irq tree.

... without this I think ...

>>> thus preventing the same IRQ to be used by the domain.
>>
>> Iirc this (answering your post-commit-message question here)
>> is for cleaning up _after_ the domain, i.e. there's no goal
>> to allow re-use of this IRQ. The comment ahead of
>> unmap_domain_pirq() validly says "The pirq should have been
>> unbound before this call." The only time we can't make
>> ourselves dependent upon this is when the guest is being
>> cleaned up. During normal operation I think we actually
>> _want_ to enforce correct behavior of the guest here.
> 
> OK, so that might be fine for legacy PCI IRQs, that are fixed, but
> quite pointless for allocated on demand MSI IRQs that can change
> between allocations.
> 
>>> It's not clear to me why we add the irq as -irq -pirq to the irq-pirq
>>> domain tree on forced unbind, as allowing to bind the irq again should
>>> just work regardless of whether it has been previously forcefully
>>> unbound?
>>
>> To continue from the above, see pirq_guest_unbind() where
>> we have
>>
>>     if ( desc == NULL )
>>     {
>>         irq = -pirq->arch.irq;
>>         BUG_ON(irq <= 0);

... this would then trigger (or other badness result) if the
guest does unmap and unbind in the wrong order.

>>         desc = irq_to_desc(irq);
>>         spin_lock_irq(&desc->lock);
>>         clear_domain_irq_pirq(d, irq, pirq);
>>     }
>>
>> as the alternative to going the normal path through
>> __pirq_guest_unbind(). Again iirc that's to cover for the
>> unbind request arriving after the unmap one (i.e. having
>> caused the unmap to force-unbind the IRQ).
> 
> Oh, so that's the point. Do you think you could add some comments to
> explain the indented behaviour? I think I get it now, but it's hard to
> follow without some pointers.

Let's first settle on what exactly to do here, because this
may then affect what exactly those comments would want to
say. (If the patch here ended up touching at least one of
the two involved pieces of code, perhaps such comments
could also get added while altering that code.)

Jan