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

[PATCH v17 02/14] hw/ppc/spapr_pci: Do not create DT for disabled PCI device

Akihiko Odaki posted 14 patches 1 year, 3 months ago
There is a newer version of this series
[PATCH v17 02/14] hw/ppc/spapr_pci: Do not create DT for disabled PCI device
Posted by Akihiko Odaki 1 year, 3 months ago 
Disabled means it is a disabled SR-IOV VF and hidden from the guest.
Do not create DT when starting the system and also keep the disabled PCI
device not linked to DRC, which generates DT in case of hotplug.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 hw/ppc/spapr_pci.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 5c0024bef9c4..679a22fe4e79 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1291,8 +1291,7 @@ static void spapr_dt_pci_device_cb(PCIBus *bus, PCIDevice *pdev,
     PciWalkFdt *p = opaque;
     int err;
 
-    if (p->err) {
-        /* Something's already broken, don't keep going */
+    if (p->err || !pdev->enabled) {
         return;
     }
 
@@ -1592,10 +1591,10 @@ static void spapr_pci_plug(HotplugHandler *plug_handler,
     uint32_t slotnr = PCI_SLOT(pdev->devfn);
 
     /*
-     * If DR is disabled we don't need to do anything in the case of
-     * hotplug or coldplug callbacks.
+     * If DR or the PCI device is disabled we don't need to do anything
+     * in the case of hotplug or coldplug callbacks.
      */
-    if (!phb->dr_enabled) {
+    if (!phb->dr_enabled || !pdev->enabled) {
         return;
     }
 
@@ -1680,6 +1679,11 @@ static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
     }
 
     g_assert(drc);
+
+    if (!drc->dev) {
+        return;
+    }
+
     g_assert(drc->dev == plugged_dev);
 
     if (!spapr_drc_unplug_requested(drc)) {

-- 
2.47.0
Re: [PATCH v17 02/14] hw/ppc/spapr_pci: Do not create DT for disabled PCI device
Posted by Shivaprasad G Bhat 1 year, 3 months ago 
On 10/22/24 2:06 PM, Akihiko Odaki wrote:
> Disabled means it is a disabled SR-IOV VF and hidden from the guest.
> Do not create DT when starting the system and also keep the disabled PCI
> device not linked to DRC, which generates DT in case of hotplug.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>   hw/ppc/spapr_pci.c | 14 +++++++++-----
>   1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 5c0024bef9c4..679a22fe4e79 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1291,8 +1291,7 @@ static void spapr_dt_pci_device_cb(PCIBus *bus, PCIDevice *pdev,
>       PciWalkFdt *p = opaque;
>       int err;
>   
> -    if (p->err) {
> -        /* Something's already broken, don't keep going */
> +    if (p->err || !pdev->enabled) {
>           return;
>       }
>   
> @@ -1592,10 +1591,10 @@ static void spapr_pci_plug(HotplugHandler *plug_handler,
>       uint32_t slotnr = PCI_SLOT(pdev->devfn);
>   
>       /*
> -     * If DR is disabled we don't need to do anything in the case of
> -     * hotplug or coldplug callbacks.
> +     * If DR or the PCI device is disabled we don't need to do anything
> +     * in the case of hotplug or coldplug callbacks.
>        */
> -    if (!phb->dr_enabled) {
> +    if (!phb->dr_enabled || !pdev->enabled) {
>           return;
>       }

Thank you. This is the right place to be called from the hotplug handler

instead of the spapr_pci_dt_populate() unlike I mentioned before..


>   
> @@ -1680,6 +1679,11 @@ static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
>       }
>   
>       g_assert(drc);
> +
> +    if (!drc->dev) {
> +        return;

I agree with the change here, but were you able to get to this path? I 
don't see

  this if condition being entered with,

qemu-system-ppc64 -nographic  -serial none -device 
spapr-pci-host-bridge,index=4,id=pci.1  -device igb,id=igb0 <<< 
'device_del igb0'


Regards,

Shivaprasad

> +    }
> +
>       g_assert(drc->dev == plugged_dev);
>   
>       if (!spapr_drc_unplug_requested(drc)) {
>
Re: [PATCH v17 02/14] hw/ppc/spapr_pci: Do not create DT for disabled PCI device
Posted by Akihiko Odaki 1 year, 3 months ago 
On 2024/10/28 12:08, Shivaprasad G Bhat wrote:
> 
> On 10/22/24 2:06 PM, Akihiko Odaki wrote:
>> Disabled means it is a disabled SR-IOV VF and hidden from the guest.
>> Do not create DT when starting the system and also keep the disabled PCI
>> device not linked to DRC, which generates DT in case of hotplug.
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>>   hw/ppc/spapr_pci.c | 14 +++++++++-----
>>   1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index 5c0024bef9c4..679a22fe4e79 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -1291,8 +1291,7 @@ static void spapr_dt_pci_device_cb(PCIBus *bus, 
>> PCIDevice *pdev,
>>       PciWalkFdt *p = opaque;
>>       int err;
>> -    if (p->err) {
>> -        /* Something's already broken, don't keep going */
>> +    if (p->err || !pdev->enabled) {
>>           return;
>>       }
>> @@ -1592,10 +1591,10 @@ static void spapr_pci_plug(HotplugHandler 
>> *plug_handler,
>>       uint32_t slotnr = PCI_SLOT(pdev->devfn);
>>       /*
>> -     * If DR is disabled we don't need to do anything in the case of
>> -     * hotplug or coldplug callbacks.
>> +     * If DR or the PCI device is disabled we don't need to do anything
>> +     * in the case of hotplug or coldplug callbacks.
>>        */
>> -    if (!phb->dr_enabled) {
>> +    if (!phb->dr_enabled || !pdev->enabled) {
>>           return;
>>       }
> 
> Thank you. This is the right place to be called from the hotplug handler
> 
> instead of the spapr_pci_dt_populate() unlike I mentioned before..
> 
> 
>> @@ -1680,6 +1679,11 @@ static void 
>> spapr_pci_unplug_request(HotplugHandler *plug_handler,
>>       }
>>       g_assert(drc);
>> +
>> +    if (!drc->dev) {
>> +        return;
> 
> I agree with the change here, but were you able to get to this path? I 
> don't see
> 
>   this if condition being entered with,
> 
> qemu-system-ppc64 -nographic  -serial none -device spapr-pci-host- 
> bridge,index=4,id=pci.1  -device igb,id=igb0 <<< 'device_del igb0'

No. VFs bypass the hotplug path when unplugging. For context, see 
unparent_vfs() in "[PATCH v17 09/14] pcie_sriov: Reuse SR-IOV VF device 
instances.

Regards,
Akihiko Odaki

> 
> 
> Regards,
> 
> Shivaprasad
> 
>> +    }
>> +
>>       g_assert(drc->dev == plugged_dev);
>>       if (!spapr_drc_unplug_requested(drc)) {
>>
Re: [PATCH v17 02/14] hw/ppc/spapr_pci: Do not create DT for disabled PCI device
Posted by Shivaprasad G Bhat 1 year, 3 months ago 
On 10/28/24 11:28 AM, Akihiko Odaki wrote:
> On 2024/10/28 12:08, Shivaprasad G Bhat wrote:
>>
>> On 10/22/24 2:06 PM, Akihiko Odaki wrote:
>>> Disabled means it is a disabled SR-IOV VF and hidden from the guest.
>>> Do not create DT when starting the system and also keep the disabled 
>>> PCI
>>> device not linked to DRC, which generates DT in case of hotplug.
>>>
>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>> ---
>>>   hw/ppc/spapr_pci.c | 14 +++++++++-----
>>>   1 file changed, 9 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>>> index 5c0024bef9c4..679a22fe4e79 100644
>>> --- a/hw/ppc/spapr_pci.c
>>> +++ b/hw/ppc/spapr_pci.c
>>> @@ -1291,8 +1291,7 @@ static void spapr_dt_pci_device_cb(PCIBus 
>>> *bus, PCIDevice *pdev,
>>>       PciWalkFdt *p = opaque;
>>>       int err;
>>> -    if (p->err) {
>>> -        /* Something's already broken, don't keep going */
>>> +    if (p->err || !pdev->enabled) {
>>>           return;
>>>       }
>>> @@ -1592,10 +1591,10 @@ static void spapr_pci_plug(HotplugHandler 
>>> *plug_handler,
>>>       uint32_t slotnr = PCI_SLOT(pdev->devfn);
>>>       /*
>>> -     * If DR is disabled we don't need to do anything in the case of
>>> -     * hotplug or coldplug callbacks.
>>> +     * If DR or the PCI device is disabled we don't need to do 
>>> anything
>>> +     * in the case of hotplug or coldplug callbacks.
>>>        */
>>> -    if (!phb->dr_enabled) {
>>> +    if (!phb->dr_enabled || !pdev->enabled) {
>>>           return;
>>>       }
>>
>> Thank you. This is the right place to be called from the hotplug handler
>>
>> instead of the spapr_pci_dt_populate() unlike I mentioned before..
>>
>>
>>> @@ -1680,6 +1679,11 @@ static void 
>>> spapr_pci_unplug_request(HotplugHandler *plug_handler,
>>>       }
>>>       g_assert(drc);
>>> +
>>> +    if (!drc->dev) {
>>> +        return;
>>
>> I agree with the change here, but were you able to get to this path? 
>> I don't see
>>
>>   this if condition being entered with,
>>
>> qemu-system-ppc64 -nographic  -serial none -device spapr-pci-host- 
>> bridge,index=4,id=pci.1  -device igb,id=igb0 <<< 'device_del igb0'
>
> No. VFs bypass the hotplug path when unplugging. For context, see 
> unparent_vfs() in "[PATCH v17 09/14] pcie_sriov: Reuse SR-IOV VF 
> device instances.
>
Thanks. Looks good to me.

Reviewed-by: Shivaprasad G Bhat<sbhat@linux.ibm.com>
Tested-by: Shivaprasad G Bhat<sbhat@linux.ibm.com>


Regards,

Shivaprasad

> Regards,
> Akihiko Odaki
>
>>
>>
>> Regards,
>>
>> Shivaprasad
>>
>>> +    }
>>> +
>>>       g_assert(drc->dev == plugged_dev);
>>>       if (!spapr_drc_unplug_requested(drc)) {
>>>
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.