Re: [Qemu-devel] [PATCH 0/3] v4 Decouple block device removal from device removal

[Markus Armbruster]

Ryan Harper <address@hidden> writes:

> * Michael S. Tsirkin <address@hidden> [2010-11-03 02:22]:
>> On Tue, Nov 02, 2010 at 03:23:38PM -0500, Ryan Harper wrote:
>> > * Michael S. Tsirkin <address@hidden> [2010-11-02 14:18]:
>> > > On Tue, Nov 02, 2010 at 02:01:08PM -0500, Ryan Harper wrote:
>> > > > > > > > I like the idea of disconnect; if part of the device_del 
>> > > > > > > > method was to
>> > > > > > > > invoke a disconnect method, we could implement that for block, 
>> > > > > > > > net, etc;
>> > > > > > > > 
>> > > > > > > > I'd think we'd want to send the notification, then disconnect.
>> > > > > > > > Struggling with whether it's worth having some reasonable 
>> > > > > > > > timeout
>> > > > > > > > between notification and disconnect.  
>> > > > > > > 
>> > > > > > > The problem with this is that it has no analog in real world.
>> > > > > > > In real world, you can send some notifications to the guest, and 
>> > > > > > > you can
>> > > > > > > remove the card.  Tying them together is what created the 
>> > > > > > > problem in the
>> > > > > > > first place.
>> > > > > > > 
>> > > > > > > Timeouts can be implemented by management, maybe with a nice 
>> > > > > > > dialog
>> > > > > > > being shown to the user.
>> > > > > > 
>> > > > > > Very true.  I'm fine with forcing a disconnect during the removal 
>> > > > > > path
>> > > > > > prior to notification.  Do we want a new disconnect method at the 
>> > > > > > device
>> > > > > > level (pci)? or just use the existing removal callback and call 
>> > > > > > that
>> > > > > > during the initial hotremov event?
>> > > > > 
>> > > > > Not sure what you mean by that, but I don't see a device doing 
>> > > > > anything
>> > > > > differently wrt surprise or ordered removal. So probably the existing
>> > > > > callback should do. I don't think we need to talk about disconnect:
>> > > > > since we decided we are emulating device removal, let's call it
>> > > > > just that.
>> > > > 
>> > > > Because current the "removal" process depends on the guest actually
>> > > > responding.  What I'm suggesting is that, in Marcus's term, and what
>> > > > drive_unplug() implements, is to disconnect the host block device from
>> > > > the guest device to prevent any further access to it in the case the
>> > > > guest doesn't respond to the removal request made via ACPI.
>> > > > 
>> > > > Very specifically, what we're suggesting instead of the drive_unplug()
>> > > > command so to complete the device removal operation without waiting for
>> > > > the guest to respond; that's what's going to happen if we invoke the
>> > > > response callback; it will appear as if the guest responded whether it
>> > > > did or not.
>> > > > 
>> > > > What I was suggesting above was to instead of calling the callback for
>> > > > handing the guest response was to add a device function called
>> > > > disconnect which would remove any association of host resources from
>> > > > guest resources before we notified the guest.  Thinking about it again
>> > > > I'm not sure this is useful, but if we're going to remove the device
>> > > > without the guests knowledge, I'm not sure how useful sending the
>> > > > removal requests via ACPI is in the first place.
>> > > > 
>> > > > My feeling is that I'd like to have explicit control over the 
>> > > > disconnect
>> > > > from host resources separate from the device removal *if* we're going 
>> > > > to
>> > > > retain the guest notification.  If we don't care to notify the guest,
>> > > > then we can just do device removal without notifying the guest
>> > > > and be done with it.
>> > > 
>> > > I imagine management would typically want to do this:
>> > > 1. notify guest
>> > > 2. wait a bit
>> > > 3. remove device
>> > 
>> > Yes; but this argues for (1) being a separate command from (3)
>> 
>> Yes. Long term I think we will want a way to do that.
>> 
>> > unless we
>> > require (3) to include (1) and (2) in the qemu implementation.
>> > 
>> > Currently we implement:
>> > 
>> > 1. device_del (attempt to remove device)
>> > 2. notify guest
>> > 3. if guest responds, remove device
>> > 4. disconnect host resource from device on destruction
>> > 
>> > With my drive_unplug patch we do:
>> > 
>> > 1. disconnect host resource from device
>> 
>> This is what drive_unplug does, right?
>
> Correct.
>
>> 
>> > 2. device_del (attempt to remove device)
>> > 3. notify guest
>> > 4. if guest responds, remove device
>> > 
>> > I think we're suggesting to instead do (if we keep disconnect as part of
>> > device_del)
>> > 
>> > 1. device_del (attemp to remove device)
>> > 2. notify guest
>> > 3. invoke device destruction callback resulting in disconnect host 
>> > resource from device
>> > 4. if guest responds, invoke device destruction path a second time.
>> 
>> By response you mean eject?  No, this is not what I was suggesting.
>> I was really suggesting that your patch is fine :)
>> Sorry about confusion.
>
> I don't mean eject; I mean responding to the ACPI event by writing a
> response to the PCI chipset which QEMU then in turn will invoke the
> qdev_unplug() path which ultimately kills the device and the Drive and
> BlockState objects.
>
>> 
>> I was also saying that from what I hear, the pci express support
>> will at some point need interfaces to
>> - notify guest about device removal/addition
>> - get eject from guest
>> - remove device without talking to guest
>> - add device without talking to guest
>> - suppress device deletion on eject
>> 
>> All this can be generic and can work through express
>> configuration mechanisms or through acpi for pci.
>> But this is completely separate from unplugging
>> the host backend, which should be possible at any point.
>
> Yes.  I think we've worked out that we do want an independent
> unplug/disconnect mechanism rather than tying it to device_del.
>
> Marcus, it sounds like then you wanted to see a net_unplug/disconnect
> and that instead of having device_del always succeed and replacing it
> with a shell, we'd need to provide an explicit command to do the
> disconnect in a similar fashion to how we're doing drive_unplug?

I'm not sure I parse this.

> With at least two of these device types needing an explicit disconnect
> to sever the bond between host/guest makes me want a device-level
> interface for doing the disconnect that each device can implement
> differently.

I'm fine with having a separate command to forcibly disconnect a device
from its host resources.

Typical use:

1. device_del
   ask guest to give up device, via ACPI

2a. guest replies "done", delete device, free host resources

2b. timeout, device_disconnect (or however we call that)

Is this what you have in mind?


With qdev, device models are connected to host resources with special
properties such as qdev_prop_netdev and qdev_prop_drive.  Thus, generic
qdev code can already find and disconnect them.

How can we make sure device models survive such a disconnect?

* Ask the device to disconnect itself (new DeviceInfo method).
  Drawback: duplicates common functionality in every device model.
  More code, more bugs.

* Let qdev core disconnect and free host resources

  - and replace them with dummies.  I guess we'd need a dummy
    constructor method for that, in PropertyInfo.  Done right, device
    models should be able to carry on unawares.

  - and leave them null.  Device models need to cope with that.  NICs
    do for netdev.

  We might need to notify the device model (new DeviceInfo method).
  Dunno.