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

[Akihiko Odaki]

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

> 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         */