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

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

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)) {
>>