[PATCH 0/3] libxl: add support for qemu base pvusb backend

[PATCH 0/3] libxl: add support for qemu base pvusb backend

[Juergen Gross]

This patch series is meant to be applied on top of Chunyan's series
to support pvusb in libxl.

It is adding support for an alternative pvusb backend "qusb" via qemu.


Juergen Gross (3):
  libxl: make libxl__need_xenpv_qemu() operate on domain config
  libxl: add domain config parameter to force start of qemu
  libxl: add new pvusb backend "qusb" provided by qemu

 docs/man/xl.cfg.pod.5                |  17 +++++-
 tools/libxl/libxl_create.c           |  10 +---
 tools/libxl/libxl_device.c           |   3 +-
 tools/libxl/libxl_dm.c               |  66 +++++++++--------------
 tools/libxl/libxl_internal.h         |   6 +--
 tools/libxl/libxl_pvusb.c            | 102 +++++++++++++++++++++++++++--------
 tools/libxl/libxl_types.idl          |   2 +
 tools/libxl/libxl_types_internal.idl |   1 +
 tools/libxl/xl_cmdimpl.c             |   3 ++
 9 files changed, 132 insertions(+), 78 deletions(-)

-- 
2.6.2


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 1/3] libxl: make libxl__need_xenpv_qemu() operate on domain config

[Juergen Gross]

libxl__need_xenpv_qemu() is called with configuration data for console,
vfbs, disks and channels today in order to evaluate the need for
starting a device model for a pv domain.

The console data is local to the caller and setup in a way to never
require a device model. All other data is taken from the domain config
structure.

In order to support other device backends via qemu change the interface
of libxl__need_xenpv_qemu() to take the domain config structure as
input instead of the single device arrays.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/libxl/libxl_create.c   |  9 +------
 tools/libxl/libxl_dm.c       | 59 ++++++++++++--------------------------------
 tools/libxl/libxl_internal.h |  5 +---
 3 files changed, 18 insertions(+), 55 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 61b5c01..0e2b0a0 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1304,7 +1304,6 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
     }
     case LIBXL_DOMAIN_TYPE_PV:
     {
-        int need_qemu = 0;
         libxl__device_console console;
         libxl__device device;
 
@@ -1314,17 +1313,11 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
         }
 
         init_console_info(gc, &console, 0);
-
-        need_qemu = libxl__need_xenpv_qemu(gc, 1, &console,
-                d_config->num_vfbs, d_config->vfbs,
-                d_config->num_disks, &d_config->disks[0],
-                d_config->num_channels, &d_config->channels[0]);
-
         console.backend_domid = state->console_domid;
         libxl__device_console_add(gc, domid, &console, state, &device);
         libxl__device_console_dispose(&console);
 
-        if (need_qemu) {
+        if (libxl__need_xenpv_qemu(gc, d_config)) {
             dcs->dmss.dm.guest_domid = domid;
             libxl__spawn_local_dm(egc, &dcs->dmss.dm);
             return;
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 4aca38e..4b840f4 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -2107,61 +2107,34 @@ int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid)
                 GCSPRINTF("/local/domain/%d/image/device-model-pid", domid));
 }
 
-int libxl__need_xenpv_qemu(libxl__gc *gc,
-        int nr_consoles, libxl__device_console *consoles,
-        int nr_vfbs, libxl_device_vfb *vfbs,
-        int nr_disks, libxl_device_disk *disks,
-        int nr_channels, libxl_device_channel *channels)
+int libxl__need_xenpv_qemu(libxl__gc *gc, libxl_domain_config *d_config)
 {
-    int i, ret = 0;
+    int i, ret;
     uint32_t domid;
 
-    /*
-     * qemu is required in order to support 2 or more consoles. So switch all
-     * backends to qemu if this is the case
-     */
-    if (nr_consoles > 1) {
-        for (i = 0; i < nr_consoles; i++)
-            consoles[i].consback = LIBXL__CONSOLE_BACKEND_IOEMU;
+    ret = libxl__get_domid(gc, &domid);
+    if (ret) goto out;
+
+    if (d_config->num_vfbs > 0) {
         ret = 1;
         goto out;
     }
 
-    for (i = 0; i < nr_consoles; i++) {
-        if (consoles[i].consback == LIBXL__CONSOLE_BACKEND_IOEMU) {
+    for (i = 0; i < d_config->num_disks; i++) {
+        if (d_config->disks[i].backend == LIBXL_DISK_BACKEND_QDISK &&
+            d_config->disks[i].backend_domid == domid) {
             ret = 1;
             goto out;
         }
     }
 
-    if (nr_vfbs > 0) {
-        ret = 1;
-        goto out;
-    }
-
-    if (nr_disks > 0) {
-        ret = libxl__get_domid(gc, &domid);
-        if (ret) goto out;
-        for (i = 0; i < nr_disks; i++) {
-            if (disks[i].backend == LIBXL_DISK_BACKEND_QDISK &&
-                disks[i].backend_domid == domid) {
-                ret = 1;
-                goto out;
-            }
-        }
-    }
-
-    if (nr_channels > 0) {
-        ret = libxl__get_domid(gc, &domid);
-        if (ret) goto out;
-        for (i = 0; i < nr_channels; i++) {
-            if (channels[i].backend_domid == domid) {
-                /* xenconsoled is limited to the first console only.
-                   Until this restriction is removed we must use qemu for
-                   secondary consoles which includes all channels. */
-                ret = 1;
-                goto out;
-            }
+    for (i = 0; i < d_config->num_channels; i++) {
+        if (d_config->channels[i].backend_domid == domid) {
+            /* xenconsoled is limited to the first console only.
+               Until this restriction is removed we must use qemu for
+               secondary consoles which includes all channels. */
+            ret = 1;
+            goto out;
         }
     }
 
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 345a764..fc7bdab 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1616,10 +1616,7 @@ _hidden int libxl__domain_build(libxl__gc *gc,
 _hidden const char *libxl__domain_device_model(libxl__gc *gc,
                                         const libxl_domain_build_info *info);
 _hidden int libxl__need_xenpv_qemu(libxl__gc *gc,
-        int nr_consoles, libxl__device_console *consoles,
-        int nr_vfbs, libxl_device_vfb *vfbs,
-        int nr_disks, libxl_device_disk *disks,
-        int nr_channels, libxl_device_channel *channels);
+                                   libxl_domain_config *d_config);
 
 /*
  * This function will fix reserved device memory conflict
-- 
2.6.2


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 1/3] libxl: make libxl__need_xenpv_qemu() operate on domain config

[George Dunlap]

On Thu, Mar 10, 2016 at 3:00 PM, Juergen Gross <jgross@suse.com> wrote:
> libxl__need_xenpv_qemu() is called with configuration data for console,
> vfbs, disks and channels today in order to evaluate the need for
> starting a device model for a pv domain.
>
> The console data is local to the caller and setup in a way to never
> require a device model. All other data is taken from the domain config
> structure.
>
> In order to support other device backends via qemu change the interface
> of libxl__need_xenpv_qemu() to take the domain config structure as
> input instead of the single device arrays.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  tools/libxl/libxl_create.c   |  9 +------
>  tools/libxl/libxl_dm.c       | 59 ++++++++++++--------------------------------
>  tools/libxl/libxl_internal.h |  5 +---
>  3 files changed, 18 insertions(+), 55 deletions(-)
>
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 61b5c01..0e2b0a0 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -1304,7 +1304,6 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
>      }
>      case LIBXL_DOMAIN_TYPE_PV:
>      {
> -        int need_qemu = 0;
>          libxl__device_console console;
>          libxl__device device;
>
> @@ -1314,17 +1313,11 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
>          }
>
>          init_console_info(gc, &console, 0);
> -
> -        need_qemu = libxl__need_xenpv_qemu(gc, 1, &console,
> -                d_config->num_vfbs, d_config->vfbs,
> -                d_config->num_disks, &d_config->disks[0],
> -                d_config->num_channels, &d_config->channels[0]);
> -
>          console.backend_domid = state->console_domid;
>          libxl__device_console_add(gc, domid, &console, state, &device);
>          libxl__device_console_dispose(&console);
>
> -        if (need_qemu) {
> +        if (libxl__need_xenpv_qemu(gc, d_config)) {
>              dcs->dmss.dm.guest_domid = domid;
>              libxl__spawn_local_dm(egc, &dcs->dmss.dm);
>              return;
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 4aca38e..4b840f4 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -2107,61 +2107,34 @@ int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid)
>                  GCSPRINTF("/local/domain/%d/image/device-model-pid", domid));
>  }
>
> -int libxl__need_xenpv_qemu(libxl__gc *gc,
> -        int nr_consoles, libxl__device_console *consoles,
> -        int nr_vfbs, libxl_device_vfb *vfbs,
> -        int nr_disks, libxl_device_disk *disks,
> -        int nr_channels, libxl_device_channel *channels)
> +int libxl__need_xenpv_qemu(libxl__gc *gc, libxl_domain_config *d_config)
>  {
> -    int i, ret = 0;
> +    int i, ret;
>      uint32_t domid;
>
> -    /*
> -     * qemu is required in order to support 2 or more consoles. So switch all
> -     * backends to qemu if this is the case
> -     */
> -    if (nr_consoles > 1) {
> -        for (i = 0; i < nr_consoles; i++)
> -            consoles[i].consback = LIBXL__CONSOLE_BACKEND_IOEMU;
> +    ret = libxl__get_domid(gc, &domid);
> +    if (ret) goto out;

Doesn't this mean that if libxl__get_domid() fails,
libxl__need_xenpv_qemu() will effectively return "true"?

I realize this is a bug in the existing code but there's no need to be
bug-for-bug compatible here. :-)

Maybe add an 'rc' variable, and make this:

rc = libxl__get_domid();
if(rc) goto out;

Other than that, looks good to me.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 1/3] libxl: make libxl__need_xenpv_qemu() operate on domain config

[Juergen Gross]

On 17/03/16 17:53, George Dunlap wrote:
> On Thu, Mar 10, 2016 at 3:00 PM, Juergen Gross <jgross@suse.com> wrote:
>> libxl__need_xenpv_qemu() is called with configuration data for console,
>> vfbs, disks and channels today in order to evaluate the need for
>> starting a device model for a pv domain.
>>
>> The console data is local to the caller and setup in a way to never
>> require a device model. All other data is taken from the domain config
>> structure.
>>
>> In order to support other device backends via qemu change the interface
>> of libxl__need_xenpv_qemu() to take the domain config structure as
>> input instead of the single device arrays.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>  tools/libxl/libxl_create.c   |  9 +------
>>  tools/libxl/libxl_dm.c       | 59 ++++++++++++--------------------------------
>>  tools/libxl/libxl_internal.h |  5 +---
>>  3 files changed, 18 insertions(+), 55 deletions(-)
>>
>> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
>> index 61b5c01..0e2b0a0 100644
>> --- a/tools/libxl/libxl_create.c
>> +++ b/tools/libxl/libxl_create.c
>> @@ -1304,7 +1304,6 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
>>      }
>>      case LIBXL_DOMAIN_TYPE_PV:
>>      {
>> -        int need_qemu = 0;
>>          libxl__device_console console;
>>          libxl__device device;
>>
>> @@ -1314,17 +1313,11 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
>>          }
>>
>>          init_console_info(gc, &console, 0);
>> -
>> -        need_qemu = libxl__need_xenpv_qemu(gc, 1, &console,
>> -                d_config->num_vfbs, d_config->vfbs,
>> -                d_config->num_disks, &d_config->disks[0],
>> -                d_config->num_channels, &d_config->channels[0]);
>> -
>>          console.backend_domid = state->console_domid;
>>          libxl__device_console_add(gc, domid, &console, state, &device);
>>          libxl__device_console_dispose(&console);
>>
>> -        if (need_qemu) {
>> +        if (libxl__need_xenpv_qemu(gc, d_config)) {
>>              dcs->dmss.dm.guest_domid = domid;
>>              libxl__spawn_local_dm(egc, &dcs->dmss.dm);
>>              return;
>> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
>> index 4aca38e..4b840f4 100644
>> --- a/tools/libxl/libxl_dm.c
>> +++ b/tools/libxl/libxl_dm.c
>> @@ -2107,61 +2107,34 @@ int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid)
>>                  GCSPRINTF("/local/domain/%d/image/device-model-pid", domid));
>>  }
>>
>> -int libxl__need_xenpv_qemu(libxl__gc *gc,
>> -        int nr_consoles, libxl__device_console *consoles,
>> -        int nr_vfbs, libxl_device_vfb *vfbs,
>> -        int nr_disks, libxl_device_disk *disks,
>> -        int nr_channels, libxl_device_channel *channels)
>> +int libxl__need_xenpv_qemu(libxl__gc *gc, libxl_domain_config *d_config)
>>  {
>> -    int i, ret = 0;
>> +    int i, ret;
>>      uint32_t domid;
>>
>> -    /*
>> -     * qemu is required in order to support 2 or more consoles. So switch all
>> -     * backends to qemu if this is the case
>> -     */
>> -    if (nr_consoles > 1) {
>> -        for (i = 0; i < nr_consoles; i++)
>> -            consoles[i].consback = LIBXL__CONSOLE_BACKEND_IOEMU;
>> +    ret = libxl__get_domid(gc, &domid);
>> +    if (ret) goto out;
> 
> Doesn't this mean that if libxl__get_domid() fails,
> libxl__need_xenpv_qemu() will effectively return "true"?
> 
> I realize this is a bug in the existing code but there's no need to be
> bug-for-bug compatible here. :-)
> 
> Maybe add an 'rc' variable, and make this:
> 
> rc = libxl__get_domid();
> if(rc) goto out;

Okay, will do (next week).

> 
> Other than that, looks good to me.

Thanks,

Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 2/3] libxl: add domain config parameter to force start of qemu

[Juergen Gross]

Today the device model (qemu) is started for a pv domain only in case
a device requiring qemu is specified in the domain configuration
(qdisk, vfb, channel). If there is no such device the device model
isn't started and hence it is possible to add such a device to the
domain later.

Add a domain configuration parameter to specify the device model is
to be started in any case. This will enable adding devices with a
qemu based backend later.

While the optimal solution would be to start the device model
automatically when needed this would require some major rework of
libxl at multiple places.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 docs/man/xl.cfg.pod.5       | 6 ++++++
 tools/libxl/libxl_create.c  | 1 +
 tools/libxl/libxl_dm.c      | 5 +++++
 tools/libxl/libxl_types.idl | 1 +
 tools/libxl/xl_cmdimpl.c    | 3 +++
 5 files changed, 16 insertions(+)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index b156caa..1dde66b 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -1939,6 +1939,12 @@ xen-qemudepriv-domid$domid or xen-qemudepriv-shared or root.
 Please note that running QEMU as non-root causes migration and PCI
 passthrough not to work properly.
 
+=item B<device_model_always=BOOLEAN>
+
+If true, start the device model for paravirtualized domains even if this isn't
+required according to the configured devices. This allows to add such devices
+later when the domain is already running.
+
 =back
 
 =head2 Keymaps
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 0e2b0a0..52a0a2f 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -73,6 +73,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
         return ERROR_INVAL;
     }
 
+    libxl_defbool_setdefault(&b_info->device_model_always, false);
     libxl_defbool_setdefault(&b_info->device_model_stubdomain, false);
 
     if (libxl_defbool_val(b_info->device_model_stubdomain) &&
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 4b840f4..637b11d 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -2115,6 +2115,11 @@ int libxl__need_xenpv_qemu(libxl__gc *gc, libxl_domain_config *d_config)
     ret = libxl__get_domid(gc, &domid);
     if (ret) goto out;
 
+    if (libxl_defbool_val(d_config->b_info.device_model_always)) {
+        ret = 1;
+        goto out;
+    }
+
     if (d_config->num_vfbs > 0) {
         ret = 1;
         goto out;
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 2a99eeb..b0a9776 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -450,6 +450,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
     ("device_model_ssidref", uint32),
     ("device_model_ssid_label", string),
     ("device_model_user", string),
+    ("device_model_always", libxl_defbool),
 
     # extra parameters pass directly to qemu, NULL terminated
     ("extra",            libxl_string_list),
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index a3610fc..0fdca73 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -2269,6 +2269,9 @@ skip_usbdev:
     }
 
     /* parse device model arguments, this works for pv, hvm and stubdom */
+    xlu_cfg_get_defbool (config, "device_model_always",
+                         &b_info->device_model_always, 0);
+
     if (!xlu_cfg_get_string (config, "device_model", &buf, 0)) {
         fprintf(stderr,
                 "WARNING: ignoring device_model directive.\n"
-- 
2.6.2


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 2/3] libxl: add domain config parameter to force start of qemu

[George Dunlap]

On Thu, Mar 10, 2016 at 3:00 PM, Juergen Gross <jgross@suse.com> wrote:
> Today the device model (qemu) is started for a pv domain only in case
> a device requiring qemu is specified in the domain configuration
> (qdisk, vfb, channel). If there is no such device the device model
> isn't started and hence it is possible to add such a device to the
> domain later.
>
> Add a domain configuration parameter to specify the device model is
> to be started in any case. This will enable adding devices with a
> qemu based backend later.
>
> While the optimal solution would be to start the device model
> automatically when needed this would require some major rework of
> libxl at multiple places.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>

So wait -- what happens now if you try to attach a disk with a qdisk
backend to a PV guest that didn't start with qemu running?

I'd really like to see patch 3 get in, but I'm not really in favor of
this sort of a user-visible hack, particularly as we have to support
it in libxl more or less indefinitely.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 2/3] libxl: add domain config parameter to force start of qemu

[Juergen Gross]

On 17/03/16 17:06, George Dunlap wrote:
> On Thu, Mar 10, 2016 at 3:00 PM, Juergen Gross <jgross@suse.com> wrote:
>> Today the device model (qemu) is started for a pv domain only in case
>> a device requiring qemu is specified in the domain configuration
>> (qdisk, vfb, channel). If there is no such device the device model
>> isn't started and hence it is possible to add such a device to the
>> domain later.
>>
>> Add a domain configuration parameter to specify the device model is
>> to be started in any case. This will enable adding devices with a
>> qemu based backend later.
>>
>> While the optimal solution would be to start the device model
>> automatically when needed this would require some major rework of
>> libxl at multiple places.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> So wait -- what happens now if you try to attach a disk with a qdisk
> backend to a PV guest that didn't start with qemu running?

It won't work (that was my test case for the patch).

> I'd really like to see patch 3 get in, but I'm not really in favor of
> this sort of a user-visible hack, particularly as we have to support
> it in libxl more or less indefinitely.

Hmm, really? We can add a smarter variant later which will start the
device model in case it isn't started yet. Then the new config parameter
could be just ignored.

I didn't do the smart variant as I'm not sure I could set it up in time
for 4.7. I'd be happy to do it with some assistance regarding the async
framework of libxl I'm not at all familiar with.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 2/3] libxl: add domain config parameter to force start of qemu

[George Dunlap]

On 18/03/16 08:11, Juergen Gross wrote:
> On 17/03/16 17:06, George Dunlap wrote:
>> On Thu, Mar 10, 2016 at 3:00 PM, Juergen Gross <jgross@suse.com> wrote:
>>> Today the device model (qemu) is started for a pv domain only in case
>>> a device requiring qemu is specified in the domain configuration
>>> (qdisk, vfb, channel). If there is no such device the device model
>>> isn't started and hence it is possible to add such a device to the
>>> domain later.
>>>
>>> Add a domain configuration parameter to specify the device model is
>>> to be started in any case. This will enable adding devices with a
>>> qemu based backend later.
>>>
>>> While the optimal solution would be to start the device model
>>> automatically when needed this would require some major rework of
>>> libxl at multiple places.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>
>> So wait -- what happens now if you try to attach a disk with a qdisk
>> backend to a PV guest that didn't start with qemu running?
> 
> It won't work (that was my test case for the patch).
> 
>> I'd really like to see patch 3 get in, but I'm not really in favor of
>> this sort of a user-visible hack, particularly as we have to support
>> it in libxl more or less indefinitely.
> 
> Hmm, really? We can add a smarter variant later which will start the
> device model in case it isn't started yet. Then the new config parameter
> could be just ignored.

Sure; but it will (probably) only actually be useful for one release
cycle (now 6 months), and then it will sit around cluttering up the
interface for years to come.  The situation wrt hotplug has been this
way for years now, and nobody has complained; I don't think an extra 6
months will be a big deal.

Ultimately it's the tools maintainers' call; I wouldn't argue against it
if one of them think it's a good idea.

> I didn't do the smart variant as I'm not sure I could set it up in time
> for 4.7. I'd be happy to do it with some assistance regarding the async
> framework of libxl I'm not at all familiar with.

I'm sure help could be arranged; but it might be difficult to do by 4.7.

 -George


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 2/3] libxl: add domain config parameter to force start of qemu

[Juergen Gross]

On 21/03/16 15:28, George Dunlap wrote:
> On 18/03/16 08:11, Juergen Gross wrote:
>> On 17/03/16 17:06, George Dunlap wrote:
>>> On Thu, Mar 10, 2016 at 3:00 PM, Juergen Gross <jgross@suse.com> wrote:
>>>> Today the device model (qemu) is started for a pv domain only in case
>>>> a device requiring qemu is specified in the domain configuration
>>>> (qdisk, vfb, channel). If there is no such device the device model
>>>> isn't started and hence it is possible to add such a device to the
>>>> domain later.
>>>>
>>>> Add a domain configuration parameter to specify the device model is
>>>> to be started in any case. This will enable adding devices with a
>>>> qemu based backend later.
>>>>
>>>> While the optimal solution would be to start the device model
>>>> automatically when needed this would require some major rework of
>>>> libxl at multiple places.
>>>>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>
>>> So wait -- what happens now if you try to attach a disk with a qdisk
>>> backend to a PV guest that didn't start with qemu running?
>>
>> It won't work (that was my test case for the patch).
>>
>>> I'd really like to see patch 3 get in, but I'm not really in favor of
>>> this sort of a user-visible hack, particularly as we have to support
>>> it in libxl more or less indefinitely.
>>
>> Hmm, really? We can add a smarter variant later which will start the
>> device model in case it isn't started yet. Then the new config parameter
>> could be just ignored.
> 
> Sure; but it will (probably) only actually be useful for one release
> cycle (now 6 months), and then it will sit around cluttering up the
> interface for years to come.  The situation wrt hotplug has been this
> way for years now, and nobody has complained; I don't think an extra 6
> months will be a big deal.
> 
> Ultimately it's the tools maintainers' call; I wouldn't argue against it
> if one of them think it's a good idea.
> 
>> I didn't do the smart variant as I'm not sure I could set it up in time
>> for 4.7. I'd be happy to do it with some assistance regarding the async
>> framework of libxl I'm not at all familiar with.
> 
> I'm sure help could be arranged; but it might be difficult to do by 4.7.

Okay, let's see what can be arranged.

BTW: This patch is not strictly required for patch 3. Patch 3 is just
adding another case where no device model is available when needed.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 3/3] libxl: add new pvusb backend "qusb" provided by qemu

[Juergen Gross]

Add a new pvusb backend type "qusb" which is provided by qemu. It can
be selected either by specifying the type directly in the configuration
or it is selected automatically by libxl in case there is no "usbback"
driver loaded.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 docs/man/xl.cfg.pod.5                |  11 +++-
 tools/libxl/libxl_device.c           |   3 +-
 tools/libxl/libxl_dm.c               |   8 +++
 tools/libxl/libxl_internal.h         |   1 +
 tools/libxl/libxl_pvusb.c            | 102 +++++++++++++++++++++++++++--------
 tools/libxl/libxl_types.idl          |   1 +
 tools/libxl/libxl_types_internal.idl |   1 +
 7 files changed, 101 insertions(+), 26 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index 1dde66b..06eeb42 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -737,8 +737,15 @@ Possible B<KEY>s are:
 
 =item B<type=TYPE>
 
-Specifies the usb controller type.  Currently only 'pv' and 'auto'
-are supported.
+Specifies the usb controller type.
+
+"pv" denotes a kernel based pvusb backend.
+
+"qusb" specifies a qemu base backend for pvusb.
+
+"auto" (the default) determines whether a kernel based backend is installed.
+If this is the case, "pv" is selected, "qusb" will be selected if no kernel
+backend is currently available.
 
 =item B<version=VERSION>
 
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 4ced9b6..eba3087 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -680,7 +680,8 @@ void libxl__devices_destroy(libxl__egc *egc, libxl__devices_remove_state *drs)
                 aodev->action = LIBXL__DEVICE_ACTION_REMOVE;
                 aodev->dev = dev;
                 aodev->force = drs->force;
-                if (dev->backend_kind == LIBXL__DEVICE_KIND_VUSB)
+                if (dev->backend_kind == LIBXL__DEVICE_KIND_VUSB ||
+                    dev->backend_kind == LIBXL__DEVICE_KIND_QUSB)
                     libxl__initiate_device_usbctrl_remove(egc, aodev);
                 else
                     libxl__initiate_device_generic_remove(egc, aodev);
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 637b11d..3793d2c 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -2143,6 +2143,14 @@ int libxl__need_xenpv_qemu(libxl__gc *gc, libxl_domain_config *d_config)
         }
     }
 
+    for (i = 0; i < d_config->num_usbctrls; i++) {
+       if (d_config->usbctrls[i].type == LIBXL_USBCTRL_TYPE_QUSB &&
+           d_config->usbctrls[i].backend_domid == domid) {
+            ret = 1;
+            goto out;
+        }
+    }
+
 out:
     return ret;
 }
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index fc7bdab..2db8b1b 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -487,6 +487,7 @@ typedef struct {
 #define QEMU_BACKEND(dev) (\
     (dev)->backend_kind == LIBXL__DEVICE_KIND_QDISK || \
     (dev)->backend_kind == LIBXL__DEVICE_KIND_VFB || \
+    (dev)->backend_kind == LIBXL__DEVICE_KIND_QUSB || \
     (dev)->backend_kind == LIBXL__DEVICE_KIND_VKBD)
 
 #define XC_PCI_BDF             "0x%x, 0x%x, 0x%x, 0x%x"
diff --git a/tools/libxl/libxl_pvusb.c b/tools/libxl/libxl_pvusb.c
index 256add9..d9bac3c 100644
--- a/tools/libxl/libxl_pvusb.c
+++ b/tools/libxl/libxl_pvusb.c
@@ -22,6 +22,21 @@
 
 #define USBHUB_CLASS_CODE 9
 
+static int usbback_is_loaded(libxl__gc *gc)
+{
+    int r;
+    struct stat st;
+
+    r = lstat(SYSFS_USBBACK_DRIVER, &st);
+
+    if (r == 0)
+        return 1;
+    if (r < 0 && errno == ENOENT)
+        return 0;
+    LOGE(ERROR, "Accessing %s", SYSFS_USBBACK_DRIVER);
+    return -1;
+}
+
 static int libxl__device_usbctrl_setdefault(libxl__gc *gc, uint32_t domid,
                                             libxl_device_usbctrl *usbctrl)
 {
@@ -36,7 +51,8 @@ static int libxl__device_usbctrl_setdefault(libxl__gc *gc, uint32_t domid,
 
     if (usbctrl->type == LIBXL_USBCTRL_TYPE_AUTO) {
         if (domtype == LIBXL_DOMAIN_TYPE_PV) {
-            usbctrl->type = LIBXL_USBCTRL_TYPE_PV;
+            usbctrl->type = usbback_is_loaded(gc) ? LIBXL_USBCTRL_TYPE_PV
+                                                  : LIBXL_USBCTRL_TYPE_QUSB;
         } else if (domtype == LIBXL_DOMAIN_TYPE_HVM) {
             /* FIXME: See if we can detect PV frontend */
             usbctrl->type = LIBXL_USBCTRL_TYPE_DEVICEMODEL;
@@ -54,7 +70,9 @@ int libxl__device_from_usbctrl(libxl__gc *gc, uint32_t domid,
 {
     device->backend_devid   = usbctrl->devid;
     device->backend_domid   = usbctrl->backend_domid;
-    device->backend_kind    = LIBXL__DEVICE_KIND_VUSB;
+    device->backend_kind    = (usbctrl->type == LIBXL_USBCTRL_TYPE_PV)
+                              ? LIBXL__DEVICE_KIND_VUSB
+                              : LIBXL__DEVICE_KIND_QUSB;
     device->devid           = usbctrl->devid;
     device->domid           = domid;
     device->kind            = LIBXL__DEVICE_KIND_VUSB;
@@ -64,9 +82,9 @@ int libxl__device_from_usbctrl(libxl__gc *gc, uint32_t domid,
 
 /* Add usbctrl information to xenstore.
  *
- * Adding a usb controller will add a new 'vusb' device in xenstore, and
- * add corresponding frontend, backend information to it. According to
- * "update_json", decide wether to update json config file.
+ * Adding a usb controller will add a new 'qusb' or 'vusb' device in xenstore,
+ * and add corresponding frontend, backend information to it. According to
+ * "update_json", decide whether to update json config file.
  */
 static int libxl__device_usbctrl_add_xenstore(libxl__gc *gc, uint32_t domid,
                                               libxl_device_usbctrl *usbctrl,
@@ -159,6 +177,18 @@ out:
     return rc;
 }
 
+static char *pvusb_get_device_type(libxl_usbctrl_type type)
+{
+    switch (type) {
+    case LIBXL_USBCTRL_TYPE_PV:
+        return "vusb";
+    case LIBXL_USBCTRL_TYPE_QUSB:
+        return "qusb";
+    default:
+        return NULL;
+    }
+}
+
 /* AO operation to add a usb controller.
  *
  * Generally, it does:
@@ -190,7 +220,8 @@ void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t domid,
         }
     }
 
-    if (usbctrl->type != LIBXL_USBCTRL_TYPE_PV) {
+    if (usbctrl->type != LIBXL_USBCTRL_TYPE_PV &&
+        usbctrl->type != LIBXL_USBCTRL_TYPE_QUSB) {
         LOG(ERROR, "Unsupported USB controller type");
         rc = ERROR_FAIL;
         goto out;
@@ -252,7 +283,8 @@ void libxl__initiate_device_usbctrl_remove(libxl__egc *egc,
     rc = libxl_device_usbctrl_getinfo(CTX, domid, &usbctrl, &usbctrlinfo);
     if (rc) goto out;
 
-    if (usbctrlinfo.type != LIBXL_USBCTRL_TYPE_PV) {
+    if (usbctrlinfo.type != LIBXL_USBCTRL_TYPE_PV &&
+        usbctrlinfo.type != LIBXL_USBCTRL_TYPE_QUSB) {
         LOG(ERROR, "Unsupported USB controller type");
         rc = ERROR_FAIL;
         goto out;
@@ -293,6 +325,7 @@ static const char *vusb_be_from_xs_fe(libxl__gc *gc, const char *fe_path,
     const char *be_path;
     int r;
     uint32_t be_domid, fe_domid;
+    char be_type[16];
 
     r = libxl__xs_read_checked(gc, XBT_NULL, GCSPRINTF("%s/backend", fe_path),
                                &be_path);
@@ -300,10 +333,10 @@ static const char *vusb_be_from_xs_fe(libxl__gc *gc, const char *fe_path,
 
     /* Check to see that it has the proper form, and that fe_domid ==
      * target domid */
-    r = sscanf(be_path, "/local/domain/%d/backend/vusb/%d",
-               &be_domid, &fe_domid);
+    r = sscanf(be_path, "/local/domain/%d/backend/%15[^/]/%d",
+               &be_domid, be_type, &fe_domid);
 
-    if (r != 2 || fe_domid != tgt_domid) {
+    if (r != 3 || fe_domid != tgt_domid) {
         LOG(ERROR, "Malformed backend, refusing to use");
         return NULL;
     }
@@ -740,8 +773,9 @@ libxl__device_usbdev_set_default_usbctrl(libxl__gc *gc, uint32_t domid,
         for (j = 0; j < usbctrls[i].ports; j++) {
             const char *path, *tmp;
 
-            path = GCSPRINTF("%s/backend/vusb/%d/%d/port/%d",
+            path = GCSPRINTF("%s/backend/%s/%d/%d/port/%d",
                              libxl__xs_get_dompath(gc, LIBXL_TOOLSTACK_DOMID),
+                             pvusb_get_device_type(usbctrls[i].type),
                              domid, usbctrls[i].devid, j + 1);
             rc = libxl__xs_read_checked(gc, XBT_NULL, path, &tmp);
             if (rc) goto out;
@@ -883,11 +917,12 @@ out:
 
 /* Add usb information to xenstore
  *
- * Adding a usb device won't create new 'vusb' device, but only write
+ * Adding a usb device won't create new 'qusb'/'vusb' device, but only write
  * the device busid to the controller:port in xenstore.
  */
 static int libxl__device_usbdev_add_xenstore(libxl__gc *gc, uint32_t domid,
                                              libxl_device_usbdev *usbdev,
+                                             libxl_usbctrl_type type,
                                              bool update_json)
 {
     char *be_path, *busid;
@@ -931,8 +966,9 @@ static int libxl__device_usbdev_add_xenstore(libxl__gc *gc, uint32_t domid,
             if (rc) goto out;
         }
 
-        be_path = GCSPRINTF("%s/backend/vusb/%d/%d/port/%d",
+        be_path = GCSPRINTF("%s/backend/%s/%d/%d/port/%d",
                             libxl__xs_get_dompath(gc, LIBXL_TOOLSTACK_DOMID),
+                            pvusb_get_device_type(type),
                             domid, usbdev->ctrl, usbdev->port);
 
         LOG(DEBUG, "Adding usb device %s to xenstore: controller %d, port %d",
@@ -956,12 +992,14 @@ out:
 }
 
 static int libxl__device_usbdev_remove_xenstore(libxl__gc *gc, uint32_t domid,
-                                                libxl_device_usbdev *usbdev)
+                                                libxl_device_usbdev *usbdev,
+                                                libxl_usbctrl_type type)
 {
     char *be_path;
 
-    be_path = GCSPRINTF("%s/backend/vusb/%d/%d/port/%d",
+    be_path = GCSPRINTF("%s/backend/%s/%d/%d/port/%d",
                         libxl__xs_get_dompath(gc, LIBXL_TOOLSTACK_DOMID),
+                        pvusb_get_device_type(type),
                         domid, usbdev->ctrl, usbdev->port);
 
     LOG(DEBUG, "Removing usb device from xenstore: controller %d, port %d",
@@ -971,12 +1009,14 @@ static int libxl__device_usbdev_remove_xenstore(libxl__gc *gc, uint32_t domid,
 }
 
 static char *usbdev_busid_from_ctrlport(libxl__gc *gc, uint32_t domid,
-                                        libxl_device_usbdev *usbdev)
+                                        libxl_device_usbdev *usbdev,
+                                        libxl_usbctrl_type type)
 {
     return libxl__xs_read(gc, XBT_NULL,
-                          GCSPRINTF("%s/backend/vusb/%d/%d/port/%d",
+                          GCSPRINTF("%s/backend/%s/%d/%d/port/%d",
                               libxl__xs_get_dompath(gc, LIBXL_TOOLSTACK_DOMID),
-                          domid, usbdev->ctrl, usbdev->port));
+                              pvusb_get_device_type(type),
+                              domid, usbdev->ctrl, usbdev->port));
 }
 
 /* get original driver path of usb interface, stored in @drvpath */
@@ -1334,15 +1374,25 @@ static int do_usbdev_add(libxl__gc *gc, uint32_t domid,
             goto out;
         }
 
-        rc = libxl__device_usbdev_add_xenstore(gc, domid, usbdev, update_json);
+        rc = libxl__device_usbdev_add_xenstore(gc, domid, usbdev,
+                                               LIBXL_USBCTRL_TYPE_PV,
+                                               update_json);
         if (rc) goto out;
 
         rc = usbback_dev_assign(gc, busid);
         if (rc) {
-            libxl__device_usbdev_remove_xenstore(gc, domid, usbdev);
+            libxl__device_usbdev_remove_xenstore(gc, domid, usbdev,
+                                                 LIBXL_USBCTRL_TYPE_PV);
             goto out;
         }
         break;
+    case LIBXL_USBCTRL_TYPE_QUSB:
+        rc = libxl__device_usbdev_add_xenstore(gc, domid, usbdev,
+                                               LIBXL_USBCTRL_TYPE_QUSB,
+                                               update_json);
+        if (rc) goto out;
+
+        break;
     case LIBXL_USBCTRL_TYPE_DEVICEMODEL:
     default:
         LOG(ERROR, "Unsupported usb controller type");
@@ -1459,7 +1509,7 @@ static int do_usbdev_remove(libxl__gc *gc, uint32_t domid,
 
     switch (usbctrlinfo.type) {
     case LIBXL_USBCTRL_TYPE_PV:
-        busid = usbdev_busid_from_ctrlport(gc, domid, usbdev);
+        busid = usbdev_busid_from_ctrlport(gc, domid, usbdev, usbctrlinfo.type);
         if (!busid) {
             rc = ERROR_FAIL;
             goto out;
@@ -1468,13 +1518,20 @@ static int do_usbdev_remove(libxl__gc *gc, uint32_t domid,
         rc = usbback_dev_unassign(gc, busid);
         if (rc) goto out;
 
-        rc = libxl__device_usbdev_remove_xenstore(gc, domid, usbdev);
+        rc = libxl__device_usbdev_remove_xenstore(gc, domid, usbdev,
+                                                  LIBXL_USBCTRL_TYPE_PV);
         if (rc) goto out;
 
         rc = usbdev_rebind(gc, busid);
         if (rc) goto out;
 
         break;
+    case LIBXL_USBCTRL_TYPE_QUSB:
+        rc = libxl__device_usbdev_remove_xenstore(gc, domid, usbdev,
+                                                  LIBXL_USBCTRL_TYPE_QUSB);
+        if (rc) goto out;
+
+        break;
     case LIBXL_USBCTRL_TYPE_DEVICEMODEL:
     default:
         LOG(ERROR, "Unsupported usb controller type");
@@ -1559,7 +1616,6 @@ int libxl_ctrlport_to_device_usbdev(libxl_ctx *ctx,
     dompath = libxl__xs_get_dompath(gc, domid);
 
     fe_path = GCSPRINTF("%s/device/vusb/%d", dompath, ctrl);
-
     be_path = vusb_be_from_xs_fe(gc, fe_path, domid);
     if (!be_path) {
         rc = ERROR_FAIL;
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index b0a9776..8934276 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -618,6 +618,7 @@ libxl_usbctrl_type = Enumeration("usbctrl_type", [
     (0, "AUTO"),
     (1, "PV"),
     (2, "DEVICEMODEL"),
+    (3, "QUSB"),
     ])
 
 libxl_usbdev_type = Enumeration("usbdev_type", [
diff --git a/tools/libxl/libxl_types_internal.idl b/tools/libxl/libxl_types_internal.idl
index 696f5f8..177f9b7 100644
--- a/tools/libxl/libxl_types_internal.idl
+++ b/tools/libxl/libxl_types_internal.idl
@@ -23,6 +23,7 @@ libxl__device_kind = Enumeration("device_kind", [
     (7, "CONSOLE"),
     (8, "VTPM"),
     (9, "VUSB"),
+    (10, "QUSB"),
     ])
 
 libxl__console_backend = Enumeration("console_backend", [
-- 
2.6.2


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 3/3] libxl: add new pvusb backend "qusb" provided by qemu

[George Dunlap]

On Thu, Mar 10, 2016 at 3:00 PM, Juergen Gross <jgross@suse.com> wrote:
> Add a new pvusb backend type "qusb" which is provided by qemu. It can
> be selected either by specifying the type directly in the configuration
> or it is selected automatically by libxl in case there is no "usbback"
> driver loaded.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>

It would be awesome if we could get a patch like this in for 4.7.

However...

> ---
>  docs/man/xl.cfg.pod.5                |  11 +++-
>  tools/libxl/libxl_device.c           |   3 +-
>  tools/libxl/libxl_dm.c               |   8 +++
>  tools/libxl/libxl_internal.h         |   1 +
>  tools/libxl/libxl_pvusb.c            | 102 +++++++++++++++++++++++++++--------
>  tools/libxl/libxl_types.idl          |   1 +
>  tools/libxl/libxl_types_internal.idl |   1 +
>  7 files changed, 101 insertions(+), 26 deletions(-)
>
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index 1dde66b..06eeb42 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -737,8 +737,15 @@ Possible B<KEY>s are:
>
>  =item B<type=TYPE>
>
> -Specifies the usb controller type.  Currently only 'pv' and 'auto'
> -are supported.
> +Specifies the usb controller type.
> +
> +"pv" denotes a kernel based pvusb backend.
> +
> +"qusb" specifies a qemu base backend for pvusb.
> +
> +"auto" (the default) determines whether a kernel based backend is installed.
> +If this is the case, "pv" is selected, "qusb" will be selected if no kernel
> +backend is currently available.

I'm going back and forth a bit about whether having a "qusb" option
here makes the most sense, or whether it makes more sense to have a
separate option, like "backendtype" (as we have for disks).

The reason I suggested having "type" here at all is that libxl cannot
know ahead of time whether the guest has pvusbfront available.  It
*can*, however, tell whether pvusbback is available (as your code
demonstrates).

On the other hand -- given that qemu will shortly  have pvusbback, I'm
wondering if it's possible to make pvusbfront do something similar to
what the blkfront drivers do -- write a magic port that tell qemu,
"Actually, I have pvusb drivers; unplug the emulated controller."

From a UI point of view that would be the best, right?  Just like
disks or network cards -- you can simply say you want a controller,
and you automatically get the most appropriate kind based on what the
kernel you run actually has available.

I think you've been working on the pvusbfront drivers for Linux, as
well as the qusb backend in qemu -- do you think that's workable?

If so, I think adding 'qusb' to this field is probably the right thing to do.

One technical question...

> @@ -1559,7 +1616,6 @@ int libxl_ctrlport_to_device_usbdev(libxl_ctx *ctx,
>      dompath = libxl__xs_get_dompath(gc, domid);
>
>      fe_path = GCSPRINTF("%s/device/vusb/%d", dompath, ctrl);

Did you miss a vusb -> %s here?

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 3/3] libxl: add new pvusb backend "qusb" provided by qemu

[Juergen Gross]

On 17/03/16 17:39, George Dunlap wrote:
> On Thu, Mar 10, 2016 at 3:00 PM, Juergen Gross <jgross@suse.com> wrote:
>> Add a new pvusb backend type "qusb" which is provided by qemu. It can
>> be selected either by specifying the type directly in the configuration
>> or it is selected automatically by libxl in case there is no "usbback"
>> driver loaded.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> It would be awesome if we could get a patch like this in for 4.7.
> 
> However...
> 
>> ---
>>  docs/man/xl.cfg.pod.5                |  11 +++-
>>  tools/libxl/libxl_device.c           |   3 +-
>>  tools/libxl/libxl_dm.c               |   8 +++
>>  tools/libxl/libxl_internal.h         |   1 +
>>  tools/libxl/libxl_pvusb.c            | 102 +++++++++++++++++++++++++++--------
>>  tools/libxl/libxl_types.idl          |   1 +
>>  tools/libxl/libxl_types_internal.idl |   1 +
>>  7 files changed, 101 insertions(+), 26 deletions(-)
>>
>> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
>> index 1dde66b..06eeb42 100644
>> --- a/docs/man/xl.cfg.pod.5
>> +++ b/docs/man/xl.cfg.pod.5
>> @@ -737,8 +737,15 @@ Possible B<KEY>s are:
>>
>>  =item B<type=TYPE>
>>
>> -Specifies the usb controller type.  Currently only 'pv' and 'auto'
>> -are supported.
>> +Specifies the usb controller type.
>> +
>> +"pv" denotes a kernel based pvusb backend.
>> +
>> +"qusb" specifies a qemu base backend for pvusb.
>> +
>> +"auto" (the default) determines whether a kernel based backend is installed.
>> +If this is the case, "pv" is selected, "qusb" will be selected if no kernel
>> +backend is currently available.
> 
> I'm going back and forth a bit about whether having a "qusb" option
> here makes the most sense, or whether it makes more sense to have a
> separate option, like "backendtype" (as we have for disks).
> 
> The reason I suggested having "type" here at all is that libxl cannot
> know ahead of time whether the guest has pvusbfront available.  It
> *can*, however, tell whether pvusbback is available (as your code
> demonstrates).
> 
> On the other hand -- given that qemu will shortly  have pvusbback, I'm
> wondering if it's possible to make pvusbfront do something similar to
> what the blkfront drivers do -- write a magic port that tell qemu,
> "Actually, I have pvusb drivers; unplug the emulated controller."
> 
> From a UI point of view that would be the best, right?  Just like
> disks or network cards -- you can simply say you want a controller,
> and you automatically get the most appropriate kind based on what the
> kernel you run actually has available.
> 
> I think you've been working on the pvusbfront drivers for Linux, as
> well as the qusb backend in qemu -- do you think that's workable?

Hmm, could work. This would require the frontend to register rather
early at boot in order to bind the devices to it before the emulated USB
connector can grab them, right? Or would the emulated USB devices seem
to be unplugged from the emulated adapter and then again plugged in via
the pvusb frontend?

> If so, I think adding 'qusb' to this field is probably the right thing to do.
> 
> One technical question...
> 
>> @@ -1559,7 +1616,6 @@ int libxl_ctrlport_to_device_usbdev(libxl_ctx *ctx,
>>      dompath = libxl__xs_get_dompath(gc, domid);
>>
>>      fe_path = GCSPRINTF("%s/device/vusb/%d", dompath, ctrl);
> 
> Did you miss a vusb -> %s here?

No. The frontend won't need to know whether the backend is kernel or
qemu based.


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 3/3] libxl: add new pvusb backend "qusb" provided by qemu

[George Dunlap]

On Thu, Mar 10, 2016 at 3:00 PM, Juergen Gross <jgross@suse.com> wrote:
> Add a new pvusb backend type "qusb" which is provided by qemu. It can
> be selected either by specifying the type directly in the configuration
> or it is selected automatically by libxl in case there is no "usbback"
> driver loaded.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>

Oh, last thing -- shouldn't we have a way for libxl to tell whether
qemu can provide a qusb backend before we offer to start it up?

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 3/3] libxl: add new pvusb backend "qusb" provided by qemu

[Juergen Gross]

On 17/03/16 17:55, George Dunlap wrote:
> On Thu, Mar 10, 2016 at 3:00 PM, Juergen Gross <jgross@suse.com> wrote:
>> Add a new pvusb backend type "qusb" which is provided by qemu. It can
>> be selected either by specifying the type directly in the configuration
>> or it is selected automatically by libxl in case there is no "usbback"
>> driver loaded.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> Oh, last thing -- shouldn't we have a way for libxl to tell whether
> qemu can provide a qusb backend before we offer to start it up?

How? Is this possible for qdisk? Or for an emulated frame buffer?


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 3/3] libxl: add new pvusb backend "qusb" provided by qemu

[Wei Liu]

On Fri, Mar 18, 2016 at 09:24:26AM +0100, Juergen Gross wrote:
> On 17/03/16 17:55, George Dunlap wrote:
> > On Thu, Mar 10, 2016 at 3:00 PM, Juergen Gross <jgross@suse.com> wrote:
> >> Add a new pvusb backend type "qusb" which is provided by qemu. It can
> >> be selected either by specifying the type directly in the configuration
> >> or it is selected automatically by libxl in case there is no "usbback"
> >> driver loaded.
> >>
> >> Signed-off-by: Juergen Gross <jgross@suse.com>
> > 
> > Oh, last thing -- shouldn't we have a way for libxl to tell whether
> > qemu can provide a qusb backend before we offer to start it up?
> 
> How? Is this possible for qdisk? Or for an emulated frame buffer?
> 

I don't think it is  possible at the moment. But I think you might be
able to do it either via qmp command or parsing qemu help string. The
latter functionality is implemented in Ian's device model deprivilege
patch series IIRC -- but it is not yet merged either.

Wei.

> 
> Juergen
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 3/3] libxl: add new pvusb backend "qusb" provided by qemu

[Juergen Gross]

On 24/03/16 21:07, Wei Liu wrote:
> On Fri, Mar 18, 2016 at 09:24:26AM +0100, Juergen Gross wrote:
>> On 17/03/16 17:55, George Dunlap wrote:
>>> On Thu, Mar 10, 2016 at 3:00 PM, Juergen Gross <jgross@suse.com> wrote:
>>>> Add a new pvusb backend type "qusb" which is provided by qemu. It can
>>>> be selected either by specifying the type directly in the configuration
>>>> or it is selected automatically by libxl in case there is no "usbback"
>>>> driver loaded.
>>>>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>
>>> Oh, last thing -- shouldn't we have a way for libxl to tell whether
>>> qemu can provide a qusb backend before we offer to start it up?
>>
>> How? Is this possible for qdisk? Or for an emulated frame buffer?
>>
> 
> I don't think it is  possible at the moment. But I think you might be
> able to do it either via qmp command or parsing qemu help string. The
> latter functionality is implemented in Ian's device model deprivilege
> patch series IIRC -- but it is not yet merged either.

Won't help for qusb, as there is no qemu parameter or help text related
to the backend. It would be possible to add a qmp command to print the
supported backend types, of course.

Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 3/3] libxl: add new pvusb backend "qusb" provided by qemu

[George Dunlap]

On Fri, Mar 25, 2016 at 6:09 AM, Juergen Gross <jgross@suse.com> wrote:
> On 24/03/16 21:07, Wei Liu wrote:
>> On Fri, Mar 18, 2016 at 09:24:26AM +0100, Juergen Gross wrote:
>>> On 17/03/16 17:55, George Dunlap wrote:
>>>> On Thu, Mar 10, 2016 at 3:00 PM, Juergen Gross <jgross@suse.com> wrote:
>>>>> Add a new pvusb backend type "qusb" which is provided by qemu. It can
>>>>> be selected either by specifying the type directly in the configuration
>>>>> or it is selected automatically by libxl in case there is no "usbback"
>>>>> driver loaded.
>>>>>
>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>
>>>> Oh, last thing -- shouldn't we have a way for libxl to tell whether
>>>> qemu can provide a qusb backend before we offer to start it up?
>>>
>>> How? Is this possible for qdisk? Or for an emulated frame buffer?
>>>
>>
>> I don't think it is  possible at the moment. But I think you might be
>> able to do it either via qmp command or parsing qemu help string. The
>> latter functionality is implemented in Ian's device model deprivilege
>> patch series IIRC -- but it is not yet merged either.
>
> Won't help for qusb, as there is no qemu parameter or help text related
> to the backend. It would be possible to add a qmp command to print the
> supported backend types, of course.

Well as I understand it, for qemu-upstream we can't assume what
version of qemu we'll end up getting: we need to be compatible with
recent releases of qemu which don't provide qusb.  Which I'm pretty
sure means failing gracefully (i.e., giving a sensible warning
message) if someone tries to add qusb when their qemu binary doesn't
support it.

If you can tell from the qemu failure that qusb is at fault, that's OK
I think; but if not, I think you'll have to add some way to query
whether qusb is supported by the qemu binary before starting it.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 3/3] libxl: add new pvusb backend "qusb" provided by qemu

[Juergen Gross]

On 29/03/16 11:45, George Dunlap wrote:
> On Fri, Mar 25, 2016 at 6:09 AM, Juergen Gross <jgross@suse.com> wrote:
>> On 24/03/16 21:07, Wei Liu wrote:
>>> On Fri, Mar 18, 2016 at 09:24:26AM +0100, Juergen Gross wrote:
>>>> On 17/03/16 17:55, George Dunlap wrote:
>>>>> On Thu, Mar 10, 2016 at 3:00 PM, Juergen Gross <jgross@suse.com> wrote:
>>>>>> Add a new pvusb backend type "qusb" which is provided by qemu. It can
>>>>>> be selected either by specifying the type directly in the configuration
>>>>>> or it is selected automatically by libxl in case there is no "usbback"
>>>>>> driver loaded.
>>>>>>
>>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>>
>>>>> Oh, last thing -- shouldn't we have a way for libxl to tell whether
>>>>> qemu can provide a qusb backend before we offer to start it up?
>>>>
>>>> How? Is this possible for qdisk? Or for an emulated frame buffer?
>>>>
>>>
>>> I don't think it is  possible at the moment. But I think you might be
>>> able to do it either via qmp command or parsing qemu help string. The
>>> latter functionality is implemented in Ian's device model deprivilege
>>> patch series IIRC -- but it is not yet merged either.
>>
>> Won't help for qusb, as there is no qemu parameter or help text related
>> to the backend. It would be possible to add a qmp command to print the
>> supported backend types, of course.
> 
> Well as I understand it, for qemu-upstream we can't assume what
> version of qemu we'll end up getting: we need to be compatible with
> recent releases of qemu which don't provide qusb.  Which I'm pretty
> sure means failing gracefully (i.e., giving a sensible warning
> message) if someone tries to add qusb when their qemu binary doesn't
> support it.
> 
> If you can tell from the qemu failure that qusb is at fault, that's OK
> I think; but if not, I think you'll have to add some way to query
> whether qusb is supported by the qemu binary before starting it.

So we would want a qemu command line option (e.g. -xen-help) printing
the supported pv backends. I think this should be easy to setup.


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 3/3] libxl: add new pvusb backend "qusb" provided by qemu

[Juergen Gross]

On 29/03/16 12:35, Juergen Gross wrote:
> On 29/03/16 11:45, George Dunlap wrote:
>> On Fri, Mar 25, 2016 at 6:09 AM, Juergen Gross <jgross@suse.com> wrote:
>>> On 24/03/16 21:07, Wei Liu wrote:
>>>> On Fri, Mar 18, 2016 at 09:24:26AM +0100, Juergen Gross wrote:
>>>>> On 17/03/16 17:55, George Dunlap wrote:
>>>>>> On Thu, Mar 10, 2016 at 3:00 PM, Juergen Gross <jgross@suse.com> wrote:
>>>>>>> Add a new pvusb backend type "qusb" which is provided by qemu. It can
>>>>>>> be selected either by specifying the type directly in the configuration
>>>>>>> or it is selected automatically by libxl in case there is no "usbback"
>>>>>>> driver loaded.
>>>>>>>
>>>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>>>
>>>>>> Oh, last thing -- shouldn't we have a way for libxl to tell whether
>>>>>> qemu can provide a qusb backend before we offer to start it up?
>>>>>
>>>>> How? Is this possible for qdisk? Or for an emulated frame buffer?
>>>>>
>>>>
>>>> I don't think it is  possible at the moment. But I think you might be
>>>> able to do it either via qmp command or parsing qemu help string. The
>>>> latter functionality is implemented in Ian's device model deprivilege
>>>> patch series IIRC -- but it is not yet merged either.
>>>
>>> Won't help for qusb, as there is no qemu parameter or help text related
>>> to the backend. It would be possible to add a qmp command to print the
>>> supported backend types, of course.
>>
>> Well as I understand it, for qemu-upstream we can't assume what
>> version of qemu we'll end up getting: we need to be compatible with
>> recent releases of qemu which don't provide qusb.  Which I'm pretty
>> sure means failing gracefully (i.e., giving a sensible warning
>> message) if someone tries to add qusb when their qemu binary doesn't
>> support it.
>>
>> If you can tell from the qemu failure that qusb is at fault, that's OK
>> I think; but if not, I think you'll have to add some way to query
>> whether qusb is supported by the qemu binary before starting it.
> 
> So we would want a qemu command line option (e.g. -xen-help) printing
> the supported pv backends. I think this should be easy to setup.

Actually there is a solution which is _really_ simple: before setting
it's state to "running" in xenstore, qemu could write the supported
backend types to xenstore, too. I'd add a xenstore directory

/local/domain/<backend-domid>/device-model/<domid>/backends

containing a directory for each supported backend. By using a directory
for each backend it would even be possible to indicate supported
features and/or parameters in the future.

"qusb" backend would be regarded to be supported only if
/local/domain/<backend-domid>/device-model/<domid>/backends/qusb
is existing.

"qdisk" would be regarded to be supported if either:
- /local/domain/<backend-domid>/device-model/<domid>/backends/qdisk
  is existing or:
- /local/domain/<backend-domid>/device-model/<domid>/backends
  is not existing.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel