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

[Shivaprasad G Bhat <sbhat@linux.ibm.com>]

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