From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=47829 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PDh9O-00035F-OI for qemu-devel@nongnu.org; Wed, 03 Nov 2010 13:29:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PDh9E-00055j-BH for qemu-devel@nongnu.org; Wed, 03 Nov 2010 13:29:28 -0400 Received: from e4.ny.us.ibm.com ([32.97.182.144]:44843) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PDh9E-00055Z-0O for qemu-devel@nongnu.org; Wed, 03 Nov 2010 13:29:20 -0400 Received: from d01relay02.pok.ibm.com (d01relay02.pok.ibm.com [9.56.227.234]) by e4.ny.us.ibm.com (8.14.4/8.13.1) with ESMTP id oA3HCtiQ028794 for ; Wed, 3 Nov 2010 13:12:55 -0400 Received: from d03av02.boulder.ibm.com (d03av02.boulder.ibm.com [9.17.195.168]) by d01relay02.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id oA3HTHQO395070 for ; Wed, 3 Nov 2010 13:29:18 -0400 Received: from d03av02.boulder.ibm.com (loopback [127.0.0.1]) by d03av02.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id oA3HTHCt031684 for ; Wed, 3 Nov 2010 11:29:17 -0600 Date: Wed, 3 Nov 2010 12:29:10 -0500 From: Ryan Harper Subject: Re: [Qemu-devel] [PATCH 0/3] v4 Decouple block device removal from device removal Message-ID: <20101103172910.GT3469@us.ibm.com> References: <20101102142201.GH22904@us.ibm.com> <20101102154615.GB32448@redhat.com> <20101102165339.GK22904@us.ibm.com> <20101102175922.GA1939@redhat.com> <20101102190108.GA3469@us.ibm.com> <20101102191749.GD2744@redhat.com> <20101102202338.GB3469@us.ibm.com> <20101103072134.GA6772@redhat.com> <20101103120443.GD3469@us.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Kevin Wolf , "Michael S. Tsirkin" , qemu-devel@nongnu.org, Anthony Liguori , Ryan Harper , Stefan Hajnoczi , yamahata@valinux.co.jp * Markus Armbruster [2010-11-03 11:42]: > Ryan Harper writes: > > > * Michael S. Tsirkin [2010-11-03 02:22]: > >> On Tue, Nov 02, 2010 at 03:23:38PM -0500, Ryan Harper wrote: > >> > * Michael S. Tsirkin [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. You were asking for net and block disconnect to have similar mechanisms. You mentioned the net fix for suprise removal was to have device_del() always succeed by replacing the device with a shell/zombie. The drive_unplug() patch doesn't do the same thing; it doesn't affect the device_del() path at all, rather it provides mgmt apps a hook to directly disconnect host resource from guest resource. > > > 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? Yeah, aboslutely. I think Michael was saying we should implement 2b in the mgmt stack. The current libvirt does the following 1. mgmt invokes detach-device 2. device_del 3. update mgmt view of resources, assumes guest has done it's part; does not confirm with qemu that device has been deleted. With drive_unplug in qemu and a patch to libvirt, it looks like: 1. mgmt invokes detach-device 2a. call drive_unplug, log warning if drive_unplug isn't available 2b. device_del 3. update mgmt view of resources, assumes guest has done it's part; does not confirm with qemu that device has been deleted. I can look at implementing the timeout before invoking the unplug (that's a bit tricky) in libvirt; but given the fact that the mgmt is invoking the removal I think it's reasonable to do forced disconnect (even if the guest hasn't responded). > > > 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. > I like the latter here; the BlockDriverState handles nulls. I think netdev should be able to as well though I haven't looked very closely though so maybe Michael can confirm if that's a true statement. > We might need to notify the device model (new DeviceInfo method). > Dunno. -- Ryan Harper Software Engineer; Linux Technology Center IBM Corp., Austin, Tx ryanh@us.ibm.com