Re: [Qemu-devel] Hot-pluggable device without ->unrealize() is highly suspect

[Markus Armbruster]

Juan, David, there's a migration bit for you towards the end.

Peter Maydell <address@hidden> writes:

> On Thu, 29 Nov 2018 at 19:03, Markus Armbruster <address@hidden> wrote:
>>
>> Li Qiang <address@hidden> writes:
>>
>> > Currently the dc390 device has no unrealize function. This
>> > can cause memory leak when hotplug/unplug device. Also more
>> > serious, it will trigger an assert when rehotplug.
>> [...]
>>
>> When a hot-pluggable device doesn't have an ->unrealize() method, unplug
>> is probably broken.  I think we should track down such devices for
>> review.  Any takers?
>
> Add an assert somewhere and catch it with the usual
> "instantiate everything" qtest?

Not as easy as it sounds.

For unplug to work, the device and all its parents in the QOM type
hierarchy up to "device" have to undo their ->realize() in their
->unrealize().

Example: "dc390" -> "am53c974" -> "pci-device" -> "device"

* device: does not implement DeviceClass::realize(), nothing to do.

* pci_device: implements DeviceClass::realize(), ::unrealize() as
  pci_qdev_realize(), pci_qdev_unrealize().  They call
  PCIDeviceClass::realize() and PCIDeviceClass::exit(), respectively.

* am53c974: implements PCIDeviceClass::realize(), ::unrealize() as
  esp_pci_scsi_realize() and esp_pci_scsi_uninit().

* dc390: implements PCIDeviceClass::realize() as dc390_scsi_realize(),
  overriding esp_pci_scsi_realize().  dc390_scsi_realize() calls
  esp_pci_scsi_realize().  Bug: it fails to overide
  esp_pci_scsi_uninit().

Three patterns:

(1) A class implements whatever realize interface its parent class
    demands.  Example: am53c974.

(2) A (generally abstract) class implements ::realize() and
    ::unrealize(), and its implementation calls methods its children may
    implement.  Example: pci_device.

(3) A class overrides its parent's implementation, and calls it.
    Example: dc390.

How can we assert any realize-like thing has a matching unrealize-like
thing?

Any class that calls realize-/unrealize-like methods its children may
implement needs to assert the child implements both or none.

The troublemaker is (3), where we may end up with an overridden
realize-like method and an non-matching, un-overridden unrealize-like
method.  Got any smart ideas?

> More generally, what is causing dc390 to be hotpluggable?
> I can't see anything obvious in the class init.

It's a PCI device.  These are all hot-pluggable by default.

>                                                 If we
> have APIs where the default is "you get this weird thing
> you weren't thinking about but it's broken because
> you weren't thinking about it" then we will have a whole
> class of bugs that we then need to squash device by device[*].
> I think it would be better for devices to have to explicitly
> opt in to implementing things like hotplug -- that way the
> failure mode is just "this isn't hotpluggable", we can
> report that helpfully to the user, and if anybody cares
> they can add the support.

I tend to agree, although for PCI devices, not being hot-pluggable is
kind of weird, except for the ones that only occur soldered onto
mainboards.

> [*] Also currently true for migration support. We should
> require devices to either provide a VMState or explicitly
> say they have no state needing migration or explicitly
> say they don't support migration, and then assert that
> they do one or the other of those, rather than having the
> default be "we'll allow migration but not migrate any
> of the device's state".

True.  Cc: Juan & Dave.