Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [Xen-devel] [PATCH v1 0/2] Remove backend xen store entry on domain destroy
@ 2019-10-08 14:10 Oleksandr Grytsov
  2019-10-08 14:10 ` [Xen-devel] [PATCH v1 1/2] libxl: introduce new backend type VINPUT Oleksandr Grytsov
  2019-10-08 14:10 ` [Xen-devel] [PATCH v1 2/2] libxl: add removing XS backend path for PV devices on domain destroy Oleksandr Grytsov
  0 siblings, 2 replies; 12+ messages in thread
From: Oleksandr Grytsov @ 2019-10-08 14:10 UTC (permalink / raw)
  To: xen-devel; +Cc: Oleksandr Grytsov, ian.jackson, wl

From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

We have added VKBD device with user space PV backend. But libxl can't
differentiate domain kind for this device. As result, it performs QEMU procedure
for adding and removing VKB device with user space backend. To fix this issue,
new device kind VINPUT is introduced. It is used as backend kind in case of
user space VKBD backend. 

Another issue addresses in this patch series is error timeout on guest domain
destroy in case using user space PV backends.

We have a driver domain with user space backends (VDISPL, VSND, VKBD). When a
guest domain destroyed, we see following error:

    "timed out while waiting for ... to be removed"

xl expects that PV device backend entries is removed. xl devd removes these
entries for specific devices only: VBD, VIF, QDISK and then deletes the guest
domain from the list. The fix is to delete guest domain only after all devices
are removed and performs generic device remove procedure for following device:
VINPUT, VDISPL, VSND.

Oleksandr Grytsov (2):
  libxl: introduce new backend type VINPUT
  libxl: add removing XS backend path for PV devices on domain destroy

 tools/libxl/libxl_device.c           | 14 +++++---------
 tools/libxl/libxl_types_internal.idl |  1 +
 tools/libxl/libxl_vkb.c              | 29 ++++++++++++++++++----------
 3 files changed, 25 insertions(+), 19 deletions(-)

-- 
2.17.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Xen-devel] [PATCH v1 1/2] libxl: introduce new backend type VINPUT
  2019-10-08 14:10 [Xen-devel] [PATCH v1 0/2] Remove backend xen store entry on domain destroy Oleksandr Grytsov
@ 2019-10-08 14:10 ` Oleksandr Grytsov
  2019-10-11 14:58   ` Ian Jackson
  2019-10-08 14:10 ` [Xen-devel] [PATCH v1 2/2] libxl: add removing XS backend path for PV devices on domain destroy Oleksandr Grytsov
  1 sibling, 1 reply; 12+ messages in thread
From: Oleksandr Grytsov @ 2019-10-08 14:10 UTC (permalink / raw)
  To: xen-devel; +Cc: Oleksandr Grytsov, ian.jackson, wl

From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

There are two kind of VKBD devices: with QEMU backend and user space
backend. In current implementation they can't be distinguished as both use
VKBD backend type. As result, user space KBD backend is started and
stopped as QEMU backend. This commit adds new device kind VINPUT to be
used as backend type for user space KBD backend.

Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
---
 tools/libxl/libxl_types_internal.idl |  1 +
 tools/libxl/libxl_vkb.c              | 29 ++++++++++++++++++----------
 2 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/tools/libxl/libxl_types_internal.idl b/tools/libxl/libxl_types_internal.idl
index cb85c3b37f..3593e21dbb 100644
--- a/tools/libxl/libxl_types_internal.idl
+++ b/tools/libxl/libxl_types_internal.idl
@@ -31,6 +31,7 @@ libxl__device_kind = Enumeration("device_kind", [
     (13, "VUART"),
     (14, "PVCALLS"),
     (15, "VSND"),
+    (16, "VINPUT"),
     ])
 
 libxl__console_backend = Enumeration("console_backend", [
diff --git a/tools/libxl/libxl_vkb.c b/tools/libxl/libxl_vkb.c
index 26376a7eef..4c44a813c1 100644
--- a/tools/libxl/libxl_vkb.c
+++ b/tools/libxl/libxl_vkb.c
@@ -38,9 +38,6 @@ static int libxl__set_xenstore_vkb(libxl__gc *gc, uint32_t domid,
                                    flexarray_t *back, flexarray_t *front,
                                    flexarray_t *ro_front)
 {
-    flexarray_append_pair(back, "backend-type",
-                          (char *)libxl_vkb_backend_to_string(vkb->backend_type));
-
     if (vkb->unique_id) {
         flexarray_append_pair(back, XENKBD_FIELD_UNIQUE_ID, vkb->unique_id);
     }
@@ -93,7 +90,8 @@ static int libxl__vkb_from_xenstore(libxl__gc *gc, const char *libxl_path,
                                     libxl_devid devid,
                                     libxl_device_vkb *vkb)
 {
-    const char *be_path, *be_type, *fe_path, *tmp;
+    const char *be_path, *fe_path, *tmp;
+    libxl__device dev;
     int rc;
 
     vkb->devid = devid;
@@ -111,13 +109,11 @@ static int libxl__vkb_from_xenstore(libxl__gc *gc, const char *libxl_path,
     rc = libxl__backendpath_parse_domid(gc, be_path, &vkb->backend_domid);
     if (rc) goto out;
 
-    rc = libxl__xs_read_mandatory(gc, XBT_NULL,
-                                  GCSPRINTF("%s/backend-type", be_path),
-                                  &be_type);
+    rc = libxl__parse_backend_path(gc, be_path, &dev);
     if (rc) goto out;
 
-    rc = libxl_vkb_backend_from_string(be_type, &vkb->backend_type);
-    if (rc) goto out;
+    vkb->backend_type = dev.backend_kind == LIBXL__DEVICE_KIND_VINPUT ?
+                                            LIBXL_VKB_BACKEND_LINUX : LIBXL_VKB_BACKEND_QEMU;
 
     vkb->unique_id = xs_read(CTX->xsh, XBT_NULL, GCSPRINTF("%s/"XENKBD_FIELD_UNIQUE_ID, be_path), NULL);
 
@@ -218,6 +214,20 @@ out:
     return rc;
 }
 
+static int libxl__device_from_vkb(libxl__gc *gc, uint32_t domid,
+                                  libxl_device_vkb *type, libxl__device *device)
+{
+    device->backend_devid   = type->devid;
+    device->backend_domid   = type->backend_domid;
+    device->backend_kind    = type->backend_type == LIBXL_VKB_BACKEND_LINUX ?
+                              LIBXL__DEVICE_KIND_VINPUT : LIBXL__DEVICE_KIND_VKBD;
+    device->devid           = type->devid;
+    device->domid           = domid;
+    device->kind            = LIBXL__DEVICE_KIND_VKBD;
+
+    return 0;
+}
+
 int libxl_device_vkb_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vkb *vkb,
                          const libxl_asyncop_how *ao_how)
 {
@@ -318,7 +328,6 @@ out:
      return rc;
 }
 
-static LIBXL_DEFINE_DEVICE_FROM_TYPE(vkb)
 static LIBXL_DEFINE_UPDATE_DEVID(vkb)
 
 #define libxl__add_vkbs NULL
-- 
2.17.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Xen-devel] [PATCH v1 2/2] libxl: add removing XS backend path for PV devices on domain destroy
  2019-10-08 14:10 [Xen-devel] [PATCH v1 0/2] Remove backend xen store entry on domain destroy Oleksandr Grytsov
  2019-10-08 14:10 ` [Xen-devel] [PATCH v1 1/2] libxl: introduce new backend type VINPUT Oleksandr Grytsov
@ 2019-10-08 14:10 ` Oleksandr Grytsov
  2019-10-11 15:07   ` Ian Jackson
  1 sibling, 1 reply; 12+ messages in thread
From: Oleksandr Grytsov @ 2019-10-08 14:10 UTC (permalink / raw)
  To: xen-devel; +Cc: Oleksandr Grytsov, ian.jackson, wl

From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

Currently backend path is remove for specific devices: VBD, VIF, QDISK.
This commit adds removing backend path for: VDISPL, VSND, VINPUT.

Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
---
 tools/libxl/libxl_device.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 1402b61a81..8ce70661e5 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -1477,7 +1477,7 @@ typedef struct libxl__ddomain_device {
  */
 typedef struct libxl__ddomain_guest {
     uint32_t domid;
-    int num_vifs, num_vbds, num_qdisks;
+    int num_qdisks;
     LIBXL_SLIST_HEAD(, struct libxl__ddomain_device) devices;
     LIBXL_SLIST_ENTRY(struct libxl__ddomain_guest) next;
 } libxl__ddomain_guest;
@@ -1530,8 +1530,7 @@ static void check_and_maybe_remove_guest(libxl__gc *gc,
 {
     assert(ddomain);
 
-    if (dguest != NULL &&
-        dguest->num_vifs + dguest->num_vbds + dguest->num_qdisks == 0) {
+    if (dguest != NULL && LIBXL_SLIST_FIRST(&dguest->devices) == NULL) {
         LIBXL_SLIST_REMOVE(&ddomain->guests, dguest, libxl__ddomain_guest,
                            next);
         LOGD(DEBUG, dguest->domid, "Removed domain from the list of active guests");
@@ -1573,9 +1572,6 @@ static int add_device(libxl__egc *egc, libxl__ao *ao,
     switch(dev->backend_kind) {
     case LIBXL__DEVICE_KIND_VBD:
     case LIBXL__DEVICE_KIND_VIF:
-        if (dev->backend_kind == LIBXL__DEVICE_KIND_VBD) dguest->num_vbds++;
-        if (dev->backend_kind == LIBXL__DEVICE_KIND_VIF) dguest->num_vifs++;
-
         GCNEW(aodev);
         libxl__prepare_ao_device(ao, aodev);
         /*
@@ -1619,11 +1615,11 @@ static int remove_device(libxl__egc *egc, libxl__ao *ao,
     int rc = 0;
 
     switch(ddev->dev->backend_kind) {
+    case LIBXL__DEVICE_KIND_VDISPL:
+    case LIBXL__DEVICE_KIND_VSND:
+    case LIBXL__DEVICE_KIND_VINPUT:
     case LIBXL__DEVICE_KIND_VBD:
     case LIBXL__DEVICE_KIND_VIF:
-        if (dev->backend_kind == LIBXL__DEVICE_KIND_VBD) dguest->num_vbds--;
-        if (dev->backend_kind == LIBXL__DEVICE_KIND_VIF) dguest->num_vifs--;
-
         GCNEW(aodev);
         libxl__prepare_ao_device(ao, aodev);
         /*
-- 
2.17.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Xen-devel] [PATCH v1 1/2] libxl: introduce new backend type VINPUT
  2019-10-08 14:10 ` [Xen-devel] [PATCH v1 1/2] libxl: introduce new backend type VINPUT Oleksandr Grytsov
@ 2019-10-11 14:58   ` Ian Jackson
  2019-10-11 15:57     ` Oleksandr Grytsov
  0 siblings, 1 reply; 12+ messages in thread
From: Ian Jackson @ 2019-10-11 14:58 UTC (permalink / raw)
  To: Oleksandr Grytsov; +Cc: xen-devel, Oleksandr Grytsov, wl

Oleksandr Grytsov writes ("[PATCH v1 1/2] libxl: introduce new backend type VINPUT"):
> From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
> 
> There are two kind of VKBD devices: with QEMU backend and user space
> backend. In current implementation they can't be distinguished as both use
> VKBD backend type. As result, user space KBD backend is started and
> stopped as QEMU backend. This commit adds new device kind VINPUT to be
> used as backend type for user space KBD backend.

I find this confusing.  I'm not sure this change is right.  But I'm
afraid the original patches in this area passed me by so I don't know
much about it.

I think it was a48e00f14a2d "libxl: add backend type and id to vkb"
which introduced what you are calling "user space" backends.  It
appears that the vkb backend type enum was introduced there
specifically to distinguish between these two situations.  For reasons

Am I wrong ?  If not, why is this not working or not suitable ?

I also don't understand why the "user space" kbd backend seems to be
"linux" in the enum.  Where is the implementation of this user space
backend ?  Is it be controlled entirely through xenstore ?

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Xen-devel] [PATCH v1 2/2] libxl: add removing XS backend path for PV devices on domain destroy
  2019-10-08 14:10 ` [Xen-devel] [PATCH v1 2/2] libxl: add removing XS backend path for PV devices on domain destroy Oleksandr Grytsov
@ 2019-10-11 15:07   ` Ian Jackson
  2019-10-11 15:32     ` Roger Pau Monné
  0 siblings, 1 reply; 12+ messages in thread
From: Ian Jackson @ 2019-10-11 15:07 UTC (permalink / raw)
  To: Oleksandr Grytsov, Roger Pau Monne; +Cc: xen-devel, Oleksandr Grytsov, wl

Oleksandr Grytsov writes ("[PATCH v1 2/2] libxl: add removing XS backend path for PV devices on domain destroy"):
> From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
> 
> Currently backend path is remove for specific devices: VBD, VIF, QDISK.
> This commit adds removing backend path for: VDISPL, VSND, VINPUT.

Thanks for this and your previous patch.

This one looks to me like it is probably correct.  But I have not been
able to understand why this code was limited to vbds and vifs before
so I am hesitant.

Searching the git history, I think this has been like this since
a0eaa86e7537
 "libxl: add device backend listener in order to launch backends"
and subsequent commits have just reorganised things but not
fundamentally changed them.  I've CC'd Roger who wrote this code.

I think:

>      switch(ddev->dev->backend_kind) {
> +    case LIBXL__DEVICE_KIND_VDISPL:
> +    case LIBXL__DEVICE_KIND_VSND:
> +    case LIBXL__DEVICE_KIND_VINPUT:
>      case LIBXL__DEVICE_KIND_VBD:
>      case LIBXL__DEVICE_KIND_VIF:

If we do want this to handle *all* kinds of device, maybe it should
have a fallback that aborts, or something ?  (I don't think it is
easy to make it a compile error to forget to add an entry here but if
we could do that it would probably be best.)

All of that assuming that the basic idea is right, which I would like
Roger's opinion about.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Xen-devel] [PATCH v1 2/2] libxl: add removing XS backend path for PV devices on domain destroy
  2019-10-11 15:07   ` Ian Jackson
@ 2019-10-11 15:32     ` Roger Pau Monné
  2019-10-11 15:55       ` Ian Jackson
  0 siblings, 1 reply; 12+ messages in thread
From: Roger Pau Monné @ 2019-10-11 15:32 UTC (permalink / raw)
  To: Ian Jackson; +Cc: xen-devel, Oleksandr Grytsov, Oleksandr Grytsov, wl

On Fri, Oct 11, 2019 at 04:07:11PM +0100, Ian Jackson wrote:
> Oleksandr Grytsov writes ("[PATCH v1 2/2] libxl: add removing XS backend path for PV devices on domain destroy"):
> > From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
> > 
> > Currently backend path is remove for specific devices: VBD, VIF, QDISK.
> > This commit adds removing backend path for: VDISPL, VSND, VINPUT.
> 
> Thanks for this and your previous patch.
> 
> This one looks to me like it is probably correct.  But I have not been
> able to understand why this code was limited to vbds and vifs before
> so I am hesitant.
> 
> Searching the git history, I think this has been like this since
> a0eaa86e7537
>  "libxl: add device backend listener in order to launch backends"
> and subsequent commits have just reorganised things but not
> fundamentally changed them.  I've CC'd Roger who wrote this code.

When this code was added (devd) those where the only backends handled
by libxl. VDISPL, VSND, VINPUT didn't exist at that point, so I think
the issue is that when support for those was added, it wasn't properly
wired into devd.

> I think:
> 
> >      switch(ddev->dev->backend_kind) {
> > +    case LIBXL__DEVICE_KIND_VDISPL:
> > +    case LIBXL__DEVICE_KIND_VSND:
> > +    case LIBXL__DEVICE_KIND_VINPUT:
> >      case LIBXL__DEVICE_KIND_VBD:
> >      case LIBXL__DEVICE_KIND_VIF:
> 
> If we do want this to handle *all* kinds of device, maybe it should
> have a fallback that aborts, or something ?  (I don't think it is
> easy to make it a compile error to forget to add an entry here but if
> we could do that it would probably be best.)

Maybe we could have some generic handling for everything != qdisk?

IIRC qdisk needs special handling so that devd can launch and destroy
a QEMU instance when required, but other backends that run in the
kernel don't need such handling and could maybe share the same generic
code path.

> All of that assuming that the basic idea is right, which I would like
> Roger's opinion about.

Looking at the patch itself, you also seem to be doing some changes
related to num_vbds and num_vifs, but those are not mentioned in the
commit message, is that a stray change?

I'm not saying it's wrong, it's just that it feels like it belongs in
a different patch maybe.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Xen-devel] [PATCH v1 2/2] libxl: add removing XS backend path for PV devices on domain destroy
  2019-10-11 15:32     ` Roger Pau Monné
@ 2019-10-11 15:55       ` Ian Jackson
  2019-10-15 15:39         ` Roger Pau Monné
  0 siblings, 1 reply; 12+ messages in thread
From: Ian Jackson @ 2019-10-11 15:55 UTC (permalink / raw)
  To: Roger Pau Monne; +Cc: xen-devel, Oleksandr Grytsov, Oleksandr Grytsov, wl

Roger Pau Monne writes ("Re: [PATCH v1 2/2] libxl: add removing XS backend path for PV devices on domain destroy"):
> When this code was added (devd) those where the only backends handled
> by libxl. VDISPL, VSND, VINPUT didn't exist at that point, so I think
> the issue is that when support for those was added, it wasn't properly
> wired into devd.
> 
> > I think:
> > 
> > >      switch(ddev->dev->backend_kind) {
> > > +    case LIBXL__DEVICE_KIND_VDISPL:
> > > +    case LIBXL__DEVICE_KIND_VSND:
> > > +    case LIBXL__DEVICE_KIND_VINPUT:
> > >      case LIBXL__DEVICE_KIND_VBD:
> > >      case LIBXL__DEVICE_KIND_VIF:
> > 
> > If we do want this to handle *all* kinds of device, maybe it should
> > have a fallback that aborts, or something ?  (I don't think it is
> > easy to make it a compile error to forget to add an entry here but if
> > we could do that it would probably be best.)
> 
> Maybe we could have some generic handling for everything != qdisk?

So make that the "default:" ?  That would be fine by me.

> IIRC qdisk needs special handling so that devd can launch and destroy
> a QEMU instance when required, but other backends that run in the
> kernel don't need such handling and could maybe share the same generic
> code path.

Right.

> > All of that assuming that the basic idea is right, which I would like
> > Roger's opinion about.
> 
> Looking at the patch itself, you also seem to be doing some changes
> related to num_vbds and num_vifs, but those are not mentioned in the
> commit message, is that a stray change?

No, I don't think so.  Those variables just tell us when the thing is
torn down but Oleksandr's code is able to use the devices slist itself
for that.  Please do check to see if you agree.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Xen-devel] [PATCH v1 1/2] libxl: introduce new backend type VINPUT
  2019-10-11 14:58   ` Ian Jackson
@ 2019-10-11 15:57     ` Oleksandr Grytsov
  2019-10-11 17:03       ` Ian Jackson
  0 siblings, 1 reply; 12+ messages in thread
From: Oleksandr Grytsov @ 2019-10-11 15:57 UTC (permalink / raw)
  To: Ian Jackson; +Cc: xen-devel, Oleksandr Grytsov, wl

On Fri, Oct 11, 2019 at 5:58 PM Ian Jackson <ian.jackson@citrix.com> wrote:
>
> Oleksandr Grytsov writes ("[PATCH v1 1/2] libxl: introduce new backend type VINPUT"):
> > From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
> >
> > There are two kind of VKBD devices: with QEMU backend and user space
> > backend. In current implementation they can't be distinguished as both use
> > VKBD backend type. As result, user space KBD backend is started and
> > stopped as QEMU backend. This commit adds new device kind VINPUT to be
> > used as backend type for user space KBD backend.
>
> I find this confusing.  I'm not sure this change is right.  But I'm
> afraid the original patches in this area passed me by so I don't know
> much about it.
>
> I think it was a48e00f14a2d "libxl: add backend type and id to vkb"
> which introduced what you are calling "user space" backends.  It
> appears that the vkb backend type enum was introduced there
> specifically to distinguish between these two situations.  For reasons
>
> Am I wrong ?  If not, why is this not working or not suitable ?

You are right. It is not working in some cases due to QEMU_BACKEND macro.
QEMU_BACKEND treats both backends as QEMU. As result xl performs device
hotplug on add/remove device. Which is not expected in case "user
space" backend.

In this patch I propose solution similar to VUSB device. Where VUSB
used for frontend
and depends on backend (kernel or QEMU) either VUSB or QUSB used for backend
device type e.g. use different backend device type for different
backends. It would be
more clear if, for example, QEMU backend has QKBD name and all other
VKBD. But it
would require changes on QEMU side too. That's why I've chosen VINPUT name.

Introducing new backend device type for VKBD also allows to have both backends
(QEMU and non QEMU) run in same domain. Not sure if it is useful
scenario but with
this proposal it is possible from technical point of view.

>
> I also don't understand why the "user space" kbd backend seems to be
> "linux" in the enum.

I agree this is not so good name. But I don't know how to call
backends which are not running
inside QEMU in general.

> Where is the implementation of this user space
> backend ?

We have extended kbd interface (kbdif.h) to support multi-touch events
as well. And we have
implemented own kbd backend https://github.com/xen-troops/displ_be/
It is integrated with display backend as both use wayland API.

> Is it be controlled entirely through xenstore ?

Yes it is controlled entirely through xenstore: lib xl creates
frontend/backend entries with
initial states and configuration.

> Ian.

-- 
Best Regards,
Oleksandr Grytsov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Xen-devel] [PATCH v1 1/2] libxl: introduce new backend type VINPUT
  2019-10-11 15:57     ` Oleksandr Grytsov
@ 2019-10-11 17:03       ` Ian Jackson
  2019-10-16 13:26         ` Oleksandr Grytsov
  0 siblings, 1 reply; 12+ messages in thread
From: Ian Jackson @ 2019-10-11 17:03 UTC (permalink / raw)
  To: Oleksandr Grytsov; +Cc: xen-devel, Oleksandr Grytsov, wl

Oleksandr Grytsov writes ("Re: [PATCH v1 1/2] libxl: introduce new backend type VINPUT"):
> On Fri, Oct 11, 2019 at 5:58 PM Ian Jackson <ian.jackson@citrix.com> wrote:
> > I think it was a48e00f14a2d "libxl: add backend type and id to vkb"
> > which introduced what you are calling "user space" backends.  It
> > appears that the vkb backend type enum was introduced there
> > specifically to distinguish between these two situations.  For reasons
> >
> > Am I wrong ?  If not, why is this not working or not suitable ?
> 
> You are right. It is not working in some cases due to QEMU_BACKEND macro.
> QEMU_BACKEND treats both backends as QEMU. As result xl performs device
> hotplug on add/remove device. Which is not expected in case "user
> space" backend.

Then perhaps this macro needs to be adjusted or called only
conditionally or something ?

> In this patch I propose solution similar to VUSB device. Where VUSB
> used for frontend and depends on backend (kernel or QEMU) either
> VUSB or QUSB used for backend device type e.g. use different backend
> device type for different backends.

I confess I don't quite follow all the VUSB stuff but I don't think it
is necessarily a good model.

> Introducing new backend device type for VKBD also allows to have
> both backends (QEMU and non QEMU) run in same domain. Not sure if it
> is useful scenario but with this proposal it is possible from
> technical point of view.

I don't understand why this is not possible simply by having a
different backend type.

> > I also don't understand why the "user space" kbd backend seems to be
> > "linux" in the enum.
> 
> I agree this is not so good name. But I don't know how to call
> backends which are not running
> inside QEMU in general.

I think this would be useable on freebsd ?  "linux" definitely seems
wrong.  I see it hasn't been in a release so it is not too late to
rename it, subject to discussion with Juergen as RM.

> > Where is the implementation of this user space
> > backend ?
> 
> We have extended kbd interface (kbdif.h) to support multi-touch events
> as well. And we have
> implemented own kbd backend https://github.com/xen-troops/displ_be/
> It is integrated with display backend as both use wayland API.

Great.

> > Is it be controlled entirely through xenstore ?
> 
> Yes it is controlled entirely through xenstore: lib xl creates
> frontend/backend entries with
> initial states and configuration.

And your display backend in "troops" (is that the name of your
project) checks whether the backend type is set to "linux", so that it
knows to ignore ones that say "qemu" ?

Maybe "linux" should be "troops"...

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Xen-devel] [PATCH v1 2/2] libxl: add removing XS backend path for PV devices on domain destroy
  2019-10-11 15:55       ` Ian Jackson
@ 2019-10-15 15:39         ` Roger Pau Monné
  2019-10-16 10:39           ` Oleksandr Grytsov
  0 siblings, 1 reply; 12+ messages in thread
From: Roger Pau Monné @ 2019-10-15 15:39 UTC (permalink / raw)
  To: Ian Jackson; +Cc: xen-devel, Oleksandr Grytsov, Oleksandr Grytsov, wl

On Fri, Oct 11, 2019 at 04:55:32PM +0100, Ian Jackson wrote:
> Roger Pau Monne writes ("Re: [PATCH v1 2/2] libxl: add removing XS backend path for PV devices on domain destroy"):
> > When this code was added (devd) those where the only backends handled
> > by libxl. VDISPL, VSND, VINPUT didn't exist at that point, so I think
> > the issue is that when support for those was added, it wasn't properly
> > wired into devd.
> > 
> > > I think:
> > > 
> > > >      switch(ddev->dev->backend_kind) {
> > > > +    case LIBXL__DEVICE_KIND_VDISPL:
> > > > +    case LIBXL__DEVICE_KIND_VSND:
> > > > +    case LIBXL__DEVICE_KIND_VINPUT:
> > > >      case LIBXL__DEVICE_KIND_VBD:
> > > >      case LIBXL__DEVICE_KIND_VIF:
> > > 
> > > If we do want this to handle *all* kinds of device, maybe it should
> > > have a fallback that aborts, or something ?  (I don't think it is
> > > easy to make it a compile error to forget to add an entry here but if
> > > we could do that it would probably be best.)
> > 
> > Maybe we could have some generic handling for everything != qdisk?
> 
> So make that the "default:" ?  That would be fine by me.

If possible yes, that would be my preference, and would prevent having
to add new device types to this switch unless special handling is
required.

> 
> > IIRC qdisk needs special handling so that devd can launch and destroy
> > a QEMU instance when required, but other backends that run in the
> > kernel don't need such handling and could maybe share the same generic
> > code path.
> 
> Right.
> 
> > > All of that assuming that the basic idea is right, which I would like
> > > Roger's opinion about.
> > 
> > Looking at the patch itself, you also seem to be doing some changes
> > related to num_vbds and num_vifs, but those are not mentioned in the
> > commit message, is that a stray change?
> 
> No, I don't think so.  Those variables just tell us when the thing is
> torn down but Oleksandr's code is able to use the devices slist itself
> for that.  Please do check to see if you agree.

Yes, that's fine. I don't think those changes are wrong, I just think
that at least they should be mentioned in the commit message. Ie:
"while there remove the num_vifs and num_vbds since they are not
needed and the same can be achieved by checking that the device list
is empty." or some such.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Xen-devel] [PATCH v1 2/2] libxl: add removing XS backend path for PV devices on domain destroy
  2019-10-15 15:39         ` Roger Pau Monné
@ 2019-10-16 10:39           ` Oleksandr Grytsov
  0 siblings, 0 replies; 12+ messages in thread
From: Oleksandr Grytsov @ 2019-10-16 10:39 UTC (permalink / raw)
  To: Roger Pau Monné; +Cc: Ian Jackson, Oleksandr Grytsov, wl, xen-devel

On Tue, Oct 15, 2019 at 6:39 PM Roger Pau Monné <roger.pau@citrix.com> wrote:
>
> On Fri, Oct 11, 2019 at 04:55:32PM +0100, Ian Jackson wrote:
> > Roger Pau Monne writes ("Re: [PATCH v1 2/2] libxl: add removing XS backend path for PV devices on domain destroy"):
> > > When this code was added (devd) those where the only backends handled
> > > by libxl. VDISPL, VSND, VINPUT didn't exist at that point, so I think
> > > the issue is that when support for those was added, it wasn't properly
> > > wired into devd.
> > >
> > > > I think:
> > > >
> > > > >      switch(ddev->dev->backend_kind) {
> > > > > +    case LIBXL__DEVICE_KIND_VDISPL:
> > > > > +    case LIBXL__DEVICE_KIND_VSND:
> > > > > +    case LIBXL__DEVICE_KIND_VINPUT:
> > > > >      case LIBXL__DEVICE_KIND_VBD:
> > > > >      case LIBXL__DEVICE_KIND_VIF:
> > > >
> > > > If we do want this to handle *all* kinds of device, maybe it should
> > > > have a fallback that aborts, or something ?  (I don't think it is
> > > > easy to make it a compile error to forget to add an entry here but if
> > > > we could do that it would probably be best.)
> > >
> > > Maybe we could have some generic handling for everything != qdisk?
> >
> > So make that the "default:" ?  That would be fine by me.
>
> If possible yes, that would be my preference, and would prevent having
> to add new device types to this switch unless special handling is
> required.
>
> >
> > > IIRC qdisk needs special handling so that devd can launch and destroy
> > > a QEMU instance when required, but other backends that run in the
> > > kernel don't need such handling and could maybe share the same generic
> > > code path.
> >
> > Right.
> >
> > > > All of that assuming that the basic idea is right, which I would like
> > > > Roger's opinion about.
> > >
> > > Looking at the patch itself, you also seem to be doing some changes
> > > related to num_vbds and num_vifs, but those are not mentioned in the
> > > commit message, is that a stray change?
> >
> > No, I don't think so.  Those variables just tell us when the thing is
> > torn down but Oleksandr's code is able to use the devices slist itself
> > for that.  Please do check to see if you agree.
>
> Yes, that's fine. I don't think those changes are wrong, I just think
> that at least they should be mentioned in the commit message. Ie:
> "while there remove the num_vifs and num_vbds since they are not
> needed and the same can be achieved by checking that the device list
> is empty." or some such.
>
> Thanks, Roger.

Ian, Roger,

Thanks for reviewing and comments. I will update the patch with your
comments above.

-- 
Best Regards,
Oleksandr Grytsov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Xen-devel] [PATCH v1 1/2] libxl: introduce new backend type VINPUT
  2019-10-11 17:03       ` Ian Jackson
@ 2019-10-16 13:26         ` Oleksandr Grytsov
  0 siblings, 0 replies; 12+ messages in thread
From: Oleksandr Grytsov @ 2019-10-16 13:26 UTC (permalink / raw)
  To: Ian Jackson; +Cc: xen-devel, Oleksandr Grytsov, wl

On Fri, Oct 11, 2019 at 8:04 PM Ian Jackson <ian.jackson@citrix.com> wrote:
>
> Oleksandr Grytsov writes ("Re: [PATCH v1 1/2] libxl: introduce new backend type VINPUT"):
> > On Fri, Oct 11, 2019 at 5:58 PM Ian Jackson <ian.jackson@citrix.com> wrote:
> > > I think it was a48e00f14a2d "libxl: add backend type and id to vkb"
> > > which introduced what you are calling "user space" backends.  It
> > > appears that the vkb backend type enum was introduced there
> > > specifically to distinguish between these two situations.  For reasons
> > >
> > > Am I wrong ?  If not, why is this not working or not suitable ?
> >
> > You are right. It is not working in some cases due to QEMU_BACKEND macro.
> > QEMU_BACKEND treats both backends as QEMU. As result xl performs device
> > hotplug on add/remove device. Which is not expected in case "user
> > space" backend.
>
> Then perhaps this macro needs to be adjusted or called only
> conditionally or something ?

I had an idea to move this macro to libxl__device_type and let device
itself decides
if it is qemu backend. But in case of VKBD it will read XS to determine backend
type. I guess it is ok.

>
> > In this patch I propose solution similar to VUSB device. Where VUSB
> > used for frontend and depends on backend (kernel or QEMU) either
> > VUSB or QUSB used for backend device type e.g. use different backend
> > device type for different backends.
>
> I confess I don't quite follow all the VUSB stuff but I don't think it
> is necessarily a good model.

If you don't mind to move QEMU_BACKEND macrto to libxl__device_type then
no need to add new device type at all.

>
> > Introducing new backend device type for VKBD also allows to have
> > both backends (QEMU and non QEMU) run in same domain. Not sure if it
> > is useful scenario but with this proposal it is possible from
> > technical point of view.
>
> I don't understand why this is not possible simply by having a
> different backend type.
>
> > > I also don't understand why the "user space" kbd backend seems to be
> > > "linux" in the enum.
> >
> > I agree this is not so good name. But I don't know how to call
> > backends which are not running
> > inside QEMU in general.
>
> I think this would be useable on freebsd ?  "linux" definitely seems
> wrong.  I see it hasn't been in a release so it is not too late to
> rename it, subject to discussion with Juergen as RM.
>
> > > Where is the implementation of this user space
> > > backend ?
> >
> > We have extended kbd interface (kbdif.h) to support multi-touch events
> > as well. And we have
> > implemented own kbd backend https://github.com/xen-troops/displ_be/
> > It is integrated with display backend as both use wayland API.
>
> Great.
>
> > > Is it be controlled entirely through xenstore ?
> >
> > Yes it is controlled entirely through xenstore: lib xl creates
> > frontend/backend entries with
> > initial states and configuration.
>
> And your display backend in "troops" (is that the name of your
> project) checks whether the backend type is set to "linux", so that it
> knows to ignore ones that say "qemu" ?
>
> Maybe "linux" should be "troops"...
>

It doesn't look as generic solution. If some user implements own backend
it should add new entry into backend type enum.
What about to have just string value instead of enum? In case QEMU
we don't have such entry at all but in case custom backend the user
can't put any string value here to be recognized by his backend.

> Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, back to index

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-08 14:10 [Xen-devel] [PATCH v1 0/2] Remove backend xen store entry on domain destroy Oleksandr Grytsov
2019-10-08 14:10 ` [Xen-devel] [PATCH v1 1/2] libxl: introduce new backend type VINPUT Oleksandr Grytsov
2019-10-11 14:58   ` Ian Jackson
2019-10-11 15:57     ` Oleksandr Grytsov
2019-10-11 17:03       ` Ian Jackson
2019-10-16 13:26         ` Oleksandr Grytsov
2019-10-08 14:10 ` [Xen-devel] [PATCH v1 2/2] libxl: add removing XS backend path for PV devices on domain destroy Oleksandr Grytsov
2019-10-11 15:07   ` Ian Jackson
2019-10-11 15:32     ` Roger Pau Monné
2019-10-11 15:55       ` Ian Jackson
2019-10-15 15:39         ` Roger Pau Monné
2019-10-16 10:39           ` Oleksandr Grytsov

Xen-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/xen-devel/0 xen-devel/git/0.git
	git clone --mirror https://lore.kernel.org/xen-devel/1 xen-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 xen-devel xen-devel/ https://lore.kernel.org/xen-devel \
		xen-devel@lists.xenproject.org xen-devel@lists.xen.org xen-devel@archiver.kernel.org
	public-inbox-index xen-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.xenproject.lists.xen-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox