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

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

[Alon Levy]

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)

 configure       |    9 +++++++++
 hmp-commands.hx |   38 ++++++++++++++++++++++++++++++++++++++
 hw/qdev.c       |    2 +-
 hw/qdev.h       |    1 +
 hw/usb-bus.c    |   16 ++++++++++++++++
 hw/usb.h        |    1 +
 sysemu.h        |    4 ++++
 vl.c            |   33 +++++++++++++++++++++++++++++++++
 8 files changed, 103 insertions(+), 1 deletions(-)

-- 
1.7.3.1

[Qemu-devel] [PATCH 1/3] qdev: make qdev_find_recursive public

[Alon Levy]

---
 hw/qdev.c |    2 +-
 hw/qdev.h |    1 +
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 35858cb..d669a9d 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -477,7 +477,7 @@ static BusState *qbus_find_recursive(BusState *bus, const char *name,
     return NULL;
 }
 
-static DeviceState *qdev_find_recursive(BusState *bus, const char *id)
+DeviceState *qdev_find_recursive(BusState *bus, const char *id)
 {
     DeviceState *dev, *ret;
     BusState *child;
diff --git a/hw/qdev.h b/hw/qdev.h
index 579328a..214066e 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -177,6 +177,7 @@ void qbus_create_inplace(BusState *bus, BusInfo *info,
                          DeviceState *parent, const char *name);
 BusState *qbus_create(BusInfo *info, DeviceState *parent, const char *name);
 void qbus_free(BusState *bus);
+DeviceState *qdev_find_recursive(BusState *bus, const char *id);
 
 #define FROM_QBUS(type, dev) DO_UPCAST(type, qbus, dev)
 
-- 
1.7.3.1

[Qemu-devel] [PATCH 2/3] usb: add public usb_device_by_id

[Alon Levy]

---
 hw/usb-bus.c |   16 ++++++++++++++++
 hw/usb.h     |    1 +
 2 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/hw/usb-bus.c b/hw/usb-bus.c
index b692503..d732bd3 100644
--- a/hw/usb-bus.c
+++ b/hw/usb-bus.c
@@ -189,6 +189,22 @@ int usb_device_detach(USBDevice *dev)
     return 0;
 }
 
+USBDevice *usb_device_by_id(const char* id)
+{
+    USBBus *bus;
+    DeviceState *qdev;
+    USBDevice *dev;
+
+    QTAILQ_FOREACH(bus, &busses, next) {
+        qdev = qdev_find_recursive(&bus->qbus, id);
+        if (qdev != NULL) {
+            dev = DO_UPCAST(USBDevice, qdev, qdev);
+            return dev;
+        }
+    }
+    return NULL;
+}
+
 int usb_device_delete_addr(int busnr, int addr)
 {
     USBBus *bus;
diff --git a/hw/usb.h b/hw/usb.h
index 00d2802..e70fccd 100644
--- a/hw/usb.h
+++ b/hw/usb.h
@@ -317,6 +317,7 @@ void usb_unregister_port(USBBus *bus, USBPort *port);
 int usb_device_attach(USBDevice *dev);
 int usb_device_detach(USBDevice *dev);
 int usb_device_delete_addr(int busnr, int addr);
+USBDevice *usb_device_by_id(const char* id);
 
 static inline USBBus *usb_bus_from_device(USBDevice *d)
 {
-- 
1.7.3.1

[Qemu-devel] [PATCH 3/3] monitor: add usb_attach and usb_detach (v2)

[Alon Levy]

v1->v2 changes:
 * fixed help text (consistent name for parameter)
 * added configure flag, also enabled with --enable-debug
---
 configure       |    9 +++++++++
 hmp-commands.hx |   38 ++++++++++++++++++++++++++++++++++++++
 sysemu.h        |    4 ++++
 vl.c            |   33 +++++++++++++++++++++++++++++++++
 4 files changed, 84 insertions(+), 0 deletions(-)

diff --git a/configure b/configure
index a869777..229a71a 100755
--- a/configure
+++ b/configure
@@ -292,6 +292,7 @@ gprof="no"
 debug_tcg="no"
 debug_mon="no"
 debug="no"
+usb_monitor_attach_detach="no"
 strip_opt="yes"
 bigendian="no"
 mingw32="no"
@@ -590,8 +591,11 @@ for opt do
       debug_tcg="yes"
       debug_mon="yes"
       debug="yes"
+      usb_monitor_attach_detach="yes"
       strip_opt="no"
   ;;
+  --enable-usb-mon-detach) usb_monitor_attach_detach="yes"
+  ;;
   --enable-sparse) sparse="yes"
   ;;
   --disable-sparse) sparse="no"
@@ -854,6 +858,7 @@ echo "  --sysconfdir=PATH        install config in PATH/qemu"
 echo "  --enable-debug-tcg       enable TCG debugging"
 echo "  --disable-debug-tcg      disable TCG debugging (default)"
 echo "  --enable-debug           enable common debug build options"
+echo "  --enable-usb-mon-detach  enable usb monitor attach & detach commands"
 echo "  --enable-sparse          enable sparse checker"
 echo "  --disable-sparse         disable sparse checker (default)"
 echo "  --disable-strip          disable stripping binaries"
@@ -2311,6 +2316,7 @@ echo "host big endian   $bigendian"
 echo "target list       $target_list"
 echo "tcg debug enabled $debug_tcg"
 echo "Mon debug enabled $debug_mon"
+echo "usb mon detach    $usb_monitor_attach_detach"
 echo "gprof enabled     $gprof"
 echo "sparse enabled    $sparse"
 echo "strip binaries    $strip_opt"
@@ -2402,6 +2408,9 @@ fi
 if test "$debug" = "yes" ; then
   echo "CONFIG_DEBUG_EXEC=y" >> $config_host_mak
 fi
+if test "$usb_monitor_attach_detach" = yes ; then
+  echo "CONFIG_USB_MONITOR_ATTACH_DETACH=y" >> $config_host_mak
+fi
 if test "$strip_opt" = "yes" ; then
   echo "STRIP=${strip}" >> $config_host_mak
 fi
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 81999aa..3014b17 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -516,6 +516,44 @@ hub. @var{devname} has the syntax @code{bus.addr}. Use the monitor
 command @code{info usb} to see the devices you can remove.
 ETEXI
 
+#ifdef CONFIG_USB_MONITOR_ATTACH_DETACH
+    {
+        .name       = "usb_attach",
+        .args_type  = "id:s",
+        .params     = "device",
+        .help       = "attach USB device by id",
+        .mhandler.cmd = do_usb_attach,
+    },
+
+STEXI
+@item usb_attach @var{id}
+@findex usb_attach
+
+Attach the USB device with id @var{id} to the QEMU virtual USB
+hub. @var{id} should be a previously detached usb device. Use
+@code{info qtree} to see devices that can be attached. This
+command is for debugging usage only.
+ETEXI
+
+    {
+        .name       = "usb_detach",
+        .args_type  = "id:s",
+        .params     = "device",
+        .help       = "remove USB device by id",
+        .mhandler.cmd = do_usb_detach,
+    },
+
+STEXI
+@item usb_detach @var{id}
+@findex usb_detach
+
+Detach the USB device @var{id} from the QEMU virtual USB
+hub. Use the monitor command @code{info usb} to see the
+devices you can detach. This command is for debugging usage
+only.
+ETEXI
+#endif // CONFIG_USB_MONITOR_ATTACH_DETACH
+
     {
         .name       = "device_add",
         .args_type  = "device:O",
diff --git a/sysemu.h b/sysemu.h
index b81a70e..2e1df04 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -182,6 +182,10 @@ extern struct soundhw soundhw[];
 
 void do_usb_add(Monitor *mon, const QDict *qdict);
 void do_usb_del(Monitor *mon, const QDict *qdict);
+#ifdef CONFIG_USB_MONITOR_ATTACH_DETACH
+void do_usb_attach(Monitor *mon, const QDict *qdict);
+void do_usb_detach(Monitor *mon, const QDict *qdict);
+#endif
 void usb_info(Monitor *mon);
 
 void rtc_change_mon_event(struct tm *tm);
diff --git a/vl.c b/vl.c
index df414ef..f233c84 100644
--- a/vl.c
+++ b/vl.c
@@ -894,6 +894,39 @@ void do_usb_del(Monitor *mon, const QDict *qdict)
     }
 }
 
+#ifdef CONFIG_USB_MONITOR_ATTACH_DETACH
+void do_usb_attach(Monitor *mon, const QDict *qdict)
+{
+    const char *id = qdict_get_str(qdict, "id");
+    USBDevice *dev;
+
+    dev = usb_device_by_id(id);
+
+    if (dev == NULL) {
+        error_report("no such USB device '%s'", id);
+        return;
+    }
+    if (usb_device_attach(dev) < 0) {
+        error_report("could not attach USB device '%s'", id);
+    }
+}
+
+void do_usb_detach(Monitor *mon, const QDict *qdict)
+{
+    const char *id = qdict_get_str(qdict, "id");
+    USBDevice *dev;
+
+    dev = usb_device_by_id(id);
+    if (dev == NULL) {
+        error_report("no such USB device '%s'", id);
+        return;
+    }
+    if (usb_device_detach(dev) < 0) {
+        error_report("could not detach USB device '%s'", id);
+    }
+}
+#endif // CONFIG_USB_MONITOR_ATTACH_DETACH
+
 /***********************************************************/
 /* PCMCIA/Cardbus */
 
-- 
1.7.3.1

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

[Gerd Hoffmann]

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 <kraxel@redhat.com>

cheers,
   Gerd

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

[Anthony Liguori]

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 <kraxel@redhat.com>

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)

[Alon Levy]

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 <kraxel@redhat.com>
> 
> 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)

[Ryan Harper]

* Alon Levy <alevy@redhat.com> [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 <kraxel@redhat.com>
> > 
> > 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
ryanh@us.ibm.com

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

[Markus Armbruster]

Sorry for coming so late to this thread...

Alon Levy <alevy@redhat.com> 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 <kraxel@redhat.com>
>> 
>> 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)

[Alon Levy]

On Wed, Nov 10, 2010 at 04:49:38PM +0100, Markus Armbruster wrote:
> Sorry for coming so late to this thread...
> 
> Alon Levy <alevy@redhat.com> 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 <kraxel@redhat.com>
> >> 
> >> 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)

[Markus Armbruster]

Alon Levy <alevy@redhat.com> writes:

> On Wed, Nov 10, 2010 at 04:49:38PM +0100, Markus Armbruster wrote:
>> Sorry for coming so late to this thread...
>> 
>> Alon Levy <alevy@redhat.com> 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 <kraxel@redhat.com>
>> >> 
>> >> 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)

[Alon Levy]

On Thu, Nov 11, 2010 at 11:29:36AM +0100, Markus Armbruster wrote:
> Alon Levy <alevy@redhat.com> writes:
> 
> > On Wed, Nov 10, 2010 at 04:49:38PM +0100, Markus Armbruster wrote:
> >> Sorry for coming so late to this thread...
> >> 
> >> Alon Levy <alevy@redhat.com> 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 <kraxel@redhat.com>
> >> >> 
> >> >> 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->exit, info->init.
Not a small amount of work :/ Actually, I think you'd need to do that anyway
to get any benefit from the detach/attach commands (apart from not deleting
associated chardevs).

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

[Markus Armbruster]

Alon Levy <alevy@redhat.com> writes:

> On Thu, Nov 11, 2010 at 11:29:36AM +0100, Markus Armbruster wrote:
>> Alon Levy <alevy@redhat.com> writes:
>> 
>> > On Wed, Nov 10, 2010 at 04:49:38PM +0100, Markus Armbruster wrote:
>> >> Sorry for coming so late to this thread...
>> >> 
>> >> Alon Levy <alevy@redhat.com> 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 <kraxel@redhat.com>
>> >> >> 
>> >> >> 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 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->exit, info->init.
> Not a small amount of work :/ Actually, I think you'd need to do that anyway
> to get any benefit from the detach/attach commands (apart from not deleting
> associated chardevs).

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?


[*] 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]

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]

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 <kraxel@redhat.com>
> 
> 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)

[Luiz Capitulino]

On Thu, 21 Oct 2010 15:27:23 +0200
Alon Levy <alevy@redhat.com> 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 <kraxel@redhat.com>
> > 
> > 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)

[Anthony Liguori]

On 10/22/2010 07:48 AM, Luiz Capitulino wrote:
> On Thu, 21 Oct 2010 15:27:23 +0200
> Alon Levy<alevy@redhat.com>  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<kraxel@redhat.com>
>>>>          
>>> 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)

[Luiz Capitulino]

On Fri, 22 Oct 2010 07:55:02 -0500
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 10/22/2010 07:48 AM, Luiz Capitulino wrote:
> > On Thu, 21 Oct 2010 15:27:23 +0200
> > Alon Levy<alevy@redhat.com>  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<kraxel@redhat.com>
> >>>>          
> >>> 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)

[Anthony Liguori]

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)

[Luiz Capitulino]

On Fri, 22 Oct 2010 08:43:39 -0500
Anthony Liguori <anthony@codemonkey.ws> 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)

[Alon Levy]

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<alevy@redhat.com>  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<kraxel@redhat.com>
> >>>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
> >>>>
> >>>>
> >>>
> 

[Qemu-devel] [PATCH 0/3] add usb_detach and usb_attach (v2)

[Alon Levy]

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

 hmp-commands.hx |   34 ++++++++++++++++++++++++++++++++++
 hw/qdev.c       |    2 +-
 hw/qdev.h       |    1 +
 hw/usb-bus.c    |   16 ++++++++++++++++
 hw/usb.h        |    1 +
 sysemu.h        |    2 ++
 vl.c            |   31 +++++++++++++++++++++++++++++++
 7 files changed, 86 insertions(+), 1 deletions(-)

-- 
1.7.3.1

[Qemu-devel] [PATCH 2/3] usb: add public usb_device_by_id

[Alon Levy]

---
 hw/usb-bus.c |   16 ++++++++++++++++
 hw/usb.h     |    1 +
 2 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/hw/usb-bus.c b/hw/usb-bus.c
index b692503..d732bd3 100644
--- a/hw/usb-bus.c
+++ b/hw/usb-bus.c
@@ -189,6 +189,22 @@ int usb_device_detach(USBDevice *dev)
     return 0;
 }
 
+USBDevice *usb_device_by_id(const char* id)
+{
+    USBBus *bus;
+    DeviceState *qdev;
+    USBDevice *dev;
+
+    QTAILQ_FOREACH(bus, &busses, next) {
+        qdev = qdev_find_recursive(&bus->qbus, id);
+        if (qdev != NULL) {
+            dev = DO_UPCAST(USBDevice, qdev, qdev);
+            return dev;
+        }
+    }
+    return NULL;
+}
+
 int usb_device_delete_addr(int busnr, int addr)
 {
     USBBus *bus;
diff --git a/hw/usb.h b/hw/usb.h
index 00d2802..e70fccd 100644
--- a/hw/usb.h
+++ b/hw/usb.h
@@ -317,6 +317,7 @@ void usb_unregister_port(USBBus *bus, USBPort *port);
 int usb_device_attach(USBDevice *dev);
 int usb_device_detach(USBDevice *dev);
 int usb_device_delete_addr(int busnr, int addr);
+USBDevice *usb_device_by_id(const char* id);
 
 static inline USBBus *usb_bus_from_device(USBDevice *d)
 {
-- 
1.7.3.1

Re: [Qemu-devel] [PATCH 2/3] usb: add public usb_device_by_id

[Gerd Hoffmann]

> +USBDevice *usb_device_by_id(const char* id)
> +{
> +    USBBus *bus;
> +    DeviceState *qdev;
> +    USBDevice *dev;
> +
> +    QTAILQ_FOREACH(bus,&busses, next) {
> +        qdev = qdev_find_recursive(&bus->qbus, id);
> +        if (qdev != NULL) {
> +            dev = DO_UPCAST(USBDevice, qdev, qdev);
> +            return dev;
> +        }
> +    }

You don't need qdev_find_recursive here.  Have a look at the usb_info() 
code to see how to loop over all usb devices.  Then compare id with 
USBDevice->qdev.id.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 2/3] usb: add public usb_device_by_id

[Alon Levy]

----- "Gerd Hoffmann" <kraxel@redhat.com> wrote:

> > +USBDevice *usb_device_by_id(const char* id)
> > +{
> > +    USBBus *bus;
> > +    DeviceState *qdev;
> > +    USBDevice *dev;
> > +
> > +    QTAILQ_FOREACH(bus,&busses, next) {
> > +        qdev = qdev_find_recursive(&bus->qbus, id);
> > +        if (qdev != NULL) {
> > +            dev = DO_UPCAST(USBDevice, qdev, qdev);
> > +            return dev;
> > +        }
> > +    }
> 
> You don't need qdev_find_recursive here.  Have a look at the
> usb_info() 
> code to see how to loop over all usb devices.  Then compare id with 
> USBDevice->qdev.id.
> 
> cheers,
>    Gerd

There is no problem to loop over all usb devices. But first of all I don't want to loop on used, since then I miss any detached devices, so I actually do want the same behavior of qdev_find_recursive, and since it's already available, why rewrite it in a different compilation unit?

Alon

Re: [Qemu-devel] [PATCH 2/3] usb: add public usb_device_by_id

[Gerd Hoffmann]

   Hi,

> There is no problem to loop over all usb devices. But first of all I
> don't want to loop on used, since then I miss any detached devices,
> so I actually do want the same behavior of qdev_find_recursive, and
> since it's already available, why rewrite it in a different
> compilation unit?

Point.  ACK then.

cheers,
   Gerd