Re: [RFC 2/8] s390x/pci: MSI-X isn't strictly required for passthrough

[Matthew Rosato]

On 12/17/20 8:08 AM, Cornelia Huck wrote:
> On Thu, 10 Dec 2020 10:13:29 -0500
> Matthew Rosato <mjrosato@linux.ibm.com> wrote:
>
> > On 12/10/20 5:28 AM, Cornelia Huck wrote:
> > > On Wed,  9 Dec 2020 15:34:20 -0500
> > > Matthew Rosato <mjrosato@linux.ibm.com> wrote:
> > >    
> > > > s390 PCI currently disallows PCI devices without the MSI-X capability.
> > > > However, this fence doesn't make sense for passthrough devices.  Move
> > > > the check to only fence emulated devices (e.g., virtio).
> > > >
> > > > Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> > > > Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
> > > > ---
> > > >    hw/s390x/s390-pci-bus.c | 14 ++++++++------
> > > >    1 file changed, 8 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> > > > index 05f7460..afad048 100644
> > > > --- a/hw/s390x/s390-pci-bus.c
> > > > +++ b/hw/s390x/s390-pci-bus.c
> > > > @@ -1028,12 +1028,12 @@ static void s390_pcihost_plug(HotplugHandler 
> > > > *hotplug_dev, DeviceState *dev,
> > > >                s390_pci_get_clp_info(pbdev);
> > > >            } else {
> > > >                pbdev->fh |= FH_SHM_EMUL;
> > > > -        }
> > > >    
> > > > -        if (s390_pci_msix_init(pbdev)) {
> > > > -            error_setg(errp, "MSI-X support is mandatory "
> > > > -                       "in the S390 architecture");
> > > > -            return;
> > > > +            if (s390_pci_msix_init(pbdev)) {
> > > > +                error_setg(errp, "MSI-X support is mandatory "
> > > > +                           "in the S390 architecture");
> > > > +                return;
> > > > +            }
> > > >            }
> > > >    
> > > >            if (dev->hotplugged) {
> > > > @@ -1073,7 +1073,9 @@ static void s390_pcihost_unplug(HotplugHandler 
> > > > *hotplug_dev, DeviceState *dev,
> > > >            devfn = pci_dev->devfn;
> > > >            qdev_unrealize(dev);
> > > >    
> > > > -        s390_pci_msix_free(pbdev);
> > > > +        if (pbdev->fh & FH_SHM_EMUL) {
> > > > +            s390_pci_msix_free(pbdev);
> > > > +        }
> > > >            s390_pci_iommu_free(s, bus, devfn);
> > > >            pbdev->pdev = NULL;
> > > >            pbdev->state = ZPCI_FS_RESERVED;
> > >
> > > Remind me: Wasn't it only msi that was strictly required (i.e., not msi-x?)
> > >
> > > Can we generally relax this requirement, possibly with some changes in
> > > the adapter interrupt mapping? I might misremember, though.
> > >    
> >
> > Yes, but even so our current emulation support only sets up for MSI-X,
> > it does not have an msi_init() equivalent.  I do believe that this
> > requirement can be relaxed at some point for the emulation support as
> > well, but the focus on this set was to at least stop fencing passthrough
> > for no reason.
>
> Looking back to when this was introduced, I see that 857cc71985dc
> ("s390x/pci: merge msix init functions") actually makes this mandatory
> and states that nothing changes for passthrough. Has anything changed
> regarding msi-x in the architecture in the meantime?

I don't believe anything has changed from the architecture or kernel 
support perspective (Maybe I'm wrong about the latter; Niklas feel free 
to correct me in that case) -- Otherwise, as far as I can, tell the 
statement from 857cc71985dc was just incorrect and the restriction was 
placed unnecessarily for passthrough.  vfio-pci sets up for both, and 
our base s390 kernel support allows for both (and I can drive ISM 
traffic and see interrupts being routed to the guest with these patches 
applied of course); it's just our qemu emulation support that only sets 
up for MSI-X.