Re: [Qemu-devel] [PATCH 0/3] add usb_detach and usb_attach (v3)
On Thu, Nov 11, 2010 at 04:03:14PM +0100, Markus Armbruster wrote:
[snip]
> Agree.
>
> Summary so far:
>
> 1. usb_{attach,detach} looks like yet another special-purpose command
>where a general command would make sense, namely
>device_{attach,detach}. We have a few of those, e.g. usb_add
>vs. device_add. I'd prefer not to add more, as far as practical.
>
> 2. We need chardev_add and chardev_del sooner rather than later.
>
> 3. Automatic deletion of host devices (character, block, net) gets in
>the way[*]. We need a way to suppress it.
>
> 4. If we have 2. and 3., we don't need 1.
>
> Fair?
Yes.
>
>
> [*] Funny coincidence: I just read Neil Brown's essay on the conflated
> design anti-pattern, http://lwn.net/Articles/412131/
Re: [Qemu-devel] [PATCH 0/3] add usb_detach and usb_attach (v3)
Alon Levy writes:
> On Thu, Nov 11, 2010 at 11:29:36AM +0100, Markus Armbruster wrote:
>> Alon Levy writes:
>>
>> > On Wed, Nov 10, 2010 at 04:49:38PM +0100, Markus Armbruster wrote:
>> >> Sorry for coming so late to this thread...
>> >>
>> >> Alon Levy 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
>> >> >>
>> >> >> 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?
>>
> I'd love chardev_add / chardev_del for testing in general, but they don't
> work for my use case, because chardev_del closes the socket (in my case).
> I could of course fix my client to work with reconnect, but it doesn't make
> this pretty.
Understand.
>> >> 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 wi
Re: [Qemu-devel] [PATCH 0/3] add usb_detach and usb_attach (v3)
On Thu, Nov 11, 2010 at 11:29:36AM +0100, Markus Armbruster wrote:
> Alon Levy writes:
>
> > On Wed, Nov 10, 2010 at 04:49:38PM +0100, Markus Armbruster wrote:
> >> Sorry for coming so late to this thread...
> >>
> >> Alon Levy 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
> >> >>
> >> >> 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?
>
I'd love chardev_add / chardev_del for testing in general, but they don't
work for my use case, because chardev_del closes the socket (in my case).
I could of course fix my client to work with reconnect, but it doesn't make
this pretty.
> >> 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.
>
We could flag which devices can do re-attach. Or you go across the board
and add a info->detach, info->attach, split from info->e
Re: [Qemu-devel] [PATCH 0/3] add usb_detach and usb_attach (v3)
Alon Levy writes:
> On Wed, Nov 10, 2010 at 04:49:38PM +0100, Markus Armbruster wrote:
>> Sorry for coming so late to this thread...
>>
>> Alon Levy 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
>> >>
>> >> 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.
Re: [Qemu-devel] [PATCH 0/3] add usb_detach and usb_attach (v3)
On Wed, Nov 10, 2010 at 04:49:38PM +0100, Markus Armbruster wrote:
> Sorry for coming so late to this thread...
>
> Alon Levy 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
> >>
> >> 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.
>
> 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.
>
> 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).
Re: [Qemu-devel] [PATCH 0/3] add usb_detach and usb_attach (v3)
Sorry for coming so late to this thread...
Alon Levy 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
>>
>> 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?
Questions:
1. If we add commands so that you can always recreate the configuration,
is detach+attach still useful? Why?
2. Why is this a USB problem, and not a general problem? In other
words, why usb_{detach,attach}, and not device_{detach,attach}?
Re: [Qemu-devel] [PATCH 0/3] add usb_detach and usb_attach (v3)
On Fri, Oct 22, 2010 at 07:55:02AM -0500, Anthony Liguori wrote: > On 10/22/2010 07:48 AM, Luiz Capitulino wrote: > >On Thu, 21 Oct 2010 15:27:23 +0200 > >Alon Levy wrote: > > > >>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 > >>>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? > >>To elaborate a little more, when using a certificates based card > >>there is no hardware event (i.e. removing/inserting the physical card) > >>that causes a usb_detach/attach to the card (both in passthru and > >>emulated), but otoh certificates is good for testing since it decouples > >>it from NSS/tcp. So I needed some way to emulate an insert/remove, and > >>I saw usb_del, which was pretty close, and voila. This is not the same > >>as card remove/reinsert, but it is exactly what will happen to the > >>guest when spicec connects/disconnects, since I detach devices on > >>disconnect and attach on connect. > >Looks reasonable to me, specially because this will be protected by > >#ifdef DEBUG. I don't see a big deal in merging this. > > I'd just like to see better documentation. A command isn't useful > for debugging if noone knows how to use it. > > Guarding with an #ifdef isn't necessary. It should be > unconditionally enabled otherwise it will bit rot. > > >Objections, Anthony? > > Not with better docs. > ok, so no QMP and better docs. Where should the docs go? just have more descriptive help messages? or in docs/usb_attach_detach.txt (for example)? > Regards, > > Anthony Liguori > > >>>This is just adding a HMP command. Is that the right approach or > >>>was that an unintentional consequence of rebasing post-HMP/QMP > >>>split? > >I don't think this should be available under QMP, it's more a debugging > >command for USB developers. > > > >>>Regards, > >>> > >>>Anthony Liguori > >>> > cheers, > Gerd > > > >>> >
Re: [Qemu-devel] [PATCH 0/3] add usb_detach and usb_attach (v3)
On Fri, 22 Oct 2010 08:43:39 -0500 Anthony Liguori wrote: > On 10/22/2010 08:17 AM, Luiz Capitulino wrote: > > You and Gerd asked about the purpose of this command, turns out that it's > > only useful for developing new USB devices for QEMU, so I thought it would > > be better to restrict it, so that people don't start using this the > > wrong way or worse, we can't drop/break it because some tool is now using > > it. > > > > Need HMP commands have no support associated with them. Given QMP, > there's no reason to script HMP commands. > > But a long standing policy in QEMU has been to avoid conditional code > because it results in dead code. I think this policy has actually > worked well for us historically. Fine with me.
Re: [Qemu-devel] [PATCH 0/3] add usb_detach and usb_attach (v3)
On 10/22/2010 08:17 AM, Luiz Capitulino wrote: You and Gerd asked about the purpose of this command, turns out that it's only useful for developing new USB devices for QEMU, so I thought it would be better to restrict it, so that people don't start using this the wrong way or worse, we can't drop/break it because some tool is now using it. Need HMP commands have no support associated with them. Given QMP, there's no reason to script HMP commands. But a long standing policy in QEMU has been to avoid conditional code because it results in dead code. I think this policy has actually worked well for us historically. Regards, Anthony Liguori Anyway, maybe a good doc will do, this was just a small suggestion. Objections, Anthony? Not with better docs. Regards, Anthony Liguori This is just adding a HMP command. Is that the right approach or was that an unintentional consequence of rebasing post-HMP/QMP split? I don't think this should be available under QMP, it's more a debugging command for USB developers. Regards, Anthony Liguori cheers, Gerd
Re: [Qemu-devel] [PATCH 0/3] add usb_detach and usb_attach (v3)
On 10/22/2010 07:48 AM, Luiz Capitulino wrote: On Thu, 21 Oct 2010 15:27:23 +0200 Alon Levy wrote: 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 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? To elaborate a little more, when using a certificates based card there is no hardware event (i.e. removing/inserting the physical card) that causes a usb_detach/attach to the card (both in passthru and emulated), but otoh certificates is good for testing since it decouples it from NSS/tcp. So I needed some way to emulate an insert/remove, and I saw usb_del, which was pretty close, and voila. This is not the same as card remove/reinsert, but it is exactly what will happen to the guest when spicec connects/disconnects, since I detach devices on disconnect and attach on connect. Looks reasonable to me, specially because this will be protected by #ifdef DEBUG. I don't see a big deal in merging this. I'd just like to see better documentation. A command isn't useful for debugging if noone knows how to use it. Guarding with an #ifdef isn't necessary. It should be unconditionally enabled otherwise it will bit rot. Objections, Anthony? Not with better docs. Regards, Anthony Liguori This is just adding a HMP command. Is that the right approach or was that an unintentional consequence of rebasing post-HMP/QMP split? I don't think this should be available under QMP, it's more a debugging command for USB developers. Regards, Anthony Liguori cheers, Gerd
Re: [Qemu-devel] [PATCH 0/3] add usb_detach and usb_attach (v3)
On Fri, 22 Oct 2010 07:55:02 -0500 Anthony Liguori wrote: > On 10/22/2010 07:48 AM, Luiz Capitulino wrote: > > On Thu, 21 Oct 2010 15:27:23 +0200 > > Alon Levy wrote: > > > > > >> 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 > > >>> 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? > >>> > >> To elaborate a little more, when using a certificates based card > >> there is no hardware event (i.e. removing/inserting the physical card) > >> that causes a usb_detach/attach to the card (both in passthru and > >> emulated), but otoh certificates is good for testing since it decouples > >> it from NSS/tcp. So I needed some way to emulate an insert/remove, and > >> I saw usb_del, which was pretty close, and voila. This is not the same > >> as card remove/reinsert, but it is exactly what will happen to the > >> guest when spicec connects/disconnects, since I detach devices on > >> disconnect and attach on connect. > >> > > Looks reasonable to me, specially because this will be protected by > > #ifdef DEBUG. I don't see a big deal in merging this. > > > > I'd just like to see better documentation. A command isn't useful for > debugging if noone knows how to use it. > > Guarding with an #ifdef isn't necessary. It should be unconditionally > enabled otherwise it will bit rot. You and Gerd asked about the purpose of this command, turns out that it's only useful for developing new USB devices for QEMU, so I thought it would be better to restrict it, so that people don't start using this the wrong way or worse, we can't drop/break it because some tool is now using it. Anyway, maybe a good doc will do, this was just a small suggestion. > > > Objections, Anthony? > > > > Not with better docs. > > Regards, > > Anthony Liguori > > >>> This is just adding a HMP command. Is that the right approach or > >>> was that an unintentional consequence of rebasing post-HMP/QMP > >>> split? > >>> > > I don't think this should be available under QMP, it's more a debugging > > command for USB developers. > > > > > >>> Regards, > >>> > >>> Anthony Liguori > >>> > >>> > cheers, > Gerd > > > > >>> > >>> > >> > > >
Re: [Qemu-devel] [PATCH 0/3] add usb_detach and usb_attach (v3)
On Thu, 21 Oct 2010 15:27:23 +0200 Alon Levy wrote: > 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 > > > > 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? > > To elaborate a little more, when using a certificates based card > there is no hardware event (i.e. removing/inserting the physical card) > that causes a usb_detach/attach to the card (both in passthru and > emulated), but otoh certificates is good for testing since it decouples > it from NSS/tcp. So I needed some way to emulate an insert/remove, and > I saw usb_del, which was pretty close, and voila. This is not the same > as card remove/reinsert, but it is exactly what will happen to the > guest when spicec connects/disconnects, since I detach devices on > disconnect and attach on connect. Looks reasonable to me, specially because this will be protected by #ifdef DEBUG. I don't see a big deal in merging this. Objections, Anthony? > > This is just adding a HMP command. Is that the right approach or > > was that an unintentional consequence of rebasing post-HMP/QMP > > split? I don't think this should be available under QMP, it's more a debugging command for USB developers. > > > > Regards, > > > > Anthony Liguori > > > > > > > >cheers, > > > Gerd > > > > > > > > > > >
Re: [Qemu-devel] [PATCH 0/3] add usb_detach and usb_attach (v3)
* Alon Levy [2010-10-21 08:26]: > 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 > > > > 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. > > > This is just adding a HMP command. Is that the right approach or > > was that an unintentional consequence of rebasing post-HMP/QMP > > split? > > > > yes, my bad, I have not used qmp before, I can add the code but no > idea how to test it easily. just walked through this one myself; you can update the qmp-command.hx to refer to the same handler (assuming your handler parses qdict etc). Then to test the QMP interface, I read qemu/QMP/README and ended up using a qmp config file and -reconfig, and then qemu/QMP/qmp-shell Which gives you a HMP like monitor interface where you can execute QMP commands. -- Ryan Harper Software Engineer; Linux Technology Center IBM Corp., Austin, Tx ry...@us.ibm.com
Re: [Qemu-devel] [PATCH 0/3] add usb_detach and usb_attach (v3)
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 > > 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. > This is just adding a HMP command. Is that the right approach or > was that an unintentional consequence of rebasing post-HMP/QMP > split? > yes, my bad, I have not used qmp before, I can add the code but no idea how to test it easily. > Regards, > > Anthony Liguori > > > > >cheers, > > Gerd > > > > > >
Re: [Qemu-devel] [PATCH 0/3] add usb_detach and usb_attach (v3)
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 > > 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? To elaborate a little more, when using a certificates based card there is no hardware event (i.e. removing/inserting the physical card) that causes a usb_detach/attach to the card (both in passthru and emulated), but otoh certificates is good for testing since it decouples it from NSS/tcp. So I needed some way to emulate an insert/remove, and I saw usb_del, which was pretty close, and voila. This is not the same as card remove/reinsert, but it is exactly what will happen to the guest when spicec connects/disconnects, since I detach devices on disconnect and attach on connect. > > This is just adding a HMP command. Is that the right approach or > was that an unintentional consequence of rebasing post-HMP/QMP > split? > > Regards, > > Anthony Liguori > > > > >cheers, > > Gerd > > > > > >
Re: [Qemu-devel] [PATCH 0/3] add usb_detach and usb_attach (v3)
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 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? This is just adding a HMP command. Is that the right approach or was that an unintentional consequence of rebasing post-HMP/QMP split? Regards, Anthony Liguori cheers, Gerd
Re: [Qemu-devel] [PATCH 0/3] add usb_detach and usb_attach (v3)
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 cheers, Gerd
