Re: [Qemu-devel] [PATCH 0/3] add usb_detach and usb_attach (v3)

[Markus Armbruster]

Alon Levy <address@hidden> writes:

> On Wed, Nov 10, 2010 at 04:49:38PM +0100, Markus Armbruster wrote:
>> Sorry for coming so late to this thread...
>> 
>> Alon Levy <address@hidden> writes:
>> 
>> > On Thu, Oct 21, 2010 at 08:13:19AM -0500, Anthony Liguori wrote:
>> >> On 10/21/2010 08:03 AM, Gerd Hoffmann wrote:
>> >> >On 10/21/10 08:36, Alon Levy wrote:
>> >> >>v2->v3 changes:
>> >> >>  * add configure parameter
>> >> >>  * fix docs
>> >> >>
>> >> >>v2 message:
>> >> >>This patchset uses id like device_del for attaching/detaching usb
>> >> >>devices. The first two patches ready the way:
>> >> >>  1. makes qdev_find_recursive non static and in qdev.h
>> >> >>  2. adds a usb_device_by_id which goes over the usb buses calling
>> >> >>   qdev_find_recursive
>> >> >>  3. adds the commands that use usb_device_by_id
>> >> >>
>> >> >>Alon Levy (3):
>> >> >>   qdev: make qdev_find_recursive public
>> >> >>   usb: add public usb_device_by_id
>> >> >>   monitor: add usb_attach and usb_detach (v2)
>> >> >>
>> >> >
>> >> >Acked-by: Gerd Hoffmann <address@hidden>
>> >> 
>> >> Okay, I am still confused about the use-case for this and I don't
>> >> see any further explanation in the commit messages.  I've seen
>> >> "debugging" but can you be a bit more specific about which cases
>> >> it's needed for?
>> >> 
>> >
>> > I use it for debugging the usb-ccid device. I think it's useful for
>> > any other usb device tests as well. The existing commands are not
>> > good enough to do a remove/insert of a usb device, since deleting
>> > a device also deletes any chardev associated with it, and there is
>> > no monitor command to add a chardev. Also sometimes you don't want
>> > to close the chardev, just have the guest see a removal/reinsert of
>> > the device.
>> [...]
>> 
>> Let's see whether I get you: detach removes the device, but doesn't
>> destroy it.  The only thing you can do with a detached device is attach
>> it.  Detach+attach is basically the same as del+add with the same
>> configuration.  Except shortcomings in our command set make it
>> impossible to recreate the configuration sometimes.  Correct?
> So the problems with the current commands from my pov:
>  - device deletion removes associated chardev
>   - no way to do it without removing chardev
>   - no way to add chardev later and use it for device add
> The outcome of which is that you can't do a guest wise attach/detach
> from monitor if your device relies on a chardev association. This
> happens with my passthrough ccid device.

Commands chardev_add, chardev_del look feasible to me.

I hate device_del destroying chardevs automatically.  If it was created
separately, it should be destroyed separately.  But any fix needs to be
backwards compatible somehow.  How to do that without embarrassingly
ugly warts isn't obvious to me.

>> Questions:
>> 
>> 1. If we add commands so that you can always recreate the configuration,
>>    is detach+attach still useful?  Why?
> If you make it so you can do a device_del and not remove the chardev, and
> later device_add using the already existing chardev, then that will be
> equivalent for me.

Would chardev_add suffice, or do you need a way to reuse the existing
chardev?

>> 2. Why is this a USB problem, and not a general problem?  In other
>>    words, why usb_{detach,attach}, and not device_{detach,attach}?
> I guess attach/detach is a don't-free-some-resources del/add. If you
> think there are users for a device_attach/detach and it makes sense
> conceptually (what's a detach/attach for an ide bus? for a pci it's
> pretty clear, for sata, etc.) then you could blow this up to a device
> specific callback or something like that (assuming that's how you
> would implement this).

For buses that don't support hot plug, such as IDE, detach makes as much
sense as delete: none.

For buses that do (USB, PCI, SCSI, virtio-serial-bus), detach looks like
the first half of delete to me: shut down, remove from device tree
(second half is destroying the device object).

Likewise, attach looks like the second have of add: insert into device
tree, start up (first half is creating the device object).

Pitfall: to make re-attach work, qdev method init() needs to work not
just for newly created objects, but after a qdev exit() as well.  This
is a change of contract for these two methods.  I wouldn't be surprised
if not all of our device were happy with that.