Re: [PATCH for-9.2 v15 04/11] s390x/pci: Check for multifunction after device realization

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Sep 11, 2024 at 07:58:15PM +0900, Akihiko Odaki wrote:
> On 2024/09/11 18:38, Cédric Le Goater wrote:
> > +Matthew +Eric
> > 
> > Side note for the maintainers :
> > 
> > Before this change, the igb device, which is multifunction, was working
> > fine under Linux.
> > 
> > Was there a fix in Linux since :
> > 
> >    57da367b9ec4 ("s390x/pci: forbid multifunction pci device")
> >    6069bcdeacee ("s390x/pci: Move some hotplug checks to the pre_plug
> > handler")
> > 
> > ?
> > 
> > s390 PCI devices do not have extended capabilities, so the igb device
> > does not expose the SRIOV capability and only the PF is accessible but
> > it doesn't seem to be an issue. (Btw, CONFIG_PCI_IOV is set to y in the
> > default Linux config which is unexpected)
> 
> Doesn't s390x really see extended capabilities? hw/s390x/s390-pci-inst.c has
> a call pci_config_size() and pci_host_config_write_common(), which means it
> is exposing the whole PCI Express configuration space. Why can't s390x use
> extended capabilities then?
> 
> The best option for fix would be to replace the SR-IOV implementation with
> stub if s390x cannot use the SR-IOV capability. However I still need to know
> at what level I should change the implementation (e.g., is it fine to remove
> the entire capability, or should I keep the capability while writes to its
> registers no-op?)
> 
> Regards,
> Akihiko Odaki

Note changing caps needs compat hacks for cross version migration to work.

> > 
> > Thanks,
> > 
> > C.
> > 
> > 
> > 
> > On 8/23/24 07:00, Akihiko Odaki wrote:
> > > The SR-IOV PFs set the multifunction bits during device realization so
> > > check them after that. This forbids adding SR-IOV devices to s390x.
> > > 
> > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > > ---
> > >   hw/s390x/s390-pci-bus.c | 14 ++++++--------
> > >   1 file changed, 6 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> > > index 3e57d5faca18..00b2c1f6157b 100644
> > > --- a/hw/s390x/s390-pci-bus.c
> > > +++ b/hw/s390x/s390-pci-bus.c
> > > @@ -971,14 +971,7 @@ static void
> > > s390_pcihost_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > >                       "this device");
> > >       }
> > > -    if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> > > -        PCIDevice *pdev = PCI_DEVICE(dev);
> > > -
> > > -        if (pdev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) {
> > > -            error_setg(errp, "multifunction not supported in s390");
> > > -            return;
> > > -        }
> > > -    } else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
> > > +    if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
> > >           S390PCIBusDevice *pbdev = S390_PCI_DEVICE(dev);
> > >           if (!s390_pci_alloc_idx(s, pbdev)) {
> > > @@ -1069,6 +1062,11 @@ static void s390_pcihost_plug(HotplugHandler
> > > *hotplug_dev, DeviceState *dev,
> > >       } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> > >           pdev = PCI_DEVICE(dev);
> > > +        if (pdev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) {
> > > +            error_setg(errp, "multifunction not supported in s390");
> > > +            return;
> > > +        }
> > > +
> > >           if (!dev->id) {
> > >               /* In the case the PCI device does not define an id */
> > >               /* we generate one based on the PCI address         */
> > > 
> >