Re: [PATCH 3/8] pci/msix: Implement PBA writes

["Nicholas Piggin" <npiggin@gmail.com>]

On Fri Dec 13, 2024 at 3:14 PM AEST, Akihiko Odaki wrote:
> On 2024/12/12 17:34, Nicholas Piggin wrote:
> > Implement MMIO PBA writes, 1 to trigger and 0 to clear.
> > 
> > This functionality is used by some qtests, which keep the msix irq
> > masked and test irq pending via the PBA bits, for simplicity. Some
> > tests expect to be able to clear the irq with a store, so a side-effect
> > of this is that qpci_msix_pending() would actually clear the pending
> > bit where it previously did not. This actually causes some [possibly
> > buggy] tests to fail. So to avoid breakage until tests are re-examined,
> > prior behavior of qpci_msix_pending() is kept by changing it to avoid
> > clearing PBA.
> > 
> > A new function qpci_msix_test_clear_pending() is added for tests that
> > do want the PBA clearing, and it will be used by XHCI and e1000e/igb
> > tests in subsequent changes.
>
> The specification says software should never write Pending Bits and its 
> result is undefined. Tests should have an alternative method to clear 
> Pending Bits.

Thanks for correcting me. I guess qpci_msix_pending() should not be
trying to write to the PBA either then.

> A possible solution is to unmask the interrupt, wait until the Pending 
> Bits get cleared, and mask it again.

PCI spec says

  If a masked vector has its Pending bit set, and the associated
  underlying interrupt events are somehow satisfied (usually by software
  though the exact manner is function-specific), the function must clear
  the Pending bit, to avoid sending a spurious interrupt message later
  when software unmasks the vector. However, if a subsequent interrupt
  event occurs while the vector is still masked, the function must again
  set the Pending bit.

It looks like e1000e acutally does that with e1000e_msix_clear{_one}.
So perhaps this will work just with the e1000e ICR clearing patch. I
will test.

e1000e and igb are the only devices that call msix_clr_pending. Does
that mean many others probably do not implement this behaviour
correctly?

Thanks,
Nick