Re: [PATCH] pci: Add option to disable device level INTx masking

[Alex Williamson]

On Fri, 8 Mar 2024 14:37:06 -0500
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Fri, Mar 08, 2024 at 10:24:14AM -0700, Alex Williamson wrote:
> > On Fri, 8 Mar 2024 11:57:38 -0500
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > On Thu, Mar 07, 2024 at 11:46:42AM -0700, Alex Williamson wrote:  
> > > > The PCI 2.3 spec added definitions of the INTx disable and status bits,
> > > > in the command and status registers respectively.  The command register
> > > > bit, commonly known as DisINTx in lspci, controls whether the device
> > > > can assert the INTx signal.
> > > > 
> > > > Operating systems will often write to this bit to test whether a device
> > > > supports this style of legacy interrupt masking.  When using device
> > > > assignment, such as with vfio-pci, the result of this test dictates
> > > > whether the device can use a shared or exclusive interrupt (ie. generic
> > > > INTx masking at the device via DisINTx or IRQ controller level INTx
> > > > masking).
> > > > 
> > > > Add an experimental option to the base set of properties for PCI
> > > > devices which allows the DisINTx bit to be excluded from wmask, making
> > > > it read-only to the guest for testing purposes related to INTx masking.
> > > >     
> > > 
> > > Could you clarify the use a bit more? It's unstable - do you
> > > expect to experiment with it and then make it permanent down
> > > the road?  
> > 
> > No, my aspirations end at providing an experimental option.
> > Technically all devices should support and honor this bit, so I don't
> > think we should provide a supported method of providing broken behavior,
> > but there do exist physical devices where this feature is broken or
> > unsupported.  Rather than implementing emulation of one of these broken
> > devices, with bug for bug compatibility, it's much easier to be able to
> > trigger broken DisINTx behavior on an arbitrary device, in an
> > unsupported fashion.  Thanks,
> > 
> > Alex  
> 
> Well, we tend not to merge patches for playing with random
> bits in config space just so people can experiment with
> whether this breaks guests, but given this is coming from
> a long term contributor and a maintainer, it's a different
> matter. So ok, to make another maintainer's life easier
> I'm prepared to take this. I'd like to figure out though -
> does your need extend to experimenting with all devices
> or just with vfio ones? If the later maybe keep it there
> where you understand what the actual need is... If the former
> as I said I'll merge it.

I'm actually looking at using it with non-vfio devices, for example I
have a dummy nvme driver that can configure either INTx, MSI, or MSI-X
interrupts.  The driver just stuffs nop commands into the admin queue to
trigger an interrupt.  This tests DMA mapping and interrupt paths.  I
intend to port this to a userspace vfio-pci driver that I can run in a
guest on an emulated nvme device, thereby enabling targeted testing
without any host hardware or device dependencies.  If I were to expose
two emulated nvme devices to the guest, one with DisINTx disabled, then
all variations could be tested.

For full disclosure, the vfio-pci kernel driver does have a nointxmask
module option, so while I think it would be useful and provides a
little more flexibility that devices in QEMU can be specified with this
behavior, there are means to do it otherwise. The QEMU vfio-pci driver
certainly has experimental options that don't necessarily have a path
to become supported, I hadn't realized your intention/preference to
make it a staging ground for to-be-supported options for PCIDevice.

If you have concerns about cluttering options or maintaining dead-end
experimental options, let's hold off on this until there's a case that
can't be met with the kernel module option.  Thanks,

Alex

> > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > > > ---
> > > >  hw/pci/pci.c         | 14 ++++++++++----
> > > >  include/hw/pci/pci.h |  2 ++
> > > >  2 files changed, 12 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > > index 6496d027ca61..8c78326ad67f 100644
> > > > --- a/hw/pci/pci.c
> > > > +++ b/hw/pci/pci.c
> > > > @@ -85,6 +85,8 @@ static Property pci_props[] = {
> > > >                      QEMU_PCIE_ERR_UNC_MASK_BITNR, true),
> > > >      DEFINE_PROP_BIT("x-pcie-ari-nextfn-1", PCIDevice, cap_present,
> > > >                      QEMU_PCIE_ARI_NEXTFN_1_BITNR, false),
> > > > +    DEFINE_PROP_BIT("x-pci-disintx", PCIDevice, cap_present,
> > > > +                    QEMU_PCI_DISINTX_BITNR, true),
> > > >      DEFINE_PROP_END_OF_LIST()
> > > >  };
> > > >  
> > > > @@ -861,13 +863,17 @@ static void pci_init_cmask(PCIDevice *dev)
> > > >  static void pci_init_wmask(PCIDevice *dev)
> > > >  {
> > > >      int config_size = pci_config_size(dev);
> > > > +    uint16_t cmd_wmask = PCI_COMMAND_IO | PCI_COMMAND_MEMORY |
> > > > +                         PCI_COMMAND_MASTER | PCI_COMMAND_SERR;
> > > >  
> > > >      dev->wmask[PCI_CACHE_LINE_SIZE] = 0xff;
> > > >      dev->wmask[PCI_INTERRUPT_LINE] = 0xff;
> > > > -    pci_set_word(dev->wmask + PCI_COMMAND,
> > > > -                 PCI_COMMAND_IO | PCI_COMMAND_MEMORY | 
> > > > PCI_COMMAND_MASTER |
> > > > -                 PCI_COMMAND_INTX_DISABLE);
> > > > -    pci_word_test_and_set_mask(dev->wmask + PCI_COMMAND, 
> > > > PCI_COMMAND_SERR);
> > > > +
> > > > +    if (dev->cap_present & QEMU_PCI_DISINTX) {
> > > > +        cmd_wmask |= PCI_COMMAND_INTX_DISABLE;
> > > > +    }
> > > > +
> > > > +    pci_set_word(dev->wmask + PCI_COMMAND, cmd_wmask);
> > > >  
> > > >      memset(dev->wmask + PCI_CONFIG_HEADER_SIZE, 0xff,
> > > >             config_size - PCI_CONFIG_HEADER_SIZE);
> > > > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > > > index eaa3fc99d884..45f0fac435cc 100644
> > > > --- a/include/hw/pci/pci.h
> > > > +++ b/include/hw/pci/pci.h
> > > > @@ -212,6 +212,8 @@ enum {
> > > >      QEMU_PCIE_ERR_UNC_MASK = (1 << QEMU_PCIE_ERR_UNC_MASK_BITNR),
> > > >  #define QEMU_PCIE_ARI_NEXTFN_1_BITNR 12
> > > >      QEMU_PCIE_ARI_NEXTFN_1 = (1 << QEMU_PCIE_ARI_NEXTFN_1_BITNR),
> > > > +#define QEMU_PCI_DISINTX_BITNR 13
> > > > +    QEMU_PCI_DISINTX = (1 << QEMU_PCI_DISINTX_BITNR),
> > > >  };
> > > >  
> > > >  typedef struct PCIINTxRoute {
> > > > -- 
> > > > 2.44.0    
> > >   
>