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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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@xxxxxxxxxx>
> 
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> 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.