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

[Akihiko Odaki <akihiko.odaki@daynix.com>]

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