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