1. Patchew
  2. QEMU
  3. [PATCH v16 00/13] hw/pci: SR-IOV related fixes and improvements
  4. View patch

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

Akihiko Odaki posted 13 patches 2 months, 1 week ago
There is a newer version of this series
[PATCH v16 04/13] s390x/pci: Avoid creating zpci for VFs
Posted by Akihiko Odaki 2 months, 1 week ago 
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;
+        }
 
         s390_pci_generate_plug_event(HP_EVENT_STANDBY_TO_RESERVED,
                                      pbdev->fh, pbdev->fid);
@@ -1206,7 +1218,10 @@ static void s390_pcihost_unplug_request(HotplugHandler *hotplug_dev,
          * we've checked the PCI device already (to prevent endless recursion).
          */
         pbdev = s390_pci_find_dev_by_pci(s, PCI_DEVICE(dev));
-        g_assert(pbdev);
+        if (!pbdev) {
+            return;
+        }
+
         pbdev->pci_unplug_request_processed = true;
         qdev_unplug(DEVICE(pbdev), errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {

-- 
2.46.0
Re: [PATCH v16 04/13] s390x/pci: Avoid creating zpci for VFs
Posted by Cédric Le Goater 2 months ago 
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 ?


Thanks,

C.



>           s390_pci_generate_plug_event(HP_EVENT_STANDBY_TO_RESERVED,
>                                        pbdev->fh, pbdev->fid);
> @@ -1206,7 +1218,10 @@ static void s390_pcihost_unplug_request(HotplugHandler *hotplug_dev,
>            * we've checked the PCI device already (to prevent endless recursion).
>            */
>           pbdev = s390_pci_find_dev_by_pci(s, PCI_DEVICE(dev));
> -        g_assert(pbdev);
> +        if (!pbdev) {
> +            return;
> +        }
> +
>           pbdev->pci_unplug_request_processed = true;
>           qdev_unplug(DEVICE(pbdev), errp);
>       } else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
>
Re: [PATCH v16 04/13] s390x/pci: Avoid creating zpci for VFs
Posted by Akihiko Odaki 2 months ago 
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.

Regards,
Akihiko Odaki
Re: [PATCH v16 04/13] s390x/pci: Avoid creating zpci for VFs
Posted by Cédric Le Goater 1 month, 2 weeks ago 
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
>
Re: [PATCH v16 04/13] s390x/pci: Avoid creating zpci for VFs
Posted by Akihiko Odaki 1 month, 1 week ago 
On 2024/10/11 0:44, Cédric Le Goater wrote:
> 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.

This should never happen unless there is a programming error so plain 
g_assert() without error reporting should be fine. We don't need to 
report an error when it is VF; we just don't have a work to do and 
nothing wrong happens here.

Regards,
Akihiko Odaki

> 
> Thanks,
> 
> C.
> 
> 
> 
> 
>>
>> Regards,
>> Akihiko Odaki
>>
>
Re: [PATCH v16 04/13] s390x/pci: Avoid creating zpci for VFs
Posted by Cédric Le Goater 1 month, 1 week ago 
Hello Akihiko,

On 10/12/24 13:05, Akihiko Odaki wrote:
> On 2024/10/11 0:44, Cédric Le Goater wrote:
>> 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.
> 
> This should never happen unless there is a programming error so plain g_assert() without error reporting should be fine. We don't need to report an error when it is VF; we just don't have a work to do and nothing wrong happens here.

unplugging a VF is still an invalid thing to do, reporting an error is preferable IMO.

Thanks,

C.
Re: [PATCH v16 04/13] s390x/pci: Avoid creating zpci for VFs
Posted by Akihiko Odaki 1 month ago 
On 2024/10/14 17:43, Cédric Le Goater wrote:
> Hello Akihiko,
> 
> On 10/12/24 13:05, Akihiko Odaki wrote:
>> On 2024/10/11 0:44, Cédric Le Goater wrote:
>>> 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.
>>
>> This should never happen unless there is a programming error so plain 
>> g_assert() without error reporting should be fine. We don't need to 
>> report an error when it is VF; we just don't have a work to do and 
>> nothing wrong happens here.
> 
> unplugging a VF is still an invalid thing to do, reporting an error is 
> preferable IMO.

Unplugging a VF will happen if you unplug igb, and it is a valid operation.

Regards,
Akihiko Odaki

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.