Re: [PATCH v2 2/2] vpci/msix: fix PBA accesses

[Alex Olson <this.is.a0lson@xxxxxxxxx>]

Hi Roger,

The revised patch looks good.  The PBA writes seen during ioatdma driver
initialization (self-test) complete successfully and the driver doesn't complain
(I see two interrupts per ioatdma device).   The driver has a self-test feature
which appears to exercise MSIX interrupts and has code which appears to cause a
PBA write.

Feel free to add "Tested-by: Alex.Olson@xxxxxxxxxx" to your patchset.

Thanks also for the pointer to your 2018 work on SR-IOV, I'll give it a try. 


FYI, with this patch,  I was seeing  msix_read() and msix_write() being called
during the driver's self-test on physical address 0xfbc01800, corresponding to
the beginning of the PBA (lspci excerpt below):


02:00.0 System peripheral: Intel Corporation Xeon Processor D Family QuickData 
Technology Register DMA Channel 0
...
        Region 0: Memory at fbc06000 (64-bit, non-prefetchable) [size=8K]
...
        Capabilities: [ac] MSI-X: Enable+ Count=1 Masked-
                Vector table: BAR=0 offset=00001000
                PBA: BAR=0 offset=00001800
...
        Kernel modules: ioatdma



The functions involved on the Linux kernel side are:

ioat_probe()
 -> ioat3_dma_self_test()
  -> ioat_dma_self_test()
   -> ioat_free_chan_resources()
    ->  ioat_reset_hw()

drivers/dma/ioat/dma.c:   ioat_reset_hw()
...
    ioat_dma->msixpba = readq(ioat_dma->reg_base + 0x1800);
...
    writeq(ioat_dma->msixpba, ioat_dma->reg_base + 0x1800);


Thanks,

-Alex

On Sat, 2022-02-26 at 11:10 +0100, Roger Pau Monné wrote:
> On Fri, Feb 25, 2022 at 11:57:05AM -0600, Alex Olson wrote:
> > I think there is an issue in the spin_lock handling of patch 2 for the
> > "msix_write" function as it results in the lock being taken a second time
> > while
> > held (hangs). 
> > 
> > The lock taken before checking "VMSIX_ADDR_IN_RANGE" isn't unlocked for the
> > non-
> > PBA case and a second lock is attempted just before the call to get_entry()
> > later in the same function.  It looks like either the added lock should
> > either
> > be moved inside the PBA case or the lock before get_entry() should be
> > removed.
> 
> Sorry, was in a rush to send this before leaving yesterday and didn't
> refresh the commit before generating the patch, v2.1 should be fixed.
> 
> Could you provide a 'Tested-by' if it work for you?
> 
> > On my server, upon loading the ioatdma driver, it now successfully attempts
> > an
> > PBA write (which now doesn't crash the system), but I'm not sure I have a
> > way to
> > fully exercise it...
> 
> Urg, that's weird, PBA should be read-only only according to the spec.
> Writes to PBA have undefined behavior.
> 
> > I also see a different (related) issue in which modify_bars is called on a
> > virtual function seemingly before the BAR addresses are initialized/known
> > and
> > will start a different thread for that topic.
> 
> SR-IOV is not supported on PVH dom0 yet, so that's not going to work.
> I've posted a series in 2018 to enable it, but sadly had no time to
> work on it anymore:
> 
> https://lore.kernel.org/xen-devel/20180717094830.54806-1-roger.pau@xxxxxxxxxx/
> 
> It's likely not going to apply cleanly, and there's a lot of comments
> to be fixed up there.
> 
> Thanks, Roger.