Re: hw/qdev: Can qdev_unrealize() ever fail?

[Akihiko Odaki]

On 2024/02/12 17:35, Philippe Mathieu-Daudé wrote:
> Hi,
>
> QDev base class doesn't expect UNREALIZE to fail, and this
> handler is only recommended for hot-plug devices:
>
> /**
>   * qdev_unrealize: Unrealize a device
>   * @dev: device to unrealize
>   *
>   * Warning: most devices in QEMU do not expect to be unrealized. Only
>   * devices which are hot-unpluggable should be unrealized (as part of
>   * the unplugging process); all other devices are expected to last for
>   * the life of the simulation and should not be unrealized and freed.
>   */
>
>
>    void qdev_unrealize(DeviceState *dev)
>    {
>        object_property_set_bool(OBJECT(dev), "realized",
>                                 false, &error_abort);
>                                        ^^^^^^^^^^^^
>    }
>
>    static void device_unparent(Object *obj)
>    {
>        DeviceState *dev = DEVICE(obj);
>        BusState *bus;
>
>        if (dev->realized) {
>            qdev_unrealize(dev);
>        }
>        while (dev->num_child_bus) {
>            bus = QLIST_FIRST(&dev->child_bus);
>            object_unparent(OBJECT(bus));
>        }
>        if (dev->parent_bus) {
>            bus_remove_child(dev->parent_bus, dev);
>            object_unref(OBJECT(dev->parent_bus));
>            dev->parent_bus = NULL;
>        }
>    }
>
> Now apparently some devices expect failures, see commit 7c0fa8dff8
> ("pcie: Add support for Single Root I/O Virtualization (SR/IOV)"):
>
>    static void unregister_vfs(PCIDevice *dev)
>    {
>        uint16_t num_vfs = dev->exp.sriov_pf.num_vfs;
>        uint16_t i;
>
>        for (i = 0; i < num_vfs; i++) {
>            Error *err = NULL;
>            PCIDevice *vf = dev->exp.sriov_pf.vf[i];
>            if (!object_property_set_bool(OBJECT(vf), "realized",
>                                          false, &err)) {
>                                                 ^^^^
>                error_reportf_err(err, "Failed to unplug: ");
>            }
>            object_unparent(OBJECT(vf));
>            object_unref(OBJECT(vf));
>        }
>        ...
>    }
>
> (Note the failure path only emits a warning).
>
> So instead of calling the QDev unrealize layer, this function is
> calling the lower layer, QOM, bypassing the class handlers, leading
> to further cleanups such commit 08f6328480 ("pcie: Release references
> of virtual functions") or recent patch
> 20240210-reuse-v2-6-24ba2a502692@daynix.com/">https://lore.kernel.org/qemu-devel/20240210-reuse-v2-6-24ba2a502692@daynix.com/.
>
> I couldn't find any explicit possible failure in:
>   pci_qdev_unrealize()
>   do_pci_unregister_device()
>   pci_bus_unrealize()
> so, what is the failure unregister_vfs() is trying to recover from?
When unrealizing, device_set_realized() is only used to report that the 
device is not hotpluggable.
> I understand if a device is in a odd state, the kernel could reject
> an unplug request. If so, how to deal with that cleanly?
The guest kernel requests to unregister VFs so QEMU shouldn't ask it if 
they should be unplugged again. I think that's why unrealize_vfs() 
doesn't use qdev_unplug().
In any case, with "[PATCH v2 5/6] pcie_sriov: Reuse SR-IOV VF device 
instances", the runtime realization/unrealization code is replaced with 
PCI-specific device enablement code, which derives from PCI power down 
logic. While the patch was written to deal with realization errors, it 
also eliminates the need to unrealize VFs at runtime.
Regards,
Akihiko Odaki