Re: [PATCH v16 04/13] s390x/pci: Avoid creating zpci for VFs

["Cédric Le Goater" <clg@redhat.com>]

Hello Akihiko,

Sorry for the late reply.

On 9/18/24 17:32, Akihiko Odaki wrote:
> On 2024/09/18 17:02, Cédric Le Goater wrote:
>> Hello,
>>
>> On 9/13/24 05:44, Akihiko Odaki wrote:
>>> VFs are automatically created by PF, and creating zpci for them will
>>> result in unexpected usage of fids. Currently QEMU does not support
>>> multifunction for s390x so we don't need zpci for VFs anyway.
>>>
>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>> ---
>>>   hw/s390x/s390-pci-bus.c | 19 +++++++++++++++++--
>>>   1 file changed, 17 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>>> index 3e57d5faca18..1a620f5b2a04 100644
>>> --- a/hw/s390x/s390-pci-bus.c
>>> +++ b/hw/s390x/s390-pci-bus.c
>>> @@ -1080,6 +1080,16 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>>>           pbdev = s390_pci_find_dev_by_target(s, dev->id);
>>>           if (!pbdev) {
>>> +            /*
>>> +             * VFs are automatically created by PF, and creating zpci for them
>>> +             * will result in unexpected usage of fids. Currently QEMU does not
>>> +             * support multifunction for s390x so we don't need zpci for VFs
>>> +             * anyway.
>>> +             */
>>> +            if (pci_is_vf(pdev)) {
>>> +                return;
>>> +            }
>>> +
>>>               pbdev = s390_pci_device_new(s, dev->id, errp);
>>>               if (!pbdev) {
>>>                   return;
>>> @@ -1167,7 +1177,9 @@ static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
>>>           int32_t devfn;
>>>           pbdev = s390_pci_find_dev_by_pci(s, PCI_DEVICE(dev));
>>> -        g_assert(pbdev);
>>> +        if (!pbdev) {
>>> +            return;
>>> +        }
>>
>>
>> I don't understand this change. Could you please explain ?
> 
> We need to tolerate that pbdev being NULL because VFs do no longer have zpci and pbdev will be NULL for them.

Then, I think we should extend the assert with a check on pci_is_vf(pdev)
to be symmetric with the plug handler and also, use the 'Error**' parameter
to report an error.

Thanks,

C.




> 
> Regards,
> Akihiko Odaki
>