[PATCH RFC 0/8] libxl, xl, public/io: PV backends feature control

[PATCH RFC 0/8] libxl, xl, public/io: PV backends feature control

[Joao Martins]

Hey folks,

Presented herewith is an attempt to implement PV backends feature control
as discussed in the list (https://lists.xen.org/archives/html/xen-devel/2017-09/msg00766.html)

Given that this a simple proposal hence I thought to include all changes
involved in the same patchset such that everyone see all the changes and has a
better estimate (but restricted to xen-devel just for the RFC purposes).

The motivation here is to allow system administrators more fine grained
control of the device features being used by guest.

The only change I made compared to the proposed discussed above was to use
"require" instead of "request" as the prefix because there is a feature which
has "request" in it. But if "request" is still preferred as a prefix I can change
it up.

The scheme proposed is quite simple:

* The directory "require" is created (inside the backend path) and within that
directory the features/capabilities names and values are written.

* Toolstack constructs a key value store of features, and user specifies those
through special entry names prefixed also as "require". Toolstack is stateless thus sys
admin has full control over what to pass to the backend. In other words it
doesn't look at particular feature names/values.

* The backend will then use that for seeding its maximum feature set to the
frontend.

An example would be a domain config to look like this:

vif = ["bridge=br0,require-multi-queue-max-queues=2"]
disk = [ "phy:/path/to/disk,hda,w,require-feature-persistent=0" ]

And if backend supports it, it would create a vif with a maximum of 2 queues,
and a vbd with persistent grants disabled.

I only implemented for blkback and netback but there is nothing really specific
to how it's done and could possibly be implemented in other PV interfaces. But
there wasn't a protocol agnostic file to put all this, so I went ahead and did
for the two individual io types (block and netif) I am most interested in.

Any comments appreciated :)

Thanks!
Joao

For Linux the diffstat/changeset is: (the last two patches)

Joao Martins (2):
  xen-blkback: frontend feature control
  xen-netback: frontend feature control

 drivers/block/xen-blkback/blkback.c |   2 +-
 drivers/block/xen-blkback/common.h  |   1 +
 drivers/block/xen-blkback/xenbus.c  |  66 ++++++++++++++++---
 drivers/net/xen-netback/xenbus.c    | 122 +++++++++++++++++++++++++++++-------
 4 files changed, 159 insertions(+), 32 deletions(-)

And for Xen the diffstat/changeset is:

Joao Martins (6):
  public/io/blkif: add directory for backend parameters
  public/io/netif: add directory for backend parameters
  libxl: add backend_features to libxl_device_disk
  libxl: add backend_features to libxl_device_nic
  libxlu: parse disk backend features parameters
  xl: parse vif backend features parameters

 tools/libxl/libxl.h           | 16 +++++++++++++++
 tools/libxl/libxl_9pfs.c      |  2 +-
 tools/libxl/libxl_console.c   |  7 ++++---
 tools/libxl/libxl_device.c    | 47 +++++++++++++++++++++++++++++++++++--------
 tools/libxl/libxl_disk.c      | 17 ++++++++++++++--
 tools/libxl/libxl_internal.h  |  6 ++++--
 tools/libxl/libxl_nic.c       | 13 +++++++++++-
 tools/libxl/libxl_pci.c       |  2 +-
 tools/libxl/libxl_types.idl   |  2 ++
 tools/libxl/libxl_usb.c       |  2 +-
 tools/libxl/libxl_vdispl.c    |  3 ++-
 tools/libxl/libxl_vtpm.c      |  2 +-
 tools/libxl/libxlu_disk_l.l   | 42 ++++++++++++++++++++++++++++++++++++++
 tools/xl/xl_parse.c           | 37 ++++++++++++++++++++++++++++++++++
 tools/xl/xl_parse.h           |  2 ++
 xen/include/public/io/blkif.h | 14 +++++++++++++
 xen/include/public/io/netif.h | 16 +++++++++++++++
 17 files changed, 209 insertions(+), 21 deletions(-)

-- 
2.11.0


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

[PATCH RFC 1/8] public/io/blkif: add directory for backend parameters

[Joao Martins]

The proposed directory provides a mechanism for tools to control the
maximum feature set of the device being provisioned by backends.
Examples include max ring page order, persistent grants, number of
queues etc.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 xen/include/public/io/blkif.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h
index 15a71e3fea..4c0a93a2bf 100644
--- a/xen/include/public/io/blkif.h
+++ b/xen/include/public/io/blkif.h
@@ -133,6 +133,20 @@
  *      This option doesn't require a backend to use O_DIRECT, so it
  *      should not be used to try to control the caching behaviour.
  *
+ * require
+ *
+ *      The directory "require" maybe be created by tools domain to
+ *      override the maximum feature set that backend provides to the
+ *      frontend. The children entries within this directory are
+ *      features names and its correspondent value e.g.:
+ *
+ *      /local/domain/X/backend/vbd/<domid>/<devno>/require
+ *      /local/domain/X/backend/vbd/<domid>/<devno>/require/multi-queue-max-queues = "2"
+ *      /local/domain/X/backend/vbd/<domid>/<devno>/require/feature-persistent = "0"
+ *
+ *      In the example above, block backend will negotiate up to a maximum of
+ *      two queues with frontend plus disabling persistent grants.
+ *
  *--------------------------------- Features ---------------------------------
  *
  * feature-barrier
-- 
2.11.0


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

[PATCH RFC 2/8] public/io/netif: add directory for backend parameters

[Joao Martins]

The proposed directory provides a mechanism for tools to control the
maximum feature set of the device being provisioned by backend.
The parameters/features include offloading features, number of
queues etc.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 xen/include/public/io/netif.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/xen/include/public/io/netif.h b/xen/include/public/io/netif.h
index 2454448baa..a412e4771d 100644
--- a/xen/include/public/io/netif.h
+++ b/xen/include/public/io/netif.h
@@ -161,6 +161,22 @@
  */
 
 /*
+ * The directory "require" maybe be created in backend path by tools
+ * domain to override the maximum feature set that backend provides to the
+ * frontend. The children entries within this directory are features names
+ * and the correspondent values that should be used backend as defaults e.g.:
+ *
+ * /local/domain/X/backend/<domid>/<handle>/require
+ * /local/domain/X/backend/<domid>/<handle>/require/multi-queue-max-queues = "2"
+ * /local/domain/X/backend/<domid>/<handle>/require/feature-no-csum-offload = "1"
+ *
+ * In the example above, network backend will negotiate up to a maximum of
+ * two queues with frontend plus disabling IPv4 checksum offloading.
+ *
+ * This directory and its children entries shall only be visible to the backend.
+ */
+
+/*
  * Control ring
  * ============
  *
-- 
2.11.0


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

[PATCH RFC 3/8] libxl: add backend_features to libxl_device_disk

[Joao Martins]

The function libxl__device_generic_add will have an additional
argument whereby it adds a second set of entries visible to the
backend only. These entries will then be used for devices
thus overriding backend maximum feature set with this user-defined ones.

libxl_device_disk.backend_features are a key value store storing:
 <feature-name> = <feature-value>

xl|libxl are stateless with respect to feature names therefore is up to the
admin to carefully select those. If backend isn't supported therefore the
features won't be overwritten.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 tools/libxl/libxl.h          |  8 ++++++++
 tools/libxl/libxl_console.c  |  5 +++--
 tools/libxl/libxl_device.c   | 37 +++++++++++++++++++++++++++++++++----
 tools/libxl/libxl_disk.c     | 17 +++++++++++++++--
 tools/libxl/libxl_internal.h |  4 +++-
 tools/libxl/libxl_pci.c      |  2 +-
 tools/libxl/libxl_types.idl  |  1 +
 tools/libxl/libxl_usb.c      |  2 +-
 8 files changed, 65 insertions(+), 11 deletions(-)

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 5e9aed739d..82990089ef 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -1101,6 +1101,14 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, const libxl_mac *src);
  */
 #define LIBXL_HAVE_SET_PARAMETERS 1
 
+/*
+ * LIBXL_HAVE_DISK_BACKEND_FEATURES
+ *
+ * libxl_device_disk contains backend_features which can be used to control
+ * what features are exposed to guest vbds.
+ */
+#define LIBXL_HAVE_DISK_BACKEND_FEATURES 1
+
 typedef char **libxl_string_list;
 void libxl_string_list_dispose(libxl_string_list *sl);
 int libxl_string_list_length(const libxl_string_list *sl);
diff --git a/tools/libxl/libxl_console.c b/tools/libxl/libxl_console.c
index c05dc28b99..f40def1276 100644
--- a/tools/libxl/libxl_console.c
+++ b/tools/libxl/libxl_console.c
@@ -339,7 +339,7 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
     libxl__device_generic_add(gc, XBT_NULL, device,
                               libxl__xs_kvs_of_flexarray(gc, back),
                               libxl__xs_kvs_of_flexarray(gc, front),
-                              libxl__xs_kvs_of_flexarray(gc, ro_front));
+                              libxl__xs_kvs_of_flexarray(gc, ro_front), NULL);
     rc = 0;
 out:
     return rc;
@@ -385,7 +385,8 @@ int libxl__device_vuart_add(libxl__gc *gc, uint32_t domid,
     rc = libxl__device_generic_add(gc, XBT_NULL, &device,
                                    libxl__xs_kvs_of_flexarray(gc, back),
                                    NULL,
-                                   libxl__xs_kvs_of_flexarray(gc, ro_front));
+                                   libxl__xs_kvs_of_flexarray(gc, ro_front),
+                                   NULL);
     return rc;
 }
 
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 5438577c3c..05178fb480 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -43,6 +43,15 @@ char *libxl__device_backend_path(libxl__gc *gc, libxl__device *device)
                      device->domid, device->devid);
 }
 
+char *libxl__device_require_path(libxl__gc *gc, libxl__device *device)
+{
+    char *dom_path = libxl__xs_get_dompath(gc, device->backend_domid);
+
+    return GCSPRINTF("%s/backend/%s/%u/%d/require", dom_path,
+                     libxl__device_kind_to_string(device->backend_kind),
+                     device->domid, device->devid);
+}
+
 char *libxl__device_libxl_path(libxl__gc *gc, libxl__device *device)
 {
     char *libxl_dom_path = libxl__xs_libxl_path(gc, device->domid);
@@ -114,13 +123,16 @@ out:
 }
 
 int libxl__device_generic_add(libxl__gc *gc, xs_transaction_t t,
-        libxl__device *device, char **bents, char **fents, char **ro_fents)
+        libxl__device *device, char **bents, char **fents, char **ro_fents,
+        char **brents)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
-    char *frontend_path = NULL, *backend_path = NULL, *libxl_path;
+    char *frontend_path = NULL, *backend_path = NULL, *require_path = NULL,
+         *libxl_path;
     struct xs_permissions frontend_perms[2];
     struct xs_permissions ro_frontend_perms[2];
     struct xs_permissions backend_perms[2];
+    struct xs_permissions require_perms[1];
     int create_transaction = t == XBT_NULL;
     int libxl_only = device->backend_kind == LIBXL__DEVICE_KIND_NONE;
     int rc;
@@ -131,6 +143,7 @@ int libxl__device_generic_add(libxl__gc *gc, xs_transaction_t t,
     } else {
         frontend_path = libxl__device_frontend_path(gc, device);
         backend_path = libxl__device_backend_path(gc, device);
+        require_path = libxl__device_require_path(gc, device);
     }
     libxl_path = libxl__device_libxl_path(gc, device);
 
@@ -144,6 +157,9 @@ int libxl__device_generic_add(libxl__gc *gc, xs_transaction_t t,
     ro_frontend_perms[1].id = backend_perms[1].id = device->domid;
     ro_frontend_perms[1].perms = backend_perms[1].perms = XS_PERM_READ;
 
+    require_perms[0].id = device->backend_domid;
+    require_perms[0].perms = XS_PERM_NONE;
+
 retry_transaction:
     if (create_transaction)
         t = xs_transaction_start(ctx->xsh);
@@ -200,6 +216,12 @@ retry_transaction:
                      frontend_path, strlen(frontend_path));
             libxl__xs_writev(gc, t, backend_path, bents);
         }
+        if (brents) {
+            xs_mkdir(ctx->xsh, t, require_path);
+            xs_set_permissions(ctx->xsh, t, require_path, require_perms,
+                               ARRAY_SIZE(require_perms));
+            libxl__xs_writev(gc, t, require_path, brents);
+        }
 
         /*
          * We make a copy of everything for the backend in the libxl
@@ -226,6 +248,11 @@ retry_transaction:
          */
         rc = libxl__xs_writev(gc, t, libxl_path, bents);
         if (rc) goto out;
+
+        if (brents) {
+            rc = libxl__xs_writev(gc, t, libxl_path, brents);
+            if (rc) goto out;
+        }
     }
 
     if (!create_transaction)
@@ -1920,7 +1947,8 @@ void libxl__device_add_async(libxl__egc *egc, uint32_t domid,
         libxl__device_generic_add(gc, t, device,
                                   libxl__xs_kvs_of_flexarray(gc, back),
                                   libxl__xs_kvs_of_flexarray(gc, front),
-                                  libxl__xs_kvs_of_flexarray(gc, ro_front));
+                                  libxl__xs_kvs_of_flexarray(gc, ro_front),
+                                  NULL);
 
         rc = libxl__xs_transaction_commit(gc, &t);
         if (!rc) break;
@@ -1984,7 +2012,8 @@ int libxl__device_add(libxl__gc *gc, uint32_t domid,
     rc = libxl__device_generic_add(gc, XBT_NULL, device,
                                    libxl__xs_kvs_of_flexarray(gc, back),
                                    libxl__xs_kvs_of_flexarray(gc, front),
-                                   libxl__xs_kvs_of_flexarray(gc, ro_front));
+                                   libxl__xs_kvs_of_flexarray(gc, ro_front),
+                                   NULL);
     if (rc) goto out;
 
     rc = 0;
diff --git a/tools/libxl/libxl_disk.c b/tools/libxl/libxl_disk.c
index 895bf4f89a..8caf763c88 100644
--- a/tools/libxl/libxl_disk.c
+++ b/tools/libxl/libxl_disk.c
@@ -239,14 +239,16 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
     STATE_AO_GC(aodev->ao);
     flexarray_t *front = NULL;
     flexarray_t *back = NULL;
+    flexarray_t *require = NULL;
     char *dev = NULL, *script;
     libxl__device *device;
-    int rc;
+    int rc, i;
     libxl_ctx *ctx = gc->owner;
     xs_transaction_t t = XBT_NULL;
     libxl_domain_config d_config;
     libxl_device_disk disk_saved;
     libxl__domain_userdata_lock *lock = NULL;
+    libxl_key_value_list back_features = disk->backend_features;
 
     libxl_domain_config_init(&d_config);
     libxl_device_disk_init(&disk_saved);
@@ -300,6 +302,7 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
 
         front = flexarray_make(gc, 16, 1);
         back = flexarray_make(gc, 16, 1);
+        require = flexarray_make(gc, 16, 1);
 
         GCNEW(device);
         rc = libxl__device_from_disk(gc, domid, disk, device);
@@ -433,6 +436,15 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
             }
         }
 
+        if (back_features) {
+            for (i = 0; back_features[i] != NULL; i += 2) {
+                flexarray_append(require, libxl__strdup(gc, back_features[i]));
+                if (back_features[i + 1])
+                    flexarray_append(require,
+                                     libxl__strdup(gc, back_features[i + 1]));
+            }
+        }
+
         if (!get_vdev && aodev->update_json) {
             rc = libxl__set_domain_configuration(gc, domid, &d_config);
             if (rc) goto out;
@@ -441,7 +453,8 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
         libxl__device_generic_add(gc, t, device,
                                   libxl__xs_kvs_of_flexarray(gc, back),
                                   libxl__xs_kvs_of_flexarray(gc, front),
-                                  NULL);
+                                  NULL,
+                                  libxl__xs_kvs_of_flexarray(gc, require));
 
         rc = libxl__xs_transaction_commit(gc, &t);
         if (!rc) break;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 45e6df6c82..19c02e27a0 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1216,8 +1216,10 @@ _hidden int libxl__device_vuart_add(libxl__gc *gc, uint32_t domid,
 _hidden int libxl__device_exists(libxl__gc *gc, xs_transaction_t t,
                                  libxl__device *device);
 _hidden int libxl__device_generic_add(libxl__gc *gc, xs_transaction_t t,
-        libxl__device *device, char **bents, char **fents, char **ro_fents);
+                                      libxl__device *device, char **bents,
+                                      char **fents, char **ro_fents, char **brents);
 _hidden char *libxl__device_backend_path(libxl__gc *gc, libxl__device *device);
+_hidden char *libxl__device_require_path(libxl__gc *gc, libxl__device *device);
 _hidden char *libxl__device_libxl_path(libxl__gc *gc, libxl__device *device);
 _hidden int libxl__parse_backend_path(libxl__gc *gc, const char *path,
                                       libxl__device *dev);
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 88a55ce8bd..1d595513f5 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -106,7 +106,7 @@ int libxl__create_pci_backend(libxl__gc *gc, uint32_t domid,
     return libxl__device_generic_add(gc, XBT_NULL, &device,
                                      libxl__xs_kvs_of_flexarray(gc, back),
                                      libxl__xs_kvs_of_flexarray(gc, front),
-                                     NULL);
+                                     NULL, NULL);
 }
 
 static int libxl__device_pci_add_xenstore(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev, int starting)
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index a239324341..949a797402 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -627,6 +627,7 @@ libxl_device_vkb = Struct("device_vkb", [
 libxl_device_disk = Struct("device_disk", [
     ("backend_domid", libxl_domid),
     ("backend_domname", string),
+    ("backend_features", libxl_key_value_list),
     ("pdev_path", string),
     ("vdev", string),
     ("backend", libxl_disk_backend),
diff --git a/tools/libxl/libxl_usb.c b/tools/libxl/libxl_usb.c
index cb0e792724..97e54272f1 100644
--- a/tools/libxl/libxl_usb.c
+++ b/tools/libxl/libxl_usb.c
@@ -283,7 +283,7 @@ static int libxl__device_usbctrl_add_xenstore(libxl__gc *gc, uint32_t domid,
         libxl__device_generic_add(gc, t, device,
                                   libxl__xs_kvs_of_flexarray(gc, back),
                                   libxl__xs_kvs_of_flexarray(gc, front),
-                                  NULL);
+                                  NULL, NULL);
 
         rc = libxl__xs_transaction_commit(gc, &t);
         if (!rc) break;
-- 
2.11.0


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

[PATCH RFC 4/8] libxl: add backend_features to libxl_device_nic

[Joao Martins]

Adds "backend_features" to the libxl_device_nic structure to
represent a set of features to be set on the device by the admin.
These backend_features is a key value store representing
an array of <feature-name> = <feature-value>, which would then be
translated into (backend-only permissions) xenstore entries in
the form of:

/local/domain/<backend-id>/backend/vif/<frontend-id>/<handle>/require
/local/domain/[...]/require/<feature-name> = <feature-value>

Entries get stored under the require directory within the backend
path.

Adjust libxl__device_add and libxl__device_add_async to pass the
third argument as the backend-only entries to be written to backend_path.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 tools/libxl/libxl.h          |  8 ++++++++
 tools/libxl/libxl_9pfs.c     |  2 +-
 tools/libxl/libxl_console.c  |  2 +-
 tools/libxl/libxl_device.c   | 14 ++++++++------
 tools/libxl/libxl_internal.h |  2 +-
 tools/libxl/libxl_nic.c      | 13 ++++++++++++-
 tools/libxl/libxl_types.idl  |  1 +
 tools/libxl/libxl_vdispl.c   |  3 ++-
 tools/libxl/libxl_vtpm.c     |  2 +-
 9 files changed, 35 insertions(+), 12 deletions(-)

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 82990089ef..5b4fbebf7b 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -1109,6 +1109,14 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, const libxl_mac *src);
  */
 #define LIBXL_HAVE_DISK_BACKEND_FEATURES 1
 
+/*
+ * LIBXL_HAVE_VIF_BACKEND_FEATURES
+ *
+ * libxl_device_nic contains backend_features which can be used to control
+ * what features are exposed to guest vifs.
+ */
+#define LIBXL_HAVE_VIF_BACKEND_FEATURES 1
+
 typedef char **libxl_string_list;
 void libxl_string_list_dispose(libxl_string_list *sl);
 int libxl_string_list_length(const libxl_string_list *sl);
diff --git a/tools/libxl/libxl_9pfs.c b/tools/libxl/libxl_9pfs.c
index 9db887b5d8..3b80b358f4 100644
--- a/tools/libxl/libxl_9pfs.c
+++ b/tools/libxl/libxl_9pfs.c
@@ -42,7 +42,7 @@ static LIBXL_DEFINE_UPDATE_DEVID(p9, "9pfs")
 static int libxl__set_xenstore_p9(libxl__gc *gc, uint32_t domid,
                                   libxl_device_p9 *p9,
                                   flexarray_t *back, flexarray_t *front,
-                                  flexarray_t *ro_front)
+                                  flexarray_t *ro_front, flexarray_t *require)
 {
     flexarray_append_pair(back, "path", p9->path);
     flexarray_append_pair(back, "security_model", p9->security_model);
diff --git a/tools/libxl/libxl_console.c b/tools/libxl/libxl_console.c
index f40def1276..1c5a298750 100644
--- a/tools/libxl/libxl_console.c
+++ b/tools/libxl/libxl_console.c
@@ -730,7 +730,7 @@ static LIBXL_DEFINE_UPDATE_DEVID(vfb, "vfb")
 static int libxl__set_xenstore_vfb(libxl__gc *gc, uint32_t domid,
                                    libxl_device_vfb *vfb,
                                   flexarray_t *back, flexarray_t *front,
-                                  flexarray_t *ro_front)
+                                  flexarray_t *ro_front, flexarray_t *require)
 {
     flexarray_append_pair(back, "vnc",
                           libxl_defbool_val(vfb->vnc.enable) ? "1" : "0");
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 05178fb480..87983e2ef9 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -1860,7 +1860,7 @@ void libxl__device_add_async(libxl__egc *egc, uint32_t domid,
                              libxl__ao_device *aodev)
 {
     STATE_AO_GC(aodev->ao);
-    flexarray_t *back;
+    flexarray_t *back, *require;
     flexarray_t *front, *ro_front;
     libxl__device *device;
     xs_transaction_t t = XBT_NULL;
@@ -1912,6 +1912,7 @@ void libxl__device_add_async(libxl__egc *egc, uint32_t domid,
     back = flexarray_make(gc, 16, 1);
     front = flexarray_make(gc, 16, 1);
     ro_front = flexarray_make(gc, 16, 1);
+    require = flexarray_make(gc, 16, 1);
 
     flexarray_append_pair(back, "frontend-id", GCSPRINTF("%d", domid));
     flexarray_append_pair(back, "online", "1");
@@ -1924,7 +1925,7 @@ void libxl__device_add_async(libxl__egc *egc, uint32_t domid,
                           GCSPRINTF("%d", XenbusStateInitialising));
 
     if (dt->set_xenstore_config)
-        dt->set_xenstore_config(gc, domid, type, back, front, ro_front);
+        dt->set_xenstore_config(gc, domid, type, back, front, ro_front, require);
 
     for (;;) {
         rc = libxl__xs_transaction_start(gc, &t);
@@ -1948,7 +1949,7 @@ void libxl__device_add_async(libxl__egc *egc, uint32_t domid,
                                   libxl__xs_kvs_of_flexarray(gc, back),
                                   libxl__xs_kvs_of_flexarray(gc, front),
                                   libxl__xs_kvs_of_flexarray(gc, ro_front),
-                                  NULL);
+                                  libxl__xs_kvs_of_flexarray(gc, require));
 
         rc = libxl__xs_transaction_commit(gc, &t);
         if (!rc) break;
@@ -1974,7 +1975,7 @@ out:
 int libxl__device_add(libxl__gc *gc, uint32_t domid,
                       const struct libxl_device_type *dt, void *type)
 {
-    flexarray_t *back;
+    flexarray_t *back, *require;
     flexarray_t *front, *ro_front;
     libxl__device *device;
     int rc;
@@ -1996,6 +1997,7 @@ int libxl__device_add(libxl__gc *gc, uint32_t domid,
     back = flexarray_make(gc, 16, 1);
     front = flexarray_make(gc, 16, 1);
     ro_front = flexarray_make(gc, 16, 1);
+    require = flexarray_make(gc, 16, 1);
 
     flexarray_append_pair(back, "frontend-id", GCSPRINTF("%d", domid));
     flexarray_append_pair(back, "online", "1");
@@ -2007,13 +2009,13 @@ int libxl__device_add(libxl__gc *gc, uint32_t domid,
                           GCSPRINTF("%d", XenbusStateInitialising));
 
     if (dt->set_xenstore_config)
-        dt->set_xenstore_config(gc, domid, type, back, front, ro_front);
+        dt->set_xenstore_config(gc, domid, type, back, front, ro_front, require);
 
     rc = libxl__device_generic_add(gc, XBT_NULL, device,
                                    libxl__xs_kvs_of_flexarray(gc, back),
                                    libxl__xs_kvs_of_flexarray(gc, front),
                                    libxl__xs_kvs_of_flexarray(gc, ro_front),
-                                   NULL);
+                                   libxl__xs_kvs_of_flexarray(gc, require));
     if (rc) goto out;
 
     rc = 0;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 19c02e27a0..0881147d3c 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3507,7 +3507,7 @@ typedef int (*device_from_xenstore_fn_t)(libxl__gc *, const char *,
                                          libxl_devid, void *);
 typedef int (*device_set_xenstore_config_fn_t)(libxl__gc *, uint32_t, void *,
                                                flexarray_t *, flexarray_t *,
-                                               flexarray_t *);
+                                               flexarray_t *, flexarray_t *);
 
 struct libxl_device_type {
     char *type;
diff --git a/tools/libxl/libxl_nic.c b/tools/libxl/libxl_nic.c
index 1e6ba6c78d..d67155d58d 100644
--- a/tools/libxl/libxl_nic.c
+++ b/tools/libxl/libxl_nic.c
@@ -143,7 +143,7 @@ static LIBXL_DEFINE_UPDATE_DEVID(nic, "vif")
 static int libxl__set_xenstore_nic(libxl__gc *gc, uint32_t domid,
                                    libxl_device_nic *nic,
                                    flexarray_t *back, flexarray_t *front,
-                                   flexarray_t *ro_front)
+                                   flexarray_t *ro_front, flexarray_t *require)
 {
     flexarray_grow(back, 2);
 
@@ -253,6 +253,17 @@ static int libxl__set_xenstore_nic(libxl__gc *gc, uint32_t domid,
     flexarray_append(front, GCSPRINTF(
                                     LIBXL_MAC_FMT, LIBXL_MAC_BYTES(nic->mac)));
 
+    if (nic->backend_features) {
+        libxl_key_value_list features = nic->backend_features;
+        int i;
+
+        for (i = 0; features[i] != NULL; i += 2) {
+            flexarray_append(require, libxl__strdup(gc, features[i]));
+            if (features[i+1])
+                flexarray_append(require, libxl__strdup(gc, features[i+1]));
+        }
+    }
+
     return 0;
 }
 
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 949a797402..fba6127bbc 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -652,6 +652,7 @@ libxl_device_disk = Struct("device_disk", [
 libxl_device_nic = Struct("device_nic", [
     ("backend_domid", libxl_domid),
     ("backend_domname", string),
+    ("backend_features", libxl_key_value_list),
     ("devid", libxl_devid),
     ("mtu", integer),
     ("model", string),
diff --git a/tools/libxl/libxl_vdispl.c b/tools/libxl/libxl_vdispl.c
index e61ef2dfcd..3c266f4c73 100644
--- a/tools/libxl/libxl_vdispl.c
+++ b/tools/libxl/libxl_vdispl.c
@@ -76,7 +76,8 @@ static void libxl__device_vdispl_add(libxl__egc *egc, uint32_t domid,
 static int libxl__set_xenstore_vdispl(libxl__gc *gc, uint32_t domid,
                                       libxl_device_vdispl *vdispl,
                                       flexarray_t *back, flexarray_t *front,
-                                      flexarray_t *ro_front)
+                                      flexarray_t *ro_front,
+                                      flexarray_t *require)
 {
     int i;
 
diff --git a/tools/libxl/libxl_vtpm.c b/tools/libxl/libxl_vtpm.c
index 3f0c56349d..1fbb95c8aa 100644
--- a/tools/libxl/libxl_vtpm.c
+++ b/tools/libxl/libxl_vtpm.c
@@ -54,7 +54,7 @@ static LIBXL_DEFINE_UPDATE_DEVID(vtpm, "vtpm")
 static int libxl__set_xenstore_vtpm(libxl__gc *gc, uint32_t domid,
                                     libxl_device_vtpm *vtpm,
                                     flexarray_t *back, flexarray_t *front,
-                                    flexarray_t *ro_front)
+                                    flexarray_t *ro_front, flexarray_t *require)
 {
     flexarray_append_pair(back, "handle", GCSPRINTF("%d", vtpm->devid));
     flexarray_append_pair(back, "uuid",
-- 
2.11.0


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

[PATCH RFC 5/8] libxlu: parse disk backend features parameters

[Joao Martins]

Any option name preceded by "require-" means a backend feature
to be set. This is stored in key value structure which libxl will parse
and tell blkback to override the specified features.

An example would be a config containing:

...
vcpus = 8
disk = [ "phy:/path/to/disk,hda,w,require-multi-queue-max-queues=1" ]
...

Which would set the number of queues to 2 as opposed to e.g. the global
blkback defined xen_blkback.max_queues parameter.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 tools/libxl/libxlu_disk_l.l | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/tools/libxl/libxlu_disk_l.l b/tools/libxl/libxlu_disk_l.l
index 97039a2800..4530c6c4fc 100644
--- a/tools/libxl/libxlu_disk_l.l
+++ b/tools/libxl/libxlu_disk_l.l
@@ -62,6 +62,9 @@ void xlu__disk_yyset_column(int  column_no, yyscan_t yyscanner);
 /* For actions whose patterns contain '=', finds the start of the value */
 #define FROMEQUALS (strchr(yytext,'=')+1)
 
+/* For actions whose patterns contain '-', finds the start of the value */
+#define FROMMINUS (strchr(yytext,'-')+1)
+
 /* Chops the delimiter off, modifying yytext and yyleng. */
 #define STRIP(delim) do{                                                \
 	if (yyleng>0 && yytext[yyleng-1]==(delim))                      \
@@ -114,6 +117,37 @@ static void setbackendtype(DiskParseContext *dpc, const char *str) {
     else xlu__disk_err(dpc,str,"unknown value for backendtype");
 }
 
+static int addbackendfeature(DiskParseContext *dpc, const char *key)
+{
+    libxl_key_value_list *sl = &dpc->disk->backend_features;
+    size_t count = libxl_key_value_list_length(sl);
+    libxl_key_value_list array = *sl;
+    char *eql = strchr(key,'=');
+    char *val = eql + 1;
+    int i;
+
+    array = calloc((count+1) * 2 + 1, sizeof(char*));
+    if (!array)
+        return -ENOMEM;
+
+    for (i = 0; i < count * 2; i++) {
+        if ((*sl)[i])
+            array[i] = strdup((*sl)[i]);
+    }
+    array[i] = NULL;
+    libxl_key_value_list_dispose(sl);
+
+    *eql = 0;
+    count *= 2;
+    array[count++] = strdup(key);
+    array[count++] = strdup(val);
+    array[count] = NULL;
+    *eql = '=';
+
+    *sl = array;
+    return 0;
+}
+
 /* Sets ->colo-port from the string.  COLO need this. */
 static void setcoloport(DiskParseContext *dpc, const char *str) {
     int port = atoi(str);
@@ -187,6 +221,14 @@ script=[^,]*,?	{ STRIP(','); SAVESTRING("script", script, FROMEQUALS); }
 direct-io-safe,? { DPC->disk->direct_io_safe = 1; }
 discard,?	{ libxl_defbool_set(&DPC->disk->discard_enable, true); }
 no-discard,?	{ libxl_defbool_set(&DPC->disk->discard_enable, false); }
+require-[a-z][-a-z0-9]*=[^,],? {
+   STRIP(',');
+   if (addbackendfeature(DPC, FROMMINUS)) {
+       xlu__disk_err(DPC,yytext,"unable to parse feature");
+       return 0;
+   }
+}
+
  /* Note that the COLO configuration settings should be considered unstable.
   * They may change incompatibly in future versions of Xen. */
 colo,?		{ libxl_defbool_set(&DPC->disk->colo_enable, true); }
-- 
2.11.0


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

[PATCH RFC 6/8] xl: parse vif backend features parameters

[Joao Martins]

Any option name preceded by "require-" means a backend feature to be {un,}set.
This is stored in key value structure which libxl will parse and inform netback
to override the specified features.

An example would be a config containing:

...
vcpus = 8
vif = ["bridge=br0,require-multi-queue-max-queues=2"]
...

Which would set the number of queues to 2 as opposed to e.g. the global
netback defined xen_netback.max_queues parameter.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 tools/xl/xl_parse.c | 37 +++++++++++++++++++++++++++++++++++++
 tools/xl/xl_parse.h |  2 ++
 2 files changed, 39 insertions(+)

diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 9a692d5ae6..007df694d8 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -401,6 +401,29 @@ void replace_string(char **str, const char *val)
     *str = xstrdup(val);
 }
 
+static void add_to_kvlist(libxl_key_value_list *sl, char *key, char *val)
+{
+    size_t count = libxl_key_value_list_length(sl);
+    libxl_key_value_list array = *sl;
+    int i;
+
+    array = xcalloc((count+1) * 2 + 1, sizeof(char*));
+
+    for (i = 0; i < count * 2; i++) {
+        if ((*sl)[i])
+            array[i] = xstrdup((*sl)[i]);
+    }
+    array[i] = NULL;
+    libxl_key_value_list_dispose(sl);
+
+    count *= 2;
+    array[count++] = xstrdup(key);
+    array[count++] = xstrdup(val);
+    array[count] = NULL;
+
+    *sl = array;
+}
+
 int match_option_size(const char *prefix, size_t len,
                       char *arg, char **argopt)
 {
@@ -559,6 +582,20 @@ int parse_nic_config(libxl_device_nic *nic, XLU_Config **config, char *token)
         fprintf(stderr, "the accel parameter for vifs is currently not supported\n");
     } else if (MATCH_OPTION("devid", token, oparg)) {
         nic->devid = parse_ulong(oparg);
+    } else if (MATCH_FEATURE("require", token, oparg)) {
+        char *key = NULL, *value = NULL;
+        int rc;
+
+        rc = split_string_into_pair(oparg, "=", &key, &value);
+        if (rc != 0) {
+            fprintf(stderr, "failed to parse vif backend feature %s", oparg);
+            return 1;
+        }
+
+        add_to_kvlist(&nic->backend_features, key, value);
+
+        free(key);
+        free(value);
     } else {
         fprintf(stderr, "unrecognized argument `%s'\n", token);
         return 1;
diff --git a/tools/xl/xl_parse.h b/tools/xl/xl_parse.h
index cc459fb43f..aea07394cc 100644
--- a/tools/xl/xl_parse.h
+++ b/tools/xl/xl_parse.h
@@ -40,6 +40,8 @@ int match_option_size(const char *prefix, size_t len,
 #define MATCH_OPTION(prefix, arg, oparg) \
     match_option_size((prefix "="), sizeof((prefix)), (arg), &(oparg))
 
+#define MATCH_FEATURE(prefix, arg, oparg) \
+    match_option_size((prefix "-"), sizeof((prefix)), (arg), &(oparg))
 
 void split_string_into_string_list(const char *str, const char *delim,
                                    libxl_string_list *psl);
-- 
2.11.0


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

[PATCH RFC 7/8] xen-blkback: frontend feature control

[Joao Martins]

Toolstack may write values to the "require" subdirectory in the
backend main directory (e.g. backend/vbd/X/Y/). Read these values
and use them when announcing those to the frontend. When backend
scans frontend features the values set in the require directory
take precedence, hence making no significant changes in feature
parsing.

xenbus_read_feature() reads from require subdirectory and prints that
value and otherwise writing a default_val in the entry. We then replace
all instances of xenbus_printf to use these previously seeded features.
A backend_features struct is introduced and all values set there are
used in place of the module parameters being used.

Note, however that feature-barrier, feature-flush-support and
feature-discard aren't probed because first two are physical
device dependent and feature-discard already has tunables to
adjust.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 drivers/block/xen-blkback/blkback.c |  2 +-
 drivers/block/xen-blkback/common.h  |  1 +
 drivers/block/xen-blkback/xenbus.c  | 66 ++++++++++++++++++++++++++++++++-----
 3 files changed, 60 insertions(+), 9 deletions(-)

diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
index c90e933330b6..05b3f124c871 100644
--- a/drivers/block/xen-blkback/blkback.c
+++ b/drivers/block/xen-blkback/blkback.c
@@ -1271,7 +1271,7 @@ static int dispatch_rw_block_io(struct xen_blkif_ring *ring,
 	    unlikely((req->operation != BLKIF_OP_INDIRECT) &&
 		     (nseg > BLKIF_MAX_SEGMENTS_PER_REQUEST)) ||
 	    unlikely((req->operation == BLKIF_OP_INDIRECT) &&
-		     (nseg > MAX_INDIRECT_SEGMENTS))) {
+		     (nseg > ring->blkif->vbd.max_indirect_segs))) {
 		pr_debug("Bad number of segments in request (%d)\n", nseg);
 		/* Haven't submitted any bio's yet. */
 		goto fail_response;
diff --git a/drivers/block/xen-blkback/common.h b/drivers/block/xen-blkback/common.h
index a7832428e0da..ff12f2d883b9 100644
--- a/drivers/block/xen-blkback/common.h
+++ b/drivers/block/xen-blkback/common.h
@@ -229,6 +229,7 @@ struct xen_vbd {
 	unsigned int		discard_secure:1;
 	unsigned int		feature_gnt_persistent:1;
 	unsigned int		overflow_max_grants:1;
+	unsigned int		max_indirect_segs;
 };
 
 struct backend_info;
diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
index 48d796ea3626..31683f29d5fb 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -25,11 +25,19 @@
 
 /* On the XenBus the max length of 'ring-ref%u'. */
 #define RINGREF_NAME_LEN (20)
+#define REQUIRE_PATH_LEN (256)
+
+struct backend_features {
+	unsigned max_queues;
+	unsigned max_ring_order;
+	unsigned pers_grants;
+};
 
 struct backend_info {
 	struct xenbus_device	*dev;
 	struct xen_blkif	*blkif;
 	struct xenbus_watch	backend_watch;
+	struct backend_features features;
 	unsigned		major;
 	unsigned		minor;
 	char			*mode;
@@ -602,6 +610,40 @@ int xen_blkbk_barrier(struct xenbus_transaction xbt,
 	return err;
 }
 
+static int xenbus_read_feature(const char *dir, const char *node,
+			       unsigned int default_val)
+{
+	char reqnode[REQUIRE_PATH_LEN];
+	unsigned int val;
+
+	snprintf(reqnode, REQUIRE_PATH_LEN, "%s/require", dir);
+	val = xenbus_read_unsigned(reqnode, node, default_val);
+	return val;
+}
+
+static void xen_blkbk_probe_features(struct xenbus_device *dev,
+				     struct backend_info *be)
+{
+	struct backend_features *ft = &be->features;
+	struct xen_vbd *vbd = &be->blkif->vbd;
+
+	vbd->max_indirect_segs = xenbus_read_feature(dev->nodename,
+						"feature-max-indirect-segments",
+						MAX_INDIRECT_SEGMENTS);
+
+	ft->max_queues = xenbus_read_feature(dev->nodename,
+					     "multi-queue-max-queues",
+					     xenblk_max_queues);
+
+	ft->max_ring_order = xenbus_read_feature(dev->nodename,
+						 "max-ring-page-order",
+						 xen_blkif_max_ring_order);
+
+	ft->pers_grants = xenbus_read_feature(dev->nodename,
+					      "feature-persistent",
+					      1);
+}
+
 /*
  * Entry point to this code when a new device is created.  Allocate the basic
  * structures, and watch the store waiting for the hotplug scripts to tell us
@@ -613,6 +655,7 @@ static int xen_blkbk_probe(struct xenbus_device *dev,
 	int err;
 	struct backend_info *be = kzalloc(sizeof(struct backend_info),
 					  GFP_KERNEL);
+	struct backend_features *ft;
 
 	/* match the pr_debug in xen_blkbk_remove */
 	pr_debug("%s %p %d\n", __func__, dev, dev->otherend_id);
@@ -633,9 +676,12 @@ static int xen_blkbk_probe(struct xenbus_device *dev,
 		goto fail;
 	}
 
+	xen_blkbk_probe_features(dev, be);
+	ft = &be->features;
+
 	err = xenbus_printf(XBT_NIL, dev->nodename,
 			    "feature-max-indirect-segments", "%u",
-			    MAX_INDIRECT_SEGMENTS);
+			    be->blkif->vbd.max_indirect_segs);
 	if (err)
 		dev_warn(&dev->dev,
 			 "writing %s/feature-max-indirect-segments (%d)",
@@ -643,7 +689,7 @@ static int xen_blkbk_probe(struct xenbus_device *dev,
 
 	/* Multi-queue: advertise how many queues are supported by us.*/
 	err = xenbus_printf(XBT_NIL, dev->nodename,
-			    "multi-queue-max-queues", "%u", xenblk_max_queues);
+			    "multi-queue-max-queues", "%u", ft->max_queues);
 	if (err)
 		pr_warn("Error writing multi-queue-max-queues\n");
 
@@ -656,7 +702,7 @@ static int xen_blkbk_probe(struct xenbus_device *dev,
 		goto fail;
 
 	err = xenbus_printf(XBT_NIL, dev->nodename, "max-ring-page-order", "%u",
-			    xen_blkif_max_ring_order);
+			    ft->max_ring_order);
 	if (err)
 		pr_warn("%s write out 'max-ring-page-order' failed\n", __func__);
 
@@ -849,6 +895,7 @@ static void frontend_changed(struct xenbus_device *dev,
  */
 static void connect(struct backend_info *be)
 {
+	struct backend_features *ft = &be->features;
 	struct xenbus_transaction xbt;
 	int err;
 	struct xenbus_device *dev = be->dev;
@@ -870,7 +917,8 @@ static void connect(struct backend_info *be)
 
 	xen_blkbk_barrier(xbt, be, be->blkif->vbd.flush_support);
 
-	err = xenbus_printf(xbt, dev->nodename, "feature-persistent", "%u", 1);
+	err = xenbus_printf(xbt, dev->nodename, "feature-persistent", "%u",
+			    ft->pers_grants);
 	if (err) {
 		xenbus_dev_fatal(dev, err, "writing %s/feature-persistent",
 				 dev->nodename);
@@ -933,6 +981,7 @@ static int read_per_ring_refs(struct xen_blkif_ring *ring, const char *dir)
 	struct pending_req *req, *n;
 	int err, i, j;
 	struct xen_blkif *blkif = ring->blkif;
+	struct backend_features *ft = &blkif->be->features;
 	struct xenbus_device *dev = blkif->be->dev;
 	unsigned int ring_page_order, nr_grefs, evtchn;
 
@@ -957,7 +1006,7 @@ static int read_per_ring_refs(struct xen_blkif_ring *ring, const char *dir)
 	} else {
 		unsigned int i;
 
-		if (ring_page_order > xen_blkif_max_ring_order) {
+		if (ring_page_order > ft->max_ring_order) {
 			err = -EINVAL;
 			xenbus_dev_fatal(dev, err, "%s/request %d ring page order exceed max:%d",
 					 dir, ring_page_order,
@@ -1030,6 +1079,7 @@ static int read_per_ring_refs(struct xen_blkif_ring *ring, const char *dir)
 
 static int connect_ring(struct backend_info *be)
 {
+	struct backend_features *ft = &be->features;
 	struct xenbus_device *dev = be->dev;
 	unsigned int pers_grants;
 	char protocol[64] = "";
@@ -1058,7 +1108,7 @@ static int connect_ring(struct backend_info *be)
 	}
 	pers_grants = xenbus_read_unsigned(dev->otherend, "feature-persistent",
 					   0);
-	be->blkif->vbd.feature_gnt_persistent = pers_grants;
+	be->blkif->vbd.feature_gnt_persistent = (pers_grants && ft->pers_grants);
 	be->blkif->vbd.overflow_max_grants = 0;
 
 	/*
@@ -1067,12 +1117,12 @@ static int connect_ring(struct backend_info *be)
 	requested_num_queues = xenbus_read_unsigned(dev->otherend,
 						    "multi-queue-num-queues",
 						    1);
-	if (requested_num_queues > xenblk_max_queues
+	if (requested_num_queues > ft->max_queues
 	    || requested_num_queues == 0) {
 		/* Buggy or malicious guest. */
 		xenbus_dev_fatal(dev, err,
 				"guest requested %u queues, exceeding the maximum of %u.",
-				requested_num_queues, xenblk_max_queues);
+				requested_num_queues, ft->max_queues);
 		return -ENOSYS;
 	}
 	be->blkif->nr_rings = requested_num_queues;
-- 
2.11.0


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

[PATCH RFC 8/8] xen-netback: frontend feature control

[Joao Martins]

Toolstack may write values to the "require" subdirectory in the backend
main xenstore directory (e.g. backend/vif/X/Y/). Read these values and
use them when announcing those to the frontend. When backend scans
frontend features the values set in the require directory take
precedence, hence making no significant changes in feature parsing.

This is achieved by using the newly introduced helper
(xenbus_printf_feature()) which reads from require subdirectory and
prints that value and otherwise printing a default_val in the entry. We
then replace all instances of xenbus_printf by this new helper. A
backend_features struct is introduced and all values set there are used
in place of the module parameters being used.

Note, however that feature-rx-copy, feature-rx-flip aren't probed
because first two aren't implemented the full set of possibilities.
Additionally probe to for 'feature-no-csum-offload' to allow toolstack
to control per device checksum offloading.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 drivers/net/xen-netback/xenbus.c | 122 +++++++++++++++++++++++++++++++--------
 1 file changed, 99 insertions(+), 23 deletions(-)

diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
index a56d3eab35dd..391f1f2e1af2 100644
--- a/drivers/net/xen-netback/xenbus.c
+++ b/drivers/net/xen-netback/xenbus.c
@@ -22,9 +22,25 @@
 #include <linux/vmalloc.h>
 #include <linux/rtnetlink.h>
 
+#define REQUIRE_PATH_LEN (256)
+
+struct backend_features {
+	unsigned int max_queues;
+	unsigned int split_evtchn:1;
+	unsigned int ctrl_ring:1;
+	unsigned int can_sg:1;
+	unsigned int gso_v4:1;
+	unsigned int gso_v6:1;
+	unsigned int mcast_ctrl:1;
+	unsigned int dyn_mcast_ctrl:1;
+	unsigned int ip_no_csum:1;
+	unsigned int ipv6_csum:1;
+};
+
 struct backend_info {
 	struct xenbus_device *dev;
 	struct xenvif *vif;
+	struct backend_features features;
 
 	/* This is the state that will be reflected in xenstore when any
 	 * active hotplug script completes.
@@ -48,6 +64,17 @@ static void xen_unregister_watchers(struct xenvif *vif);
 static void set_backend_state(struct backend_info *be,
 			      enum xenbus_state state);
 
+static int xenbus_read_feature(const char *dir, const char *node,
+			       unsigned int default_val)
+{
+	char reqnode[REQUIRE_PATH_LEN];
+	unsigned int val;
+
+	snprintf(reqnode, REQUIRE_PATH_LEN, "%s/require", dir);
+	val = xenbus_read_unsigned(reqnode, node, default_val);
+	return val;
+}
+
 #ifdef CONFIG_DEBUG_FS
 struct dentry *xen_netback_dbg_root = NULL;
 
@@ -280,6 +307,32 @@ static int netback_remove(struct xenbus_device *dev)
 	return 0;
 }
 
+static void netback_probe_features(struct xenbus_device *dev,
+				   struct backend_info *be)
+{
+	struct backend_features *ft = &be->features;
+
+	ft->can_sg = xenbus_read_feature(dev->nodename, "feature-sg", 1);
+	ft->gso_v4 = xenbus_read_feature(dev->nodename, "feature-gso-v4", 1);
+	ft->gso_v6 = xenbus_read_feature(dev->nodename, "feature-gso-v6", 1);
+	ft->gso_v6 = xenbus_read_feature(dev->nodename, "feature-gso-v6", 1);
+	ft->ipv6_csum = xenbus_read_feature(dev->nodename,
+					    "feature-ipv6-csum-offload", 1);
+	ft->ip_no_csum = xenbus_read_feature(dev->nodename,
+					    "feature-no-csum-offload", 0);
+	ft->mcast_ctrl = xenbus_read_feature(dev->nodename,
+					     "feature-multicast-control", 1);
+	ft->dyn_mcast_ctrl = xenbus_read_feature(dev->nodename,
+					"feature-dynamic-multicast-control", 1);
+	ft->split_evtchn = xenbus_read_feature(dev->nodename,
+					       "feature-split-event-channels",
+					       separate_tx_rx_irq);
+	ft->max_queues = xenbus_read_feature(dev->nodename,
+					     "multi-queue-max-queues",
+					     xenvif_max_queues);
+	ft->ctrl_ring = xenbus_read_feature(dev->nodename, "feature-ctrl-ring",
+					    1);
+}
 
 /**
  * Entry point to this code when a new device is created.  Allocate the basic
@@ -291,8 +344,8 @@ static int netback_probe(struct xenbus_device *dev,
 	const char *message;
 	struct xenbus_transaction xbt;
 	int err;
-	int sg;
 	const char *script;
+	struct backend_features *ft;
 	struct backend_info *be = kzalloc(sizeof(struct backend_info),
 					  GFP_KERNEL);
 	if (!be) {
@@ -309,7 +362,8 @@ static int netback_probe(struct xenbus_device *dev,
 	if (err)
 		goto fail;
 
-	sg = 1;
+	netback_probe_features(dev, be);
+	ft = &be->features;
 
 	do {
 		err = xenbus_transaction_start(&xbt);
@@ -318,21 +372,22 @@ static int netback_probe(struct xenbus_device *dev,
 			goto fail;
 		}
 
-		err = xenbus_printf(xbt, dev->nodename, "feature-sg", "%d", sg);
+		err = xenbus_printf(xbt, dev->nodename, "feature-sg", "%d",
+				    ft->can_sg);
 		if (err) {
 			message = "writing feature-sg";
 			goto abort_transaction;
 		}
 
 		err = xenbus_printf(xbt, dev->nodename, "feature-gso-tcpv4",
-				    "%d", sg);
+				    "%d", ft->gso_v4);
 		if (err) {
 			message = "writing feature-gso-tcpv4";
 			goto abort_transaction;
 		}
 
 		err = xenbus_printf(xbt, dev->nodename, "feature-gso-tcpv6",
-				    "%d", sg);
+				    "%d", ft->gso_v6);
 		if (err) {
 			message = "writing feature-gso-tcpv6";
 			goto abort_transaction;
@@ -341,12 +396,21 @@ static int netback_probe(struct xenbus_device *dev,
 		/* We support partial checksum setup for IPv6 packets */
 		err = xenbus_printf(xbt, dev->nodename,
 				    "feature-ipv6-csum-offload",
-				    "%d", 1);
+				    "%d", ft->ipv6_csum);
 		if (err) {
 			message = "writing feature-ipv6-csum-offload";
 			goto abort_transaction;
 		}
 
+		/* We support partial checksum setup for IPv4 packets */
+		err = xenbus_printf(xbt, dev->nodename,
+				    "feature-no-csum-offload",
+				    "%d", ft->ip_no_csum);
+		if (err) {
+			message = "writing feature-no-csum-offload";
+			goto abort_transaction;
+		}
+
 		/* We support rx-copy path. */
 		err = xenbus_printf(xbt, dev->nodename,
 				    "feature-rx-copy", "%d", 1);
@@ -368,7 +432,8 @@ static int netback_probe(struct xenbus_device *dev,
 
 		/* We support dynamic multicast-control. */
 		err = xenbus_printf(xbt, dev->nodename,
-				    "feature-multicast-control", "%d", 1);
+				    "feature-multicast-control", "%d",
+				    ft->mcast_ctrl);
 		if (err) {
 			message = "writing feature-multicast-control";
 			goto abort_transaction;
@@ -376,7 +441,7 @@ static int netback_probe(struct xenbus_device *dev,
 
 		err = xenbus_printf(xbt, dev->nodename,
 				    "feature-dynamic-multicast-control",
-				    "%d", 1);
+				    "%d", ft->dyn_mcast_ctrl);
 		if (err) {
 			message = "writing feature-dynamic-multicast-control";
 			goto abort_transaction;
@@ -396,19 +461,19 @@ static int netback_probe(struct xenbus_device *dev,
 	 */
 	err = xenbus_printf(XBT_NIL, dev->nodename,
 			    "feature-split-event-channels",
-			    "%u", separate_tx_rx_irq);
+			    "%u", ft->split_evtchn);
 	if (err)
 		pr_debug("Error writing feature-split-event-channels\n");
 
 	/* Multi-queue support: This is an optional feature. */
 	err = xenbus_printf(XBT_NIL, dev->nodename,
-			    "multi-queue-max-queues", "%u", xenvif_max_queues);
+			    "multi-queue-max-queues", "%u", ft->max_queues);
 	if (err)
 		pr_debug("Error writing multi-queue-max-queues\n");
 
 	err = xenbus_printf(XBT_NIL, dev->nodename,
 			    "feature-ctrl-ring",
-			    "%u", true);
+			    "%u", ft->ctrl_ring);
 	if (err)
 		pr_debug("Error writing feature-ctrl-ring\n");
 
@@ -904,6 +969,9 @@ static int connect_ctrl_ring(struct backend_info *be)
 	unsigned int evtchn;
 	int err;
 
+	if (!be->features.ctrl_ring)
+		goto done;
+
 	err = xenbus_scanf(XBT_NIL, dev->otherend,
 			   "ctrl-ring-ref", "%u", &val);
 	if (err < 0)
@@ -951,11 +1019,11 @@ static void connect(struct backend_info *be)
 	 */
 	requested_num_queues = xenbus_read_unsigned(dev->otherend,
 					"multi-queue-num-queues", 1);
-	if (requested_num_queues > xenvif_max_queues) {
+	if (requested_num_queues > be->features.max_queues) {
 		/* buggy or malicious guest */
 		xenbus_dev_fatal(dev, -EINVAL,
 				 "guest requested %u queues, exceeding the maximum of %u.",
-				 requested_num_queues, xenvif_max_queues);
+				 requested_num_queues, be->features.max_queues);
 		return;
 	}
 
@@ -1110,10 +1178,13 @@ static int connect_data_rings(struct backend_info *be,
 		goto err;
 	}
 
+	err = -EOPNOTSUPP;
 	/* Try split event channels first, then single event channel. */
-	err = xenbus_gather(XBT_NIL, xspath,
-			    "event-channel-tx", "%u", &tx_evtchn,
-			    "event-channel-rx", "%u", &rx_evtchn, NULL);
+	if (be->features.split_evtchn)
+		err = xenbus_gather(XBT_NIL, xspath,
+				    "event-channel-tx", "%u", &tx_evtchn,
+				    "event-channel-rx", "%u", &rx_evtchn, NULL);
+
 	if (err < 0) {
 		err = xenbus_scanf(XBT_NIL, xspath,
 				   "event-channel", "%u", &tx_evtchn);
@@ -1173,21 +1244,26 @@ static int read_xenbus_vif_flags(struct backend_info *be)
 		be->vif->stall_timeout = 0;
 	}
 
-	vif->can_sg = !!xenbus_read_unsigned(dev->otherend, "feature-sg", 0);
+	vif->can_sg = be->features.can_sg &&
+			!!xenbus_read_unsigned(dev->otherend, "feature-sg", 0);
 
 	vif->gso_mask = 0;
 
-	if (xenbus_read_unsigned(dev->otherend, "feature-gso-tcpv4", 0))
+	if (be->features.gso_v4 &&
+	    xenbus_read_unsigned(dev->otherend, "feature-gso-tcpv4", 0))
 		vif->gso_mask |= GSO_BIT(TCPV4);
 
-	if (xenbus_read_unsigned(dev->otherend, "feature-gso-tcpv6", 0))
+	if (be->features.gso_v6 &&
+	    xenbus_read_unsigned(dev->otherend, "feature-gso-tcpv6", 0))
 		vif->gso_mask |= GSO_BIT(TCPV6);
 
-	vif->ip_csum = !xenbus_read_unsigned(dev->otherend,
-					     "feature-no-csum-offload", 0);
+	vif->ip_csum = !be->features.ip_no_csum &&
+			!xenbus_read_unsigned(dev->otherend,
+					      "feature-no-csum-offload", 0);
 
-	vif->ipv6_csum = !!xenbus_read_unsigned(dev->otherend,
-						"feature-ipv6-csum-offload", 0);
+	vif->ipv6_csum = be->features.ipv6_csum &&
+			!!xenbus_read_unsigned(dev->otherend,
+					       "feature-ipv6-csum-offload", 0);
 
 	return 0;
 }
-- 
2.11.0


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

Re: [PATCH RFC 2/8] public/io/netif: add directory for backend parameters

[Paul Durrant]

> -----Original Message-----
> From: Joao Martins [mailto:joao.m.martins@oracle.com]
> Sent: 02 November 2017 18:06
> To: Xen Development List <xen-devel@lists.xen.org>
> Cc: Joao Martins <joao.m.martins@oracle.com>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Paul Durrant <Paul.Durrant@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>
> Subject: [PATCH RFC 2/8] public/io/netif: add directory for backend
> parameters
> 
> The proposed directory provides a mechanism for tools to control the
> maximum feature set of the device being provisioned by backend.
> The parameters/features include offloading features, number of
> queues etc.
> 
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
>  xen/include/public/io/netif.h | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/xen/include/public/io/netif.h b/xen/include/public/io/netif.h
> index 2454448baa..a412e4771d 100644
> --- a/xen/include/public/io/netif.h
> +++ b/xen/include/public/io/netif.h
> @@ -161,6 +161,22 @@
>   */
> 
>  /*
> + * The directory "require" maybe be created in backend path by tools
> + * domain to override the maximum feature set that backend provides to
> the
> + * frontend. The children entries within this directory are features names
> + * and the correspondent values that should be used backend as defaults
> e.g.:
> + *
> + * /local/domain/X/backend/<domid>/<handle>/require
> + * /local/domain/X/backend/<domid>/<handle>/require/multi-queue-
> max-queues = "2"
> + * /local/domain/X/backend/<domid>/<handle>/require/feature-no-csum-
> offload = "1"
> + *
> + * In the example above, network backend will negotiate up to a maximum
> of
> + * two queues with frontend plus disabling IPv4 checksum offloading.
> + *
> + * This directory and its children entries shall only be visible to the backend.
> + */
> +

What should happen if the toolstack sets something in 'require' that the backend cannot provide? I don't see anything in your RFC patches to check that the backend has responded appropriately to the keys.

  Paul

> +/*
>   * Control ring
>   * ============
>   *
> --
> 2.11.0


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

Re: [PATCH RFC 2/8] public/io/netif: add directory for backend parameters

[Joao Martins]

On Mon, Nov 06, 2017 at 10:33:59AM +0000, Paul Durrant wrote:
> > -----Original Message-----
> > From: Joao Martins [mailto:joao.m.martins@oracle.com]
> > Sent: 02 November 2017 18:06
> > To: Xen Development List <xen-devel@lists.xen.org>
> > Cc: Joao Martins <joao.m.martins@oracle.com>; Konrad Rzeszutek Wilk
> > <konrad.wilk@oracle.com>; Paul Durrant <Paul.Durrant@citrix.com>; Wei Liu
> > <wei.liu2@citrix.com>
> > Subject: [PATCH RFC 2/8] public/io/netif: add directory for backend
> > parameters
> > 
> > The proposed directory provides a mechanism for tools to control the
> > maximum feature set of the device being provisioned by backend.
> > The parameters/features include offloading features, number of
> > queues etc.
> > 
> > Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> > ---
> >  xen/include/public/io/netif.h | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/xen/include/public/io/netif.h b/xen/include/public/io/netif.h
> > index 2454448baa..a412e4771d 100644
> > --- a/xen/include/public/io/netif.h
> > +++ b/xen/include/public/io/netif.h
> > @@ -161,6 +161,22 @@
> >   */
> > 
> >  /*
> > + * The directory "require" maybe be created in backend path by tools
> > + * domain to override the maximum feature set that backend provides to
> > the
> > + * frontend. The children entries within this directory are features names
> > + * and the correspondent values that should be used backend as defaults
> > e.g.:
> > + *
> > + * /local/domain/X/backend/<domid>/<handle>/require
> > + * /local/domain/X/backend/<domid>/<handle>/require/multi-queue-
> > max-queues = "2"
> > + * /local/domain/X/backend/<domid>/<handle>/require/feature-no-csum-
> > offload = "1"
> > + *
> > + * In the example above, network backend will negotiate up to a maximum
> > of
> > + * two queues with frontend plus disabling IPv4 checksum offloading.
> > + *
> > + * This directory and its children entries shall only be visible to the backend.
> > + */
> > +
> 
> What should happen if the toolstack sets something in 'require' that
> the backend cannot provide? I don't see anything in your RFC patches
> to check that the backend has responded appropriately to the keys.

Hmm, you're right that this RFC doesn't handle that properly - but for the
ones the backend provide I had suggested (albeit not implemented here)
back in the other thread that we could compare the values of feature in
"require" with the one announced to the frontend. But well this wouldn't
cover the non-provided ones, and possibly would fall a bit as a hack.

I could change the format of the entries within "require"
directory to be e.g. "<id>-<feature-name> = <feature-value>" and the
acknowledgement entry would come in the form "<id>-status
= <error code>". Consequently the lack of a "<id>-status" entry would
have a stronger semantic i.e. unsupported and ignored. The toolstack then would have
means to check whether the feature was really succesfull set as desired
or not. But then one question comes to mind: should the backend be
prevented to init in the event that the features requested fail to be
set? In which case uevent (on Linux) isn't triggered and xenbus state doesn't
get changed and toolstack would fail with timeout later on.

Also, a nice thing of this stuff is that we could also use this to set
set backend implementation specific parameters that are not
described or relevant in I/O specs. But then I start to wonder where would
be the correct place for backends to specify its maximum feature set of
changeable entries? Maybe:

/local/domain/X/backend/vif/features/<feature-name>
/local/domain/X/backend/vif/features/<feature-name>-desc = "Description
of <feature-name>"

Cheers,
Joao

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

Re: [PATCH RFC 3/8] libxl: add backend_features to libxl_device_disk

[Oleksandr Grytsov]

[-- Attachment #1.1: Type: text/plain, Size: 1395 bytes --]

On Thu, Nov 2, 2017 at 8:06 PM, Joao Martins <joao.m.martins@oracle.com>
wrote:

> The function libxl__device_generic_add will have an additional
> argument whereby it adds a second set of entries visible to the
> backend only. These entries will then be used for devices
> thus overriding backend maximum feature set with this user-defined ones.
>
> libxl_device_disk.backend_features are a key value store storing:
>  <feature-name> = <feature-value>
>
> xl|libxl are stateless with respect to feature names therefore is up to the
> admin to carefully select those. If backend isn't supported therefore the
> features won't be overwritten.
>
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
>  tools/libxl/libxl.h          |  8 ++++++++
>  tools/libxl/libxl_console.c  |  5 +++--
>  tools/libxl/libxl_device.c   | 37 +++++++++++++++++++++++++++++++++----
>  tools/libxl/libxl_disk.c     | 17 +++++++++++++++--
>  tools/libxl/libxl_internal.h |  4 +++-
>  tools/libxl/libxl_pci.c      |  2 +-
>  tools/libxl/libxl_types.idl  |  1 +
>  tools/libxl/libxl_usb.c      |  2 +-
>  8 files changed, 65 insertions(+), 11 deletions(-)
>
>
No need to extend libxl__device_generic_add with additional parameter
(brents).
You can add nested entry in libxl__set_xenstore_<device> as following:

flexarray_append(back, "require/feature-persistent", "0");

-- 
Best Regards,
Oleksandr Grytsov.

[-- Attachment #1.2: Type: text/html, Size: 2259 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH RFC 3/8] libxl: add backend_features to libxl_device_disk

[Joao Martins]

On 11/07/2017 11:28 AM, Oleksandr Grytsov wrote:
> On Thu, Nov 2, 2017 at 8:06 PM, Joao Martins <joao.m.martins@oracle.com
> <mailto:joao.m.martins@oracle.com>> wrote:
> 
>     The function libxl__device_generic_add will have an additional
>     argument whereby it adds a second set of entries visible to the
>     backend only. These entries will then be used for devices
>     thus overriding backend maximum feature set with this user-defined ones.
> 
>     libxl_device_disk.backend_features are a key value store storing:
>      <feature-name> = <feature-value>
> 
>     xl|libxl are stateless with respect to feature names therefore is up to the
>     admin to carefully select those. If backend isn't supported therefore the
>     features won't be overwritten.
> 
>     Signed-off-by: Joao Martins <joao.m.martins@oracle.com
>     <mailto:joao.m.martins@oracle.com>>
>     ---
>      tools/libxl/libxl.h          |  8 ++++++++
>      tools/libxl/libxl_console.c  |  5 +++--
>      tools/libxl/libxl_device.c   | 37 +++++++++++++++++++++++++++++++++----
>      tools/libxl/libxl_disk.c     | 17 +++++++++++++++--
>      tools/libxl/libxl_internal.h |  4 +++-
>      tools/libxl/libxl_pci.c      |  2 +-
>      tools/libxl/libxl_types.idl  |  1 +
>      tools/libxl/libxl_usb.c      |  2 +-
>      8 files changed, 65 insertions(+), 11 deletions(-)
> 
> 
> No need to extend libxl__device_generic_add with additional parameter (brents).
> You can add nested entry in libxl__set_xenstore_<device> as following:
> 
> flexarray_append(back, "require/feature-persistent", "0");

Right, although entries on "back" array will have readonly permission to the
frontend. And these newly added "require" directory in this RFC was meant to be
only visible to the backend, hence only having XS_PERM_NONE permission set.

Joao

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

Re: [PATCH RFC 2/8] public/io/netif: add directory for backend parameters

[Wei Liu]

(Three months after you sent this, sorry...)

On Mon, Nov 06, 2017 at 12:33:06PM +0000, Joao Martins wrote:
> On Mon, Nov 06, 2017 at 10:33:59AM +0000, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Joao Martins [mailto:joao.m.martins@oracle.com]
> > > Sent: 02 November 2017 18:06
> > > To: Xen Development List <xen-devel@lists.xen.org>
> > > Cc: Joao Martins <joao.m.martins@oracle.com>; Konrad Rzeszutek Wilk
> > > <konrad.wilk@oracle.com>; Paul Durrant <Paul.Durrant@citrix.com>; Wei Liu
> > > <wei.liu2@citrix.com>
> > > Subject: [PATCH RFC 2/8] public/io/netif: add directory for backend
> > > parameters
> > > 
> > > The proposed directory provides a mechanism for tools to control the
> > > maximum feature set of the device being provisioned by backend.
> > > The parameters/features include offloading features, number of
> > > queues etc.
> > > 
> > > Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> > > ---
> > >  xen/include/public/io/netif.h | 16 ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > > 
> > > diff --git a/xen/include/public/io/netif.h b/xen/include/public/io/netif.h
> > > index 2454448baa..a412e4771d 100644
> > > --- a/xen/include/public/io/netif.h
> > > +++ b/xen/include/public/io/netif.h
> > > @@ -161,6 +161,22 @@
> > >   */
> > > 
> > >  /*
> > > + * The directory "require" maybe be created in backend path by tools
> > > + * domain to override the maximum feature set that backend provides to
> > > the
> > > + * frontend. The children entries within this directory are features names
> > > + * and the correspondent values that should be used backend as defaults
> > > e.g.:
> > > + *
> > > + * /local/domain/X/backend/<domid>/<handle>/require
> > > + * /local/domain/X/backend/<domid>/<handle>/require/multi-queue-
> > > max-queues = "2"
> > > + * /local/domain/X/backend/<domid>/<handle>/require/feature-no-csum-
> > > offload = "1"
> > > + *
> > > + * In the example above, network backend will negotiate up to a maximum
> > > of
> > > + * two queues with frontend plus disabling IPv4 checksum offloading.
> > > + *
> > > + * This directory and its children entries shall only be visible to the backend.
> > > + */
> > > +
> > 
> > What should happen if the toolstack sets something in 'require' that
> > the backend cannot provide? I don't see anything in your RFC patches
> > to check that the backend has responded appropriately to the keys.
> 
> Hmm, you're right that this RFC doesn't handle that properly - but for the
> ones the backend provide I had suggested (albeit not implemented here)
> back in the other thread that we could compare the values of feature in
> "require" with the one announced to the frontend. But well this wouldn't
> cover the non-provided ones, and possibly would fall a bit as a hack.
> 
> I could change the format of the entries within "require"
> directory to be e.g. "<id>-<feature-name> = <feature-value>" and the
> acknowledgement entry would come in the form "<id>-status
> = <error code>". Consequently the lack of a "<id>-status" entry would
> have a stronger semantic i.e. unsupported and ignored. The toolstack then would have
> means to check whether the feature was really succesfull set as desired
> or not. But then one question comes to mind: should the backend be
> prevented to init in the event that the features requested fail to be
> set? In which case uevent (on Linux) isn't triggered and xenbus state doesn't
> get changed and toolstack would fail with timeout later on.
> 

I think the backend should not proceed if it can't meet the
requirements. But to be clear I also don't think the timeout behaviour
should be used to determine if the setting is successful because it is
asking other part of the system to pick up the slack and system
administrators would be left in the dark (unless there is easily
accessible message that can be obtained by libxl to return to system
administrators).

> Also, a nice thing of this stuff is that we could also use this to set
> set backend implementation specific parameters that are not
> described or relevant in I/O specs. But then I start to wonder where would
> be the correct place for backends to specify its maximum feature set of
> changeable entries? Maybe:
> 
> /local/domain/X/backend/vif/features/<feature-name>
> /local/domain/X/backend/vif/features/<feature-name>-desc = "Description
> of <feature-name>"

No idea. I'm very bad at naming things.

Wei.

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

Re: [PATCH RFC 0/8] libxl, xl, public/io: PV backends feature control

[Roger Pau Monné]

On Thu, Nov 02, 2017 at 06:06:08PM +0000, Joao Martins wrote:
> Hey folks,
> 
> Presented herewith is an attempt to implement PV backends feature control
> as discussed in the list (https://lists.xen.org/archives/html/xen-devel/2017-09/msg00766.html)
> 
> Given that this a simple proposal hence I thought to include all changes
> involved in the same patchset such that everyone see all the changes and has a
> better estimate (but restricted to xen-devel just for the RFC purposes).
> 
> The motivation here is to allow system administrators more fine grained
> control of the device features being used by guest.
> 
> The only change I made compared to the proposed discussed above was to use
> "require" instead of "request" as the prefix because there is a feature which

require in the above context, like:

require-multi-queue-max-queues=2

Seems to imply that the guest _must_ have support for multiqueue and
use exactly two queues.

What about using 'config' prefix?

config-multi-queue-max-queues=2
config-feature-persistent=0

> has "request" in it. But if "request" is still preferred as a prefix I can change
> it up.
> 
> The scheme proposed is quite simple:
> 
> * The directory "require" is created (inside the backend path) and within that
> directory the features/capabilities names and values are written.

I'm quite sure I'm missing something, but what's the point in having
this require directory in the xenstore backend path?

AFAICT you should just write this directly to the backend directory,
and backends should be modified to check if there's a value already
present before writing the default one.

> * Toolstack constructs a key value store of features, and user specifies those
> through special entry names prefixed also as "require". Toolstack is stateless thus sys
> admin has full control over what to pass to the backend. In other words it
> doesn't look at particular feature names/values.
> 
> * The backend will then use that for seeding its maximum feature set to the
> frontend.

Oh, I see. So the backend picks up the suggested config from this
directory together with it's capabilities and then produces the final
set of features exposed to the frontend.

In order to prevent adding more logic to the backends, would it make
sense to export the backend capabilities in /sys/ (or sysctl on BSDs)
and do those calculations from the toolstack itself, so that the
toolstack directly writes the features to the backend top level
xenstore directory?

Thanks, Roger.

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

Re: [PATCH RFC 0/8] libxl, xl, public/io: PV backends feature control

[Juergen Gross]

On 07/02/18 12:16, Roger Pau Monné wrote:
> On Thu, Nov 02, 2017 at 06:06:08PM +0000, Joao Martins wrote:
>> Hey folks,
>>
>> Presented herewith is an attempt to implement PV backends feature control
>> as discussed in the list (https://lists.xen.org/archives/html/xen-devel/2017-09/msg00766.html)
>>
>> Given that this a simple proposal hence I thought to include all changes
>> involved in the same patchset such that everyone see all the changes and has a
>> better estimate (but restricted to xen-devel just for the RFC purposes).
>>
>> The motivation here is to allow system administrators more fine grained
>> control of the device features being used by guest.
>>
>> The only change I made compared to the proposed discussed above was to use
>> "require" instead of "request" as the prefix because there is a feature which
> 
> require in the above context, like:
> 
> require-multi-queue-max-queues=2
> 
> Seems to imply that the guest _must_ have support for multiqueue and
> use exactly two queues.
> 
> What about using 'config' prefix?
> 
> config-multi-queue-max-queues=2
> config-feature-persistent=0
> 
>> has "request" in it. But if "request" is still preferred as a prefix I can change
>> it up.
>>
>> The scheme proposed is quite simple:
>>
>> * The directory "require" is created (inside the backend path) and within that
>> directory the features/capabilities names and values are written.
> 
> I'm quite sure I'm missing something, but what's the point in having
> this require directory in the xenstore backend path?
> 
> AFAICT you should just write this directly to the backend directory,
> and backends should be modified to check if there's a value already
> present before writing the default one.
> 
>> * Toolstack constructs a key value store of features, and user specifies those
>> through special entry names prefixed also as "require". Toolstack is stateless thus sys
>> admin has full control over what to pass to the backend. In other words it
>> doesn't look at particular feature names/values.
>>
>> * The backend will then use that for seeding its maximum feature set to the
>> frontend.
> 
> Oh, I see. So the backend picks up the suggested config from this
> directory together with it's capabilities and then produces the final
> set of features exposed to the frontend.
> 
> In order to prevent adding more logic to the backends, would it make
> sense to export the backend capabilities in /sys/ (or sysctl on BSDs)
> and do those calculations from the toolstack itself, so that the
> toolstack directly writes the features to the backend top level
> xenstore directory?

So you want the toolstack to read the /sys/ entries? How would that work
for driver domains?

Juergen

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

Re: [PATCH RFC 0/8] libxl, xl, public/io: PV backends feature control

[Roger Pau Monné]

On Wed, Feb 07, 2018 at 12:20:42PM +0100, Juergen Gross wrote:
> On 07/02/18 12:16, Roger Pau Monné wrote:
> > On Thu, Nov 02, 2017 at 06:06:08PM +0000, Joao Martins wrote:
> >> * Toolstack constructs a key value store of features, and user specifies those
> >> through special entry names prefixed also as "require". Toolstack is stateless thus sys
> >> admin has full control over what to pass to the backend. In other words it
> >> doesn't look at particular feature names/values.
> >>
> >> * The backend will then use that for seeding its maximum feature set to the
> >> frontend.
> > 
> > Oh, I see. So the backend picks up the suggested config from this
> > directory together with it's capabilities and then produces the final
> > set of features exposed to the frontend.
> > 
> > In order to prevent adding more logic to the backends, would it make
> > sense to export the backend capabilities in /sys/ (or sysctl on BSDs)
> > and do those calculations from the toolstack itself, so that the
> > toolstack directly writes the features to the backend top level
> > xenstore directory?
> 
> So you want the toolstack to read the /sys/ entries? How would that work
> for driver domains?

Right, that won't work for driver domains... And feeding that
information from a driver domain into the control domain is even
worse, so I'm fine with this.

Roger.

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

Re: [PATCH RFC 1/8] public/io/blkif: add directory for backend parameters

[Roger Pau Monné]

On Thu, Nov 02, 2017 at 06:06:09PM +0000, Joao Martins wrote:
> The proposed directory provides a mechanism for tools to control the
> maximum feature set of the device being provisioned by backends.
> Examples include max ring page order, persistent grants, number of
> queues etc.
> 
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
>  xen/include/public/io/blkif.h | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h
> index 15a71e3fea..4c0a93a2bf 100644
> --- a/xen/include/public/io/blkif.h
> +++ b/xen/include/public/io/blkif.h
> @@ -133,6 +133,20 @@
>   *      This option doesn't require a backend to use O_DIRECT, so it
>   *      should not be used to try to control the caching behaviour.
>   *
> + * require

I would maybe name this 'config'.

> + *
> + *      The directory "require" maybe be created by tools domain to
> + *      override the maximum feature set that backend provides to the
> + *      frontend. The children entries within this directory are
> + *      features names and its correspondent value e.g.:
                                  ^ corresponding

Thanks, Roger.

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

Re: [PATCH RFC 0/8] libxl, xl, public/io: PV backends feature control

[Joao Martins]

On 02/07/2018 11:30 AM, Roger Pau Monné wrote:
> On Wed, Feb 07, 2018 at 12:20:42PM +0100, Juergen Gross wrote:
>> On 07/02/18 12:16, Roger Pau Monné wrote:
>>> On Thu, Nov 02, 2017 at 06:06:08PM +0000, Joao Martins wrote:
>>>> * Toolstack constructs a key value store of features, and user specifies those
>>>> through special entry names prefixed also as "require". Toolstack is stateless thus sys
>>>> admin has full control over what to pass to the backend. In other words it
>>>> doesn't look at particular feature names/values.
>>>>
>>>> * The backend will then use that for seeding its maximum feature set to the
>>>> frontend.
>>>
>>> Oh, I see. So the backend picks up the suggested config from this
>>> directory together with it's capabilities and then produces the final
>>> set of features exposed to the frontend.
>>>
>>> In order to prevent adding more logic to the backends, would it make
>>> sense to export the backend capabilities in /sys/ (or sysctl on BSDs)
>>> and do those calculations from the toolstack itself, so that the
>>> toolstack directly writes the features to the backend top level
>>> xenstore directory?
>>
>> So you want the toolstack to read the /sys/ entries? How would that work
>> for driver domains?
> 
> Right, that won't work for driver domains... And feeding that
> information from a driver domain into the control domain is even
> worse, so I'm fine with this.

Right, in the first email thread[0] we had, Konrad also pointed this out about
FreeBSD and driver domains. So the choice of xenstore over OS specific
constructs are because of driver domains basically.

Joao

[0] https://lists.xen.org/archives/html/xen-devel/2017-09/msg02275.html

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

Re: [PATCH RFC 0/8] libxl, xl, public/io: PV backends feature control

[Joao Martins]

On 02/07/2018 11:30 AM, Roger Pau Monné wrote:
> On Wed, Feb 07, 2018 at 12:20:42PM +0100, Juergen Gross wrote:
>> On 07/02/18 12:16, Roger Pau Monné wrote:
>>> On Thu, Nov 02, 2017 at 06:06:08PM +0000, Joao Martins wrote:
>>>> * Toolstack constructs a key value store of features, and user specifies those
>>>> through special entry names prefixed also as "require". Toolstack is stateless thus sys
>>>> admin has full control over what to pass to the backend. In other words it
>>>> doesn't look at particular feature names/values.
>>>>
>>>> * The backend will then use that for seeding its maximum feature set to the
>>>> frontend.
>>>
>>> Oh, I see. So the backend picks up the suggested config from this
>>> directory together with it's capabilities and then produces the final
>>> set of features exposed to the frontend.
>>>
>>> In order to prevent adding more logic to the backends, would it make
>>> sense to export the backend capabilities in /sys/ (or sysctl on BSDs)
>>> and do those calculations from the toolstack itself, so that the
>>> toolstack directly writes the features to the backend top level
>>> xenstore directory?
>>
>> So you want the toolstack to read the /sys/ entries? How would that work
>> for driver domains?
> 
> Right, that won't work for driver domains... And feeding that
> information from a driver domain into the control domain is even
> worse, so I'm fine with this.

Right, in the first email thread[0] we had, Konrad also pointed this out about
driver domains. So the choice of xenstore over OS specific constructs was mainly
because of driver domains.

Joao

[0] https://lists.xen.org/archives/html/xen-devel/2017-09/msg02275.html

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

Re: [PATCH RFC 0/8] libxl, xl, public/io: PV backends feature control

[Joao Martins]

On 02/07/2018 11:16 AM, Roger Pau Monné wrote:
> On Thu, Nov 02, 2017 at 06:06:08PM +0000, Joao Martins wrote:
>> Hey folks,
>>
>> Presented herewith is an attempt to implement PV backends feature control
>> as discussed in the list (https://lists.xen.org/archives/html/xen-devel/2017-09/msg00766.html)
>>
>> Given that this a simple proposal hence I thought to include all changes
>> involved in the same patchset such that everyone see all the changes and has a
>> better estimate (but restricted to xen-devel just for the RFC purposes).
>>
>> The motivation here is to allow system administrators more fine grained
>> control of the device features being used by guest.
>>
>> The only change I made compared to the proposed discussed above was to use
>> "require" instead of "request" as the prefix because there is a feature which
> 
> require in the above context, like:
> 
> require-multi-queue-max-queues=2
> 
> Seems to imply that the guest _must_ have support for multiqueue and
> use exactly two queues.
> 
> What about using 'config' prefix?
> 
> config-multi-queue-max-queues=2
> config-feature-persistent=0
>

Hmm, 'config' sounds better indeed. We mainly chose 'require' because the domain
shall only be booted with those configs. I am fine with both.

>> has "request" in it. But if "request" is still preferred as a prefix I can change
>> it up.
>>
>> The scheme proposed is quite simple:
>>
>> * The directory "require" is created (inside the backend path) and within that
>> directory the features/capabilities names and values are written.
> 
> I'm quite sure I'm missing something, but what's the point in having
> this require directory in the xenstore backend path?
> 
> AFAICT you should just write this directly to the backend directory,
> and backends should be modified to check if there's a value already
> present before writing the default one.
> It's also an option which I can go with if folks prefer it.

Creating a config/require directory for requested features simply sounded
cleaner to me, and would also allow you to know what config you passed on to the
backend vs the one the backend exposed (better separation). And writing over the
currently reserved entries would be a little confusing when dealing with new
features (e.g. you write multi-queue-max-queues on a non multi queue backend and
the entry being present is a little misleading even though you would restrict
its access with backend-only permissions).

>> * Toolstack constructs a key value store of features, and user specifies those
>> through special entry names prefixed also as "require". Toolstack is stateless thus sys
>> admin has full control over what to pass to the backend. In other words it
>> doesn't look at particular feature names/values.
>>
>> * The backend will then use that for seeding its maximum feature set to the
>> frontend.
> 
> Oh, I see. So the backend picks up the suggested config from this
> directory together with it's capabilities and then produces the final
> set of features exposed to the frontend.
> 
/nods

> In order to prevent adding more logic to the backends, would it make
> sense to export the backend capabilities in /sys/ (or sysctl on BSDs)
> and do those calculations from the toolstack itself, so that the
> toolstack directly writes the features to the backend top level
> xenstore directory?

I had suggested in answer to Paul's comment[0] to create this maximum featureset
of the backend in its top level directory. Pasting it here that part (with one
addition):

/local/domain/X/backend/<backend_type>/features/<feature-name>-desc = "Short
description
of <feature-name>"
/local/domain/X/backend/<backend_type>/features/<feature-name>-defval =
"<something>"
/local/domain/X/backend/<backend_type>/features/<feature-name>-type =
"uint|int|string|bool" (but could be done with regexp instead of this entry)

e.g.

/local/domain/X/backend/vif/features/multi-queue-max-queues-desc = "Number of
queues of the interface"
/local/domain/X/backend/vif/features/multi-queue-max-queues-defval = "8"
/local/domain/X/backend/vif/features/multi-queue-max-queues-type = "uint"

Just wondering about the handling of these that would complicate the backend
implementation (e.g. types, error checking). But I am not seeing another good
way of doing this.

Joao

[0] https://lists.xen.org/archives/html/xen-devel/2017-11/msg00347.html

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

Re: [PATCH RFC 7/8] xen-blkback: frontend feature control

[Roger Pau Monné]

On Thu, Nov 02, 2017 at 06:06:15PM +0000, Joao Martins wrote:
> Toolstack may write values to the "require" subdirectory in the
> backend main directory (e.g. backend/vbd/X/Y/). Read these values
> and use them when announcing those to the frontend. When backend
> scans frontend features the values set in the require directory
> take precedence, hence making no significant changes in feature
> parsing.
> 
> xenbus_read_feature() reads from require subdirectory and prints that
> value and otherwise writing a default_val in the entry. We then replace
> all instances of xenbus_printf to use these previously seeded features.
> A backend_features struct is introduced and all values set there are
> used in place of the module parameters being used.
> 
> Note, however that feature-barrier, feature-flush-support and
> feature-discard aren't probed because first two are physical
> device dependent and feature-discard already has tunables to
> adjust.
> 
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
>  drivers/block/xen-blkback/blkback.c |  2 +-
>  drivers/block/xen-blkback/common.h  |  1 +
>  drivers/block/xen-blkback/xenbus.c  | 66 ++++++++++++++++++++++++++++++++-----
>  3 files changed, 60 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
> index c90e933330b6..05b3f124c871 100644
> --- a/drivers/block/xen-blkback/blkback.c
> +++ b/drivers/block/xen-blkback/blkback.c
> @@ -1271,7 +1271,7 @@ static int dispatch_rw_block_io(struct xen_blkif_ring *ring,
>  	    unlikely((req->operation != BLKIF_OP_INDIRECT) &&
>  		     (nseg > BLKIF_MAX_SEGMENTS_PER_REQUEST)) ||
>  	    unlikely((req->operation == BLKIF_OP_INDIRECT) &&
> -		     (nseg > MAX_INDIRECT_SEGMENTS))) {
> +		     (nseg > ring->blkif->vbd.max_indirect_segs))) {
>  		pr_debug("Bad number of segments in request (%d)\n", nseg);
>  		/* Haven't submitted any bio's yet. */
>  		goto fail_response;
> diff --git a/drivers/block/xen-blkback/common.h b/drivers/block/xen-blkback/common.h
> index a7832428e0da..ff12f2d883b9 100644
> --- a/drivers/block/xen-blkback/common.h
> +++ b/drivers/block/xen-blkback/common.h
> @@ -229,6 +229,7 @@ struct xen_vbd {
>  	unsigned int		discard_secure:1;
>  	unsigned int		feature_gnt_persistent:1;
>  	unsigned int		overflow_max_grants:1;
> +	unsigned int		max_indirect_segs;

Please place this above, in order to get less padding between fields.

>  };
>  
>  struct backend_info;
> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
> index 48d796ea3626..31683f29d5fb 100644
> --- a/drivers/block/xen-blkback/xenbus.c
> +++ b/drivers/block/xen-blkback/xenbus.c
> @@ -25,11 +25,19 @@
>  
>  /* On the XenBus the max length of 'ring-ref%u'. */
>  #define RINGREF_NAME_LEN (20)
> +#define REQUIRE_PATH_LEN (256)
> +
> +struct backend_features {
> +	unsigned max_queues;
> +	unsigned max_ring_order;

unsigned int.

> +	unsigned pers_grants;

bool?

Since you are already doing this, is it worth adding support to
disable other features like flush or discard?

> +};
>  
>  struct backend_info {
>  	struct xenbus_device	*dev;
>  	struct xen_blkif	*blkif;
>  	struct xenbus_watch	backend_watch;
> +	struct backend_features features;
>  	unsigned		major;
>  	unsigned		minor;
>  	char			*mode;
> @@ -602,6 +610,40 @@ int xen_blkbk_barrier(struct xenbus_transaction xbt,
>  	return err;
>  }
>  
> +static int xenbus_read_feature(const char *dir, const char *node,
> +			       unsigned int default_val)
> +{
> +	char reqnode[REQUIRE_PATH_LEN];
> +	unsigned int val;
> +
> +	snprintf(reqnode, REQUIRE_PATH_LEN, "%s/require", dir);
> +	val = xenbus_read_unsigned(reqnode, node, default_val);
> +	return val;
> +}

You could avoid the temporary buffer by doing something like:

> +
> +static void xen_blkbk_probe_features(struct xenbus_device *dev,
> +				     struct backend_info *be)
> +{
> +	struct backend_features *ft = &be->features;
> +	struct xen_vbd *vbd = &be->blkif->vbd;
> +

#define READ_FEAT(path, feat, default) \
	xenbus_read_unsigned(path, "require/" node, default)

> +	vbd->max_indirect_segs = xenbus_read_feature(dev->nodename,
> +						"feature-max-indirect-segments",
> +						MAX_INDIRECT_SEGMENTS);
> +
> +	ft->max_queues = xenbus_read_feature(dev->nodename,
> +					     "multi-queue-max-queues",
> +					     xenblk_max_queues);
> +
> +	ft->max_ring_order = xenbus_read_feature(dev->nodename,
> +						 "max-ring-page-order",
> +						 xen_blkif_max_ring_order);
> +
> +	ft->pers_grants = xenbus_read_feature(dev->nodename,
> +					      "feature-persistent",
> +					      1);
#undef READ_FEAT

Aren't you missing some check here or elsewhere to make sure the
proposed values don't exceed the maximum ones supported by blback?

I would expect something like:

vbd->max_indirect_segs = min(vbd->max_indirect_segs, MAX_INDIRECT_SEGMENTS);

And the same with max_queues and max_ring_oder.

It might also be a good idea to print some message when the proposed
value by the toolstack is not supported by the backend.

Thanks, Roger.

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

Re: [PATCH RFC 2/8] public/io/netif: add directory for backend parameters

[Joao Martins]

On 02/06/2018 05:12 PM, Wei Liu wrote:
> (Three months after you sent this, sorry...)
> 
> On Mon, Nov 06, 2017 at 12:33:06PM +0000, Joao Martins wrote:
>> On Mon, Nov 06, 2017 at 10:33:59AM +0000, Paul Durrant wrote:
>>>> -----Original Message-----
>>>> From: Joao Martins [mailto:joao.m.martins@oracle.com]
>>>> Sent: 02 November 2017 18:06
>>>> To: Xen Development List <xen-devel@lists.xen.org>
>>>> Cc: Joao Martins <joao.m.martins@oracle.com>; Konrad Rzeszutek Wilk
>>>> <konrad.wilk@oracle.com>; Paul Durrant <Paul.Durrant@citrix.com>; Wei Liu
>>>> <wei.liu2@citrix.com>
>>>> Subject: [PATCH RFC 2/8] public/io/netif: add directory for backend
>>>> parameters
>>>>
>>>> The proposed directory provides a mechanism for tools to control the
>>>> maximum feature set of the device being provisioned by backend.
>>>> The parameters/features include offloading features, number of
>>>> queues etc.
>>>>
>>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>>>> ---
>>>>  xen/include/public/io/netif.h | 16 ++++++++++++++++
>>>>  1 file changed, 16 insertions(+)
>>>>
>>>> diff --git a/xen/include/public/io/netif.h b/xen/include/public/io/netif.h
>>>> index 2454448baa..a412e4771d 100644
>>>> --- a/xen/include/public/io/netif.h
>>>> +++ b/xen/include/public/io/netif.h
>>>> @@ -161,6 +161,22 @@
>>>>   */
>>>>
>>>>  /*
>>>> + * The directory "require" maybe be created in backend path by tools
>>>> + * domain to override the maximum feature set that backend provides to
>>>> the
>>>> + * frontend. The children entries within this directory are features names
>>>> + * and the correspondent values that should be used backend as defaults
>>>> e.g.:
>>>> + *
>>>> + * /local/domain/X/backend/<domid>/<handle>/require
>>>> + * /local/domain/X/backend/<domid>/<handle>/require/multi-queue-
>>>> max-queues = "2"
>>>> + * /local/domain/X/backend/<domid>/<handle>/require/feature-no-csum-
>>>> offload = "1"
>>>> + *
>>>> + * In the example above, network backend will negotiate up to a maximum
>>>> of
>>>> + * two queues with frontend plus disabling IPv4 checksum offloading.
>>>> + *
>>>> + * This directory and its children entries shall only be visible to the backend.
>>>> + */
>>>> +
>>>
>>> What should happen if the toolstack sets something in 'require' that
>>> the backend cannot provide? I don't see anything in your RFC patches
>>> to check that the backend has responded appropriately to the keys.
>>
>> Hmm, you're right that this RFC doesn't handle that properly - but for the
>> ones the backend provide I had suggested (albeit not implemented here)
>> back in the other thread that we could compare the values of feature in
>> "require" with the one announced to the frontend. But well this wouldn't
>> cover the non-provided ones, and possibly would fall a bit as a hack.
>>
>> I could change the format of the entries within "require"
>> directory to be e.g. "<id>-<feature-name> = <feature-value>" and the
>> acknowledgement entry would come in the form "<id>-status
>> = <error code>". Consequently the lack of a "<id>-status" entry would
>> have a stronger semantic i.e. unsupported and ignored. The toolstack then would have
>> means to check whether the feature was really succesfull set as desired
>> or not. But then one question comes to mind: should the backend be
>> prevented to init in the event that the features requested fail to be
>> set? In which case uevent (on Linux) isn't triggered and xenbus state doesn't
>> get changed and toolstack would fail with timeout later on.
>>
> 
> I think the backend should not proceed if it can't meet the
> requirements. But to be clear I also don't think the timeout behaviour
> should be used to determine if the setting is successful because it is
> asking other part of the system to pick up the slack and system
> administrators would be left in the dark (unless there is easily
> accessible message that can be obtained by libxl to return to system
> administrators).

That timeout behaviour is already there *I think* (or maybe I have the wrong
impression)? The alternative is to trigger the uevent and add more logic on the
hotplug script to check if the parameters were set according to config, but OTOH
you add more complexity there. Or perhaps we can check that the backend set to
its state to Unknown (or some other state) and that determines the failure - but
still no uevent should be triggered. Unless you had something else in your mind?

	Joao

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

Re: [PATCH RFC 7/8] xen-blkback: frontend feature control

[Joao Martins]

On 02/07/2018 12:08 PM, Roger Pau Monné wrote:
> On Thu, Nov 02, 2017 at 06:06:15PM +0000, Joao Martins wrote:
>> Toolstack may write values to the "require" subdirectory in the
>> backend main directory (e.g. backend/vbd/X/Y/). Read these values
>> and use them when announcing those to the frontend. When backend
>> scans frontend features the values set in the require directory
>> take precedence, hence making no significant changes in feature
>> parsing.
>>
>> xenbus_read_feature() reads from require subdirectory and prints that
>> value and otherwise writing a default_val in the entry. We then replace
>> all instances of xenbus_printf to use these previously seeded features.
>> A backend_features struct is introduced and all values set there are
>> used in place of the module parameters being used.
>>
>> Note, however that feature-barrier, feature-flush-support and
>> feature-discard aren't probed because first two are physical
>> device dependent and feature-discard already has tunables to
>> adjust.
>>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> ---
>>  drivers/block/xen-blkback/blkback.c |  2 +-
>>  drivers/block/xen-blkback/common.h  |  1 +
>>  drivers/block/xen-blkback/xenbus.c  | 66 ++++++++++++++++++++++++++++++++-----
>>  3 files changed, 60 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
>> index c90e933330b6..05b3f124c871 100644
>> --- a/drivers/block/xen-blkback/blkback.c
>> +++ b/drivers/block/xen-blkback/blkback.c
>> @@ -1271,7 +1271,7 @@ static int dispatch_rw_block_io(struct xen_blkif_ring *ring,
>>  	    unlikely((req->operation != BLKIF_OP_INDIRECT) &&
>>  		     (nseg > BLKIF_MAX_SEGMENTS_PER_REQUEST)) ||
>>  	    unlikely((req->operation == BLKIF_OP_INDIRECT) &&
>> -		     (nseg > MAX_INDIRECT_SEGMENTS))) {
>> +		     (nseg > ring->blkif->vbd.max_indirect_segs))) {
>>  		pr_debug("Bad number of segments in request (%d)\n", nseg);
>>  		/* Haven't submitted any bio's yet. */
>>  		goto fail_response;
>> diff --git a/drivers/block/xen-blkback/common.h b/drivers/block/xen-blkback/common.h
>> index a7832428e0da..ff12f2d883b9 100644
>> --- a/drivers/block/xen-blkback/common.h
>> +++ b/drivers/block/xen-blkback/common.h
>> @@ -229,6 +229,7 @@ struct xen_vbd {
>>  	unsigned int		discard_secure:1;
>>  	unsigned int		feature_gnt_persistent:1;
>>  	unsigned int		overflow_max_grants:1;
>> +	unsigned int		max_indirect_segs;
> 
> Please place this above, in order to get less padding between fields.
> 
/nods

>>  };
>>  
>>  struct backend_info;
>> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
>> index 48d796ea3626..31683f29d5fb 100644
>> --- a/drivers/block/xen-blkback/xenbus.c
>> +++ b/drivers/block/xen-blkback/xenbus.c
>> @@ -25,11 +25,19 @@
>>  
>>  /* On the XenBus the max length of 'ring-ref%u'. */
>>  #define RINGREF_NAME_LEN (20)
>> +#define REQUIRE_PATH_LEN (256)
>> +
>> +struct backend_features {
>> +	unsigned max_queues;
>> +	unsigned max_ring_order;
> 
> unsigned int.
> 
>> +	unsigned pers_grants;
> 
> bool?
> 
> Since you are already doing this, is it worth adding support to
> disable other features like flush or discard?
>
Hmm, possibly. But I didn't really know if you folks wanted discard because it
already has a tunable? I guess probably yes given libxl is stateless, it could
be good for behaviour consistency. flush/barrier depended on whether the queue
had it enabled or not so I left it out thinking there was some other way to
mangle these features.

>> +};
>>  
>>  struct backend_info {
>>  	struct xenbus_device	*dev;
>>  	struct xen_blkif	*blkif;
>>  	struct xenbus_watch	backend_watch;
>> +	struct backend_features features;
>>  	unsigned		major;
>>  	unsigned		minor;
>>  	char			*mode;
>> @@ -602,6 +610,40 @@ int xen_blkbk_barrier(struct xenbus_transaction xbt,
>>  	return err;
>>  }
>>  
>> +static int xenbus_read_feature(const char *dir, const char *node,
>> +			       unsigned int default_val)
>> +{
>> +	char reqnode[REQUIRE_PATH_LEN];
>> +	unsigned int val;
>> +
>> +	snprintf(reqnode, REQUIRE_PATH_LEN, "%s/require", dir);
>> +	val = xenbus_read_unsigned(reqnode, node, default_val);
>> +	return val;
>> +}
> 
> You could avoid the temporary buffer by doing something like:
> 
>> +
>> +static void xen_blkbk_probe_features(struct xenbus_device *dev,
>> +				     struct backend_info *be)
>> +{
>> +	struct backend_features *ft = &be->features;
>> +	struct xen_vbd *vbd = &be->blkif->vbd;
>> +
> 
> #define READ_FEAT(path, feat, default) \
> 	xenbus_read_unsigned(path, "require/" node, default)
> 
>> +	vbd->max_indirect_segs = xenbus_read_feature(dev->nodename,
>> +						"feature-max-indirect-segments",
>> +						MAX_INDIRECT_SEGMENTS);
>> +
>> +	ft->max_queues = xenbus_read_feature(dev->nodename,
>> +					     "multi-queue-max-queues",
>> +					     xenblk_max_queues);
>> +
>> +	ft->max_ring_order = xenbus_read_feature(dev->nodename,
>> +						 "max-ring-page-order",
>> +						 xen_blkif_max_ring_order);
>> +
>> +	ft->pers_grants = xenbus_read_feature(dev->nodename,
>> +					      "feature-persistent",
>> +					      1);
> #undef READ_FEAT
> 
> Aren't you missing some check here or elsewhere to make sure the
> proposed values don't exceed the maximum ones supported by blback?
> 
> I would expect something like:
> 
> vbd->max_indirect_segs = min(vbd->max_indirect_segs, MAX_INDIRECT_SEGMENTS);
> 
> And the same with max_queues and max_ring_oder.
> 

This was deliberate to some extent. How do we define the value of max_queues?
Relying on the module parameters seems wrong as those *also* represent default
values for all guests. So I wouldn't cap to xen_blkbk_max_queues because it
would be left out to toolstack to pick. indirect_segs and max_ring_order there
are actual maximum values defined in macros so those definitely make sense to
check/validate.

> It might also be a good idea to print some message when the proposed
> value by the toolstack is not supported by the backend.

Yeap, I agree.

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

Re: [PATCH RFC 7/8] xen-blkback: frontend feature control

[Roger Pau Monné]

On Wed, Feb 07, 2018 at 02:16:14PM +0000, Joao Martins wrote:
> On 02/07/2018 12:08 PM, Roger Pau Monné wrote:
> > On Thu, Nov 02, 2017 at 06:06:15PM +0000, Joao Martins wrote:
> >> +static void xen_blkbk_probe_features(struct xenbus_device *dev,
> >> +				     struct backend_info *be)
> >> +{
> >> +	struct backend_features *ft = &be->features;
> >> +	struct xen_vbd *vbd = &be->blkif->vbd;
> >> +
> > 
> > #define READ_FEAT(path, feat, default) \
> > 	xenbus_read_unsigned(path, "require/" node, default)
> > 
> >> +	vbd->max_indirect_segs = xenbus_read_feature(dev->nodename,
> >> +						"feature-max-indirect-segments",
> >> +						MAX_INDIRECT_SEGMENTS);
> >> +
> >> +	ft->max_queues = xenbus_read_feature(dev->nodename,
> >> +					     "multi-queue-max-queues",
> >> +					     xenblk_max_queues);
> >> +
> >> +	ft->max_ring_order = xenbus_read_feature(dev->nodename,
> >> +						 "max-ring-page-order",
> >> +						 xen_blkif_max_ring_order);
> >> +
> >> +	ft->pers_grants = xenbus_read_feature(dev->nodename,
> >> +					      "feature-persistent",
> >> +					      1);
> > #undef READ_FEAT
> > 
> > Aren't you missing some check here or elsewhere to make sure the
> > proposed values don't exceed the maximum ones supported by blback?
> > 
> > I would expect something like:
> > 
> > vbd->max_indirect_segs = min(vbd->max_indirect_segs, MAX_INDIRECT_SEGMENTS);
> > 
> > And the same with max_queues and max_ring_oder.
> > 
> 
> This was deliberate to some extent. How do we define the value of max_queues?
> Relying on the module parameters seems wrong as those *also* represent default
> values for all guests. So I wouldn't cap to xen_blkbk_max_queues because it
> would be left out to toolstack to pick. indirect_segs and max_ring_order there
> are actual maximum values defined in macros so those definitely make sense to
> check/validate.

Yes, the maximum number of indirect segments is ATM a compile time
value, so exposing something bigger to the guest will only end up in
failed requests.

Thanks, Roger.

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

Re: [PATCH RFC 2/8] public/io/netif: add directory for backend parameters

[Wei Liu]

On Wed, Feb 07, 2018 at 12:10:37PM +0000, Joao Martins wrote:
> On 02/06/2018 05:12 PM, Wei Liu wrote:
> > (Three months after you sent this, sorry...)
> > 
> > On Mon, Nov 06, 2017 at 12:33:06PM +0000, Joao Martins wrote:
> >> On Mon, Nov 06, 2017 at 10:33:59AM +0000, Paul Durrant wrote:
> >>>> -----Original Message-----
> >>>> From: Joao Martins [mailto:joao.m.martins@oracle.com]
> >>>> Sent: 02 November 2017 18:06
> >>>> To: Xen Development List <xen-devel@lists.xen.org>
> >>>> Cc: Joao Martins <joao.m.martins@oracle.com>; Konrad Rzeszutek Wilk
> >>>> <konrad.wilk@oracle.com>; Paul Durrant <Paul.Durrant@citrix.com>; Wei Liu
> >>>> <wei.liu2@citrix.com>
> >>>> Subject: [PATCH RFC 2/8] public/io/netif: add directory for backend
> >>>> parameters
> >>>>
> >>>> The proposed directory provides a mechanism for tools to control the
> >>>> maximum feature set of the device being provisioned by backend.
> >>>> The parameters/features include offloading features, number of
> >>>> queues etc.
> >>>>
> >>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> >>>> ---
> >>>>  xen/include/public/io/netif.h | 16 ++++++++++++++++
> >>>>  1 file changed, 16 insertions(+)
> >>>>
> >>>> diff --git a/xen/include/public/io/netif.h b/xen/include/public/io/netif.h
> >>>> index 2454448baa..a412e4771d 100644
> >>>> --- a/xen/include/public/io/netif.h
> >>>> +++ b/xen/include/public/io/netif.h
> >>>> @@ -161,6 +161,22 @@
> >>>>   */
> >>>>
> >>>>  /*
> >>>> + * The directory "require" maybe be created in backend path by tools
> >>>> + * domain to override the maximum feature set that backend provides to
> >>>> the
> >>>> + * frontend. The children entries within this directory are features names
> >>>> + * and the correspondent values that should be used backend as defaults
> >>>> e.g.:
> >>>> + *
> >>>> + * /local/domain/X/backend/<domid>/<handle>/require
> >>>> + * /local/domain/X/backend/<domid>/<handle>/require/multi-queue-
> >>>> max-queues = "2"
> >>>> + * /local/domain/X/backend/<domid>/<handle>/require/feature-no-csum-
> >>>> offload = "1"
> >>>> + *
> >>>> + * In the example above, network backend will negotiate up to a maximum
> >>>> of
> >>>> + * two queues with frontend plus disabling IPv4 checksum offloading.
> >>>> + *
> >>>> + * This directory and its children entries shall only be visible to the backend.
> >>>> + */
> >>>> +
> >>>
> >>> What should happen if the toolstack sets something in 'require' that
> >>> the backend cannot provide? I don't see anything in your RFC patches
> >>> to check that the backend has responded appropriately to the keys.
> >>
> >> Hmm, you're right that this RFC doesn't handle that properly - but for the
> >> ones the backend provide I had suggested (albeit not implemented here)
> >> back in the other thread that we could compare the values of feature in
> >> "require" with the one announced to the frontend. But well this wouldn't
> >> cover the non-provided ones, and possibly would fall a bit as a hack.
> >>
> >> I could change the format of the entries within "require"
> >> directory to be e.g. "<id>-<feature-name> = <feature-value>" and the
> >> acknowledgement entry would come in the form "<id>-status
> >> = <error code>". Consequently the lack of a "<id>-status" entry would
> >> have a stronger semantic i.e. unsupported and ignored. The toolstack then would have
> >> means to check whether the feature was really succesfull set as desired
> >> or not. But then one question comes to mind: should the backend be
> >> prevented to init in the event that the features requested fail to be
> >> set? In which case uevent (on Linux) isn't triggered and xenbus state doesn't
> >> get changed and toolstack would fail with timeout later on.
> >>
> > 
> > I think the backend should not proceed if it can't meet the
> > requirements. But to be clear I also don't think the timeout behaviour
> > should be used to determine if the setting is successful because it is
> > asking other part of the system to pick up the slack and system
> > administrators would be left in the dark (unless there is easily
> > accessible message that can be obtained by libxl to return to system
> > administrators).
> 
> That timeout behaviour is already there *I think* (or maybe I have the wrong
> impression)? The alternative is to trigger the uevent and add more logic on the

Yes, it is already there. Libxl will wait for the backend to change its
state for X seconds.

The difference now is the system administrators can potentially easily
trigger a timeout due to misconfiguration, while previously it is mostly
due to the module not getting loaded or some other failures that are not
the system administrators' fault.

> hotplug script to check if the parameters were set according to config, but OTOH
> you add more complexity there. Or perhaps we can check that the backend set to
> its state to Unknown (or some other state) and that determines the failure - but
> still no uevent should be triggered. Unless you had something else in your mind?
> 

On the other hand, I don't think adding uevent or whatever other logic
is a good idea and it is a bit risky to rely on the state of driver to
determine failure because we don't have a state machine that applies to
all drivers. We can probably specify a xenstore node in the spec to
return some error code and let libxl read it. With that model old tools
work the same (extra node ignored) but new tools can utilise the new
node. IIRC there could already be some node that can be utilised --
xenbus_dev_fatal writes message to xenstore, I think.

What do you think?

Wei.

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

Re: [PATCH RFC 2/8] public/io/netif: add directory for backend parameters

[Joao Martins]

On 02/08/2018 11:13 AM, Wei Liu wrote:
> On Wed, Feb 07, 2018 at 12:10:37PM +0000, Joao Martins wrote:
>> On 02/06/2018 05:12 PM, Wei Liu wrote:
>>> (Three months after you sent this, sorry...)
>>>
>>> On Mon, Nov 06, 2017 at 12:33:06PM +0000, Joao Martins wrote:
>>>> On Mon, Nov 06, 2017 at 10:33:59AM +0000, Paul Durrant wrote:
>>>>>> -----Original Message-----
>>>>>> From: Joao Martins [mailto:joao.m.martins@oracle.com]
>>>>>> Sent: 02 November 2017 18:06
>>>>>> To: Xen Development List <xen-devel@lists.xen.org>
>>>>>> Cc: Joao Martins <joao.m.martins@oracle.com>; Konrad Rzeszutek Wilk
>>>>>> <konrad.wilk@oracle.com>; Paul Durrant <Paul.Durrant@citrix.com>; Wei Liu
>>>>>> <wei.liu2@citrix.com>
>>>>>> Subject: [PATCH RFC 2/8] public/io/netif: add directory for backend
>>>>>> parameters
>>>>>>
>>>>>> The proposed directory provides a mechanism for tools to control the
>>>>>> maximum feature set of the device being provisioned by backend.
>>>>>> The parameters/features include offloading features, number of
>>>>>> queues etc.
>>>>>>
>>>>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>>>>>> ---
>>>>>>  xen/include/public/io/netif.h | 16 ++++++++++++++++
>>>>>>  1 file changed, 16 insertions(+)
>>>>>>
>>>>>> diff --git a/xen/include/public/io/netif.h b/xen/include/public/io/netif.h
>>>>>> index 2454448baa..a412e4771d 100644
>>>>>> --- a/xen/include/public/io/netif.h
>>>>>> +++ b/xen/include/public/io/netif.h
>>>>>> @@ -161,6 +161,22 @@
>>>>>>   */
>>>>>>
>>>>>>  /*
>>>>>> + * The directory "require" maybe be created in backend path by tools
>>>>>> + * domain to override the maximum feature set that backend provides to
>>>>>> the
>>>>>> + * frontend. The children entries within this directory are features names
>>>>>> + * and the correspondent values that should be used backend as defaults
>>>>>> e.g.:
>>>>>> + *
>>>>>> + * /local/domain/X/backend/<domid>/<handle>/require
>>>>>> + * /local/domain/X/backend/<domid>/<handle>/require/multi-queue-
>>>>>> max-queues = "2"
>>>>>> + * /local/domain/X/backend/<domid>/<handle>/require/feature-no-csum-
>>>>>> offload = "1"
>>>>>> + *
>>>>>> + * In the example above, network backend will negotiate up to a maximum
>>>>>> of
>>>>>> + * two queues with frontend plus disabling IPv4 checksum offloading.
>>>>>> + *
>>>>>> + * This directory and its children entries shall only be visible to the backend.
>>>>>> + */
>>>>>> +
>>>>>
>>>>> What should happen if the toolstack sets something in 'require' that
>>>>> the backend cannot provide? I don't see anything in your RFC patches
>>>>> to check that the backend has responded appropriately to the keys.
>>>>
>>>> Hmm, you're right that this RFC doesn't handle that properly - but for the
>>>> ones the backend provide I had suggested (albeit not implemented here)
>>>> back in the other thread that we could compare the values of feature in
>>>> "require" with the one announced to the frontend. But well this wouldn't
>>>> cover the non-provided ones, and possibly would fall a bit as a hack.
>>>>
>>>> I could change the format of the entries within "require"
>>>> directory to be e.g. "<id>-<feature-name> = <feature-value>" and the
>>>> acknowledgement entry would come in the form "<id>-status
>>>> = <error code>". Consequently the lack of a "<id>-status" entry would
>>>> have a stronger semantic i.e. unsupported and ignored. The toolstack then would have
>>>> means to check whether the feature was really succesfull set as desired
>>>> or not. But then one question comes to mind: should the backend be
>>>> prevented to init in the event that the features requested fail to be
>>>> set? In which case uevent (on Linux) isn't triggered and xenbus state doesn't
>>>> get changed and toolstack would fail with timeout later on.
>>>>
>>>
>>> I think the backend should not proceed if it can't meet the
>>> requirements. But to be clear I also don't think the timeout behaviour
>>> should be used to determine if the setting is successful because it is
>>> asking other part of the system to pick up the slack and system
>>> administrators would be left in the dark (unless there is easily
>>> accessible message that can be obtained by libxl to return to system
>>> administrators).
>>
>> That timeout behaviour is already there *I think* (or maybe I have the wrong
>> impression)? The alternative is to trigger the uevent and add more logic on the
> 
> Yes, it is already there. Libxl will wait for the backend to change its
> state for X seconds.
> 
> The difference now is the system administrators can potentially easily
> trigger a timeout due to misconfiguration, while previously it is mostly
> due to the module not getting loaded or some other failures that are not
> the system administrators' fault.
> 
/nods

>> hotplug script to check if the parameters were set according to config, but OTOH
>> you add more complexity there. Or perhaps we can check that the backend set to
>> its state to Unknown (or some other state) and that determines the failure - but
>> still no uevent should be triggered. Unless you had something else in your mind?
> 
> On the other hand, I don't think adding uevent or whatever other logic
> is a good idea and it is a bit risky to rely on the state of driver to
> determine failure because we don't have a state machine that applies to
> all drivers. 

Agreed.

> We can probably specify a xenstore node in the spec to
> return some error code and let libxl read it. With that model old tools
> work the same (extra node ignored) but new tools can utilise the new
> node. IIRC there could already be some node that can be utilised --
> xenbus_dev_fatal writes message to xenstore, I think.
> 

I almost forgot about xenbus_dev_fatal(). It writes to an "error" entry in the
backend|frontend path with the errno plus error message. But it also changes the
device xenbus state to XenbusClosed. Taking into consideration your earlier
comment you probably meant xenbus_dev_error() instead? netback does allow
Initialising state to be directly into Closing, but others might not be the same.

> What do you think?

I like the idea of having a similar "error" entry in the config|require
directory following the same pattern as mentioned in the last paragraph.

Something like:

<backend|frontend path>/config/error = "<errno> <message>"

I would imagine this could be wrapper in a xenbus_config_fatal().

I had suggested a slightly more complicated version of it in a old reply to Paul
(at top of this message) with:

<backend|frontend path>/require/<id>-<feature-name> = "<feature-value>"
<backend|frontend path>/require/<id>-status = "<error code>"

But taking morphing it with your comment could also be something like:

<backend|frontend path>/config/<feature-name> = "<feature-value>"
<backend|frontend path>/config/<feature-name>/error = "<errno> <message>"

But either this option or the config/error global one depend on whether people
here prefer to deliver all configuration errors at once, or one global
"config/error" which would give the first entry with an error. The latter though
could lead to the sysadmin having to recreate the domain multiple times to
see/handle all the errors.

	Joao

P.S. s/require/config

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

Re: [PATCH RFC 2/8] public/io/netif: add directory for backend parameters

[Wei Liu]

On Thu, Feb 08, 2018 at 01:51:28PM +0000, Joao Martins wrote:
> > We can probably specify a xenstore node in the spec to
> > return some error code and let libxl read it. With that model old tools
> > work the same (extra node ignored) but new tools can utilise the new
> > node. IIRC there could already be some node that can be utilised --
> > xenbus_dev_fatal writes message to xenstore, I think.
> > 
> 
> I almost forgot about xenbus_dev_fatal(). It writes to an "error" entry in the
> backend|frontend path with the errno plus error message. But it also changes the
> device xenbus state to XenbusClosed. Taking into consideration your earlier
> comment you probably meant xenbus_dev_error() instead? netback does allow
> Initialising state to be directly into Closing, but others might not be the same.
> 

Yep, xenbus_dev_error is probably better.

> > What do you think?
> 
> I like the idea of having a similar "error" entry in the config|require
> directory following the same pattern as mentioned in the last paragraph.
> 
> Something like:
> 
> <backend|frontend path>/config/error = "<errno> <message>"
> 
> I would imagine this could be wrapper in a xenbus_config_fatal().
> 
> I had suggested a slightly more complicated version of it in a old reply to Paul
> (at top of this message) with:
> 
> <backend|frontend path>/require/<id>-<feature-name> = "<feature-value>"
> <backend|frontend path>/require/<id>-status = "<error code>"
> 
> But taking morphing it with your comment could also be something like:
> 
> <backend|frontend path>/config/<feature-name> = "<feature-value>"
> <backend|frontend path>/config/<feature-name>/error = "<errno> <message>"
> 
> But either this option or the config/error global one depend on whether people
> here prefer to deliver all configuration errors at once, or one global
> "config/error" which would give the first entry with an error. The latter though
> could lead to the sysadmin having to recreate the domain multiple times to
> see/handle all the errors.
> 

I'm not too opinionated on this really. I just want things to be
properly documented, cover known use cases well and allow for future
extensions.

Wei.

> 	Joao
> 
> P.S. s/require/config

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