All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v7 0/7]  xl / libxl: named PCI pass-through devices
@ 2021-01-05 17:46 Paul Durrant
  2021-01-05 17:46 ` [PATCH v7 1/7] docs/man: modify xl(1) in preparation for naming of assignable devices Paul Durrant
                   ` (7 more replies)
  0 siblings, 8 replies; 13+ messages in thread
From: Paul Durrant @ 2021-01-05 17:46 UTC (permalink / raw)
  To: xen-devel; +Cc: Paul Durrant

From: Paul Durrant <pdurrant@amazon.com>

These are re-worked versions of the remaining patches from v6 of the series
that were reverted by commit ac6a0af3870b "Revert patches that break libxl
API".

Paul Durrant (7):
  docs/man: modify xl(1) in preparation for naming of assignable devices
  libxlu: introduce xlu_pci_parse_spec_string()
  libxl: stop setting 'vdevfn' in pci_struct_fill()
  libxl: add 'name' field to 'libxl_device_pci' in the IDL...
  xl: support naming of assignable devices
  docs/man: modify xl-pci-configuration(5) to add 'name' field to
    PCI_SPEC_STRING
  libxl / libxlu: support 'xl pci-attach/detach' by name

 docs/man/xl-pci-configuration.5.pod |  25 ++-
 docs/man/xl.1.pod.in                |  19 +-
 tools/include/libxl.h               |   6 +
 tools/include/libxlutil.h           |   8 +-
 tools/libs/light/libxl_pci.c        | 133 ++++++++++++--
 tools/libs/light/libxl_types.idl    |  13 +-
 tools/libs/util/libxlu_pci.c        | 353 +++++++++++++++++++-----------------
 tools/xl/xl_cmdtable.c              |  16 +-
 tools/xl/xl_parse.c                 |   4 +-
 tools/xl/xl_pci.c                   | 120 ++++++++----
 10 files changed, 457 insertions(+), 240 deletions(-)

-- 
2.11.0



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

* [PATCH v7 1/7] docs/man: modify xl(1) in preparation for naming of assignable devices
  2021-01-05 17:46 [PATCH v7 0/7] xl / libxl: named PCI pass-through devices Paul Durrant
@ 2021-01-05 17:46 ` Paul Durrant
  2021-01-05 17:46 ` [PATCH v7 2/7] libxlu: introduce xlu_pci_parse_spec_string() Paul Durrant
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Paul Durrant @ 2021-01-05 17:46 UTC (permalink / raw)
  To: xen-devel; +Cc: Paul Durrant, Ian Jackson

From: Paul Durrant <pdurrant@amazon.com>

A subsequent patch will introduce code to allow a name to be specified to
'xl pci-assignable-add' such that the assignable device may be referred to
by than name in subsequent operations.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Acked-by: Wei Liu <wl@xen.org>
---
Cc: Ian Jackson <iwj@xenproject.org>
---
 docs/man/xl.1.pod.in | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/docs/man/xl.1.pod.in b/docs/man/xl.1.pod.in
index af31d2b572..f4779d8fd6 100644
--- a/docs/man/xl.1.pod.in
+++ b/docs/man/xl.1.pod.in
@@ -1595,19 +1595,23 @@ List virtual network interfaces for a domain.
 
 =over 4
 
-=item B<pci-assignable-list>
+=item B<pci-assignable-list> [I<-n>]
 
 List all the B<BDF> of assignable PCI devices. See
-L<xl-pci-configuration(5)> for more information.
+L<xl-pci-configuration(5)> for more information. If the -n option is
+specified then any name supplied when the device was made assignable
+will also be displayed.
 
 These are devices in the system which are configured to be
 available for passthrough and are bound to a suitable PCI
 backend driver in domain 0 rather than a real driver.
 
-=item B<pci-assignable-add> I<BDF>
+=item B<pci-assignable-add> [I<-n NAME>] I<BDF>
 
 Make the device at B<BDF> assignable to guests. See
-L<xl-pci-configuration(5)> for more information.
+L<xl-pci-configuration(5)> for more information. If the -n option is
+supplied then the assignable device entry will the named with the
+given B<NAME>.
 
 This will bind the device to the pciback driver and assign it to the
 "quarantine domain".  If it is already bound to a driver, it will
@@ -1622,10 +1626,11 @@ not to do this on a device critical to domain 0's operation, such as
 storage controllers, network interfaces, or GPUs that are currently
 being used.
 
-=item B<pci-assignable-remove> [I<-r>] I<BDF>
+=item B<pci-assignable-remove> [I<-r>] I<BDF>|I<NAME>
 
-Make the device at B<BDF> not assignable to guests. See
-L<xl-pci-configuration(5)> for more information.
+Make a device non-assignable to guests. The device may be identified
+either by its B<BDF> or the B<NAME> supplied when the device was made
+assignable. See L<xl-pci-configuration(5)> for more information.
 
 This will at least unbind the device from pciback, and
 re-assign it from the "quarantine domain" back to domain 0.  If the -r
-- 
2.11.0



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

* [PATCH v7 2/7] libxlu: introduce xlu_pci_parse_spec_string()
  2021-01-05 17:46 [PATCH v7 0/7] xl / libxl: named PCI pass-through devices Paul Durrant
  2021-01-05 17:46 ` [PATCH v7 1/7] docs/man: modify xl(1) in preparation for naming of assignable devices Paul Durrant
@ 2021-01-05 17:46 ` Paul Durrant
  2021-01-05 17:46 ` [PATCH v7 3/7] libxl: stop setting 'vdevfn' in pci_struct_fill() Paul Durrant
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Paul Durrant @ 2021-01-05 17:46 UTC (permalink / raw)
  To: xen-devel; +Cc: Paul Durrant, Ian Jackson, Anthony PERARD

From: Paul Durrant <pdurrant@amazon.com>

This patch largely re-writes the code to parse a PCI_SPEC_STRING and enters
it via the newly introduced function. The new parser also deals with 'bdf'
and 'vslot' as non-positional paramaters, as per the documentation in
xl-pci-configuration(5).

The existing xlu_pci_parse_bdf() function remains, but now strictly parses
BDF values. Some existing callers of xlu_pci_parse_bdf() are
modified to call xlu_pci_parse_spec_string() as per the documentation in xl(1).

NOTE: Usage text in xl_cmdtable.c and error messages are also modified
      appropriately.
      As a side-effect this patch also fixes a bug where using '*' to specify
      all functions would lead to an assertion failure at the end of
      xlu_pci_parse_bdf().

Fixes: d25cc3ec93eb ("libxl: workaround gcc 10.2 maybe-uninitialized warning")
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Acked-by: Wei Liu <wl@xen.org>
---
Cc: Ian Jackson <iwj@xenproject.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>

v7:
 - Minor modifications now that the 'libxl_pci_bdf' type no longer exists
 - Modifications are largely mechanical so Wei's A-b is retained
---
 tools/include/libxlutil.h    |   8 +-
 tools/libs/util/libxlu_pci.c | 348 +++++++++++++++++++++++--------------------
 tools/xl/xl_cmdtable.c       |   4 +-
 tools/xl/xl_parse.c          |   4 +-
 tools/xl/xl_pci.c            |  33 ++--
 5 files changed, 213 insertions(+), 184 deletions(-)

diff --git a/tools/include/libxlutil.h b/tools/include/libxlutil.h
index 92e35c5462..4dd3c5e92b 100644
--- a/tools/include/libxlutil.h
+++ b/tools/include/libxlutil.h
@@ -109,9 +109,15 @@ int xlu_disk_parse(XLU_Config *cfg, int nspecs, const char *const *specs,
    */
 
 /*
+ * PCI BDF
+ */
+int xlu_pci_parse_bdf(XLU_Config *cfg, libxl_device_pci *pci, const char *str);
+
+/*
  * PCI specification parsing
  */
-int xlu_pci_parse_bdf(XLU_Config *cfg, libxl_device_pci *pcidev, const char *str);
+int xlu_pci_parse_spec_string(XLU_Config *cfg, libxl_device_pci *pci,
+                              const char *str);
 
 /*
  * RDM parsing
diff --git a/tools/libs/util/libxlu_pci.c b/tools/libs/util/libxlu_pci.c
index 1d38fffce3..05472a0bd1 100644
--- a/tools/libs/util/libxlu_pci.c
+++ b/tools/libs/util/libxlu_pci.c
@@ -1,5 +1,7 @@
 #define _GNU_SOURCE
 
+#include <ctype.h>
+
 #include "libxlu_internal.h"
 #include "libxlu_disk_l.h"
 #include "libxlu_disk_i.h"
@@ -9,185 +11,209 @@
 #define XLU__PCI_ERR(_c, _x, _a...) \
     if((_c) && (_c)->report) fprintf((_c)->report, _x, ##_a)
 
-static int hex_convert(const char *str, unsigned int *val, unsigned int mask)
+static int parse_bdf(libxl_device_pci *pci, const char *str, const char **endp)
 {
-    unsigned long ret;
-    char *end;
-
-    ret = strtoul(str, &end, 16);
-    if ( end == str || *end != '\0' )
-        return -1;
-    if ( ret & ~mask )
-        return -1;
-    *val = (unsigned int)ret & mask;
-    return 0;
-}
+    const char *ptr = str;
+    unsigned int colons = 0;
+    unsigned int domain, bus, dev, func;
+    int n;
+
+    /* Count occurrences of ':' to detrmine presence/absence of the 'domain' */
+    while (isxdigit(*ptr) || *ptr == ':') {
+        if (*ptr == ':')
+            colons++;
+        ptr++;
+    }
+
+    ptr = str;
+    switch (colons) {
+    case 1:
+        domain = 0;
+        if (sscanf(ptr, "%x:%x.%n", &bus, &dev, &n) != 2)
+            return ERROR_INVAL;
+        break;
+    case 2:
+        if (sscanf(ptr, "%x:%x:%x.%n", &domain, &bus, &dev, &n) != 3)
+            return ERROR_INVAL;
+        break;
+    default:
+        return ERROR_INVAL;
+    }
+
+    if (domain > 0xffff || bus > 0xff || dev > 0x1f)
+        return ERROR_INVAL;
+
+    ptr += n;
+    if (*ptr == '*') {
+        pci->vfunc_mask = LIBXL_PCI_FUNC_ALL;
+        func = 0;
+        ptr++;
+    } else {
+        if (sscanf(ptr, "%x%n", &func, &n) != 1)
+            return ERROR_INVAL;
+        if (func > 7)
+            return ERROR_INVAL;
+        pci->vfunc_mask = 1;
+        ptr += n;
+    }
 
-static int pci_struct_fill(libxl_device_pci *pci, unsigned int domain,
-                           unsigned int bus, unsigned int dev,
-                           unsigned int func, unsigned int vdevfn)
-{
     pci->domain = domain;
     pci->bus = bus;
     pci->dev = dev;
     pci->func = func;
-    pci->vdevfn = vdevfn;
+
+    if (endp)
+        *endp = ptr;
+
     return 0;
 }
 
-#define STATE_DOMAIN    0
-#define STATE_BUS       1
-#define STATE_DEV       2
-#define STATE_FUNC      3
-#define STATE_VSLOT     4
-#define STATE_OPTIONS_K 6
-#define STATE_OPTIONS_V 7
-#define STATE_TERMINAL  8
-#define STATE_TYPE      9
-#define STATE_RDM_STRATEGY      10
-#define STATE_RESERVE_POLICY    11
-#define INVALID         0xffffffff
-int xlu_pci_parse_bdf(XLU_Config *cfg, libxl_device_pci *pci, const char *str)
+static int parse_vslot(uint32_t *vdevfnp, const char *str, const char **endp)
 {
-    unsigned state = STATE_DOMAIN;
-    unsigned dom = INVALID, bus = INVALID, dev = INVALID, func = INVALID, vslot = 0;
-    char *buf2, *tok, *ptr, *end, *optkey = NULL;
+    const char *ptr = str;
+    unsigned int val;
+    int n;
+
+    if (sscanf(ptr, "%x%n", &val, &n) != 1)
+        return ERROR_INVAL;
+
+    if (val > 0x1f)
+        return ERROR_INVAL;
+
+    ptr += n;
+
+    *vdevfnp = val << 3;
+
+    if (endp)
+        *endp = ptr;
+
+    return 0;
+}
+
+static int parse_key_val(char **keyp, char**valp, const char *str,
+                         const char **endp)
+{
+    const char *ptr = str;
+    char *key, *val;
+
+    while (*ptr != '=' && *ptr != '\0')
+        ptr++;
+
+    if (*ptr == '\0')
+        return ERROR_INVAL;
 
-    if ( NULL == (buf2 = ptr = strdup(str)) )
+    key = strndup(str, ptr - str);
+    if (!key)
         return ERROR_NOMEM;
 
-    for(tok = ptr, end = ptr + strlen(ptr) + 1; ptr < end; ptr++) {
-        switch(state) {
-        case STATE_DOMAIN:
-            if ( *ptr == ':' ) {
-                state = STATE_BUS;
-                *ptr = '\0';
-                if ( hex_convert(tok, &dom, 0xffff) )
-                    goto parse_error;
-                tok = ptr + 1;
-            }
-            break;
-        case STATE_BUS:
-            if ( *ptr == ':' ) {
-                state = STATE_DEV;
-                *ptr = '\0';
-                if ( hex_convert(tok, &bus, 0xff) )
-                    goto parse_error;
-                tok = ptr + 1;
-            }else if ( *ptr == '.' ) {
-                state = STATE_FUNC;
-                *ptr = '\0';
-                if ( dom & ~0xff )
-                    goto parse_error;
-                bus = dom;
-                dom = 0;
-                if ( hex_convert(tok, &dev, 0xff) )
-                    goto parse_error;
-                tok = ptr + 1;
-            }
-            break;
-        case STATE_DEV:
-            if ( *ptr == '.' ) {
-                state = STATE_FUNC;
-                *ptr = '\0';
-                if ( hex_convert(tok, &dev, 0xff) )
-                    goto parse_error;
-                tok = ptr + 1;
-            }
-            break;
-        case STATE_FUNC:
-            if ( *ptr == '\0' || *ptr == '@' || *ptr == ',' ) {
-                switch( *ptr ) {
-                case '\0':
-                    state = STATE_TERMINAL;
-                    break;
-                case '@':
-                    state = STATE_VSLOT;
-                    break;
-                case ',':
-                    state = STATE_OPTIONS_K;
-                    break;
-                }
-                *ptr = '\0';
-                if ( !strcmp(tok, "*") ) {
-                    pci->vfunc_mask = LIBXL_PCI_FUNC_ALL;
-                }else{
-                    if ( hex_convert(tok, &func, 0x7) )
-                        goto parse_error;
-                    pci->vfunc_mask = (1 << 0);
-                }
-                tok = ptr + 1;
-            }
-            break;
-        case STATE_VSLOT:
-            if ( *ptr == '\0' || *ptr == ',' ) {
-                state = ( *ptr == ',' ) ? STATE_OPTIONS_K : STATE_TERMINAL;
-                *ptr = '\0';
-                if ( hex_convert(tok, &vslot, 0xff) )
-                    goto parse_error;
-                tok = ptr + 1;
-            }
-            break;
-        case STATE_OPTIONS_K:
-            if ( *ptr == '=' ) {
-                state = STATE_OPTIONS_V;
-                *ptr = '\0';
-                optkey = tok;
-                tok = ptr + 1;
-            }
-            break;
-        case STATE_OPTIONS_V:
-            if ( *ptr == ',' || *ptr == '\0' ) {
-                state = (*ptr == ',') ? STATE_OPTIONS_K : STATE_TERMINAL;
-                *ptr = '\0';
-                if ( !strcmp(optkey, "msitranslate") ) {
-                    pci->msitranslate = atoi(tok);
-                }else if ( !strcmp(optkey, "power_mgmt") ) {
-                    pci->power_mgmt = atoi(tok);
-                }else if ( !strcmp(optkey, "permissive") ) {
-                    pci->permissive = atoi(tok);
-                }else if ( !strcmp(optkey, "seize") ) {
-                    pci->seize = atoi(tok);
-                } else if (!strcmp(optkey, "rdm_policy")) {
-                    if (!strcmp(tok, "strict")) {
-                        pci->rdm_policy = LIBXL_RDM_RESERVE_POLICY_STRICT;
-                    } else if (!strcmp(tok, "relaxed")) {
-                        pci->rdm_policy = LIBXL_RDM_RESERVE_POLICY_RELAXED;
-                    } else {
-                        XLU__PCI_ERR(cfg, "%s is not an valid PCI RDM property"
-                                          " policy: 'strict' or 'relaxed'.",
-                                     tok);
-                        goto parse_error;
-                    }
-                } else {
-                    XLU__PCI_ERR(cfg, "Unknown PCI BDF option: %s", optkey);
-                }
-                tok = ptr + 1;
-            }
-        default:
-            break;
+    str = ++ptr; /* skip '=' */
+    while (*ptr != ',' && *ptr != '\0')
+        ptr++;
+
+    val = strndup(str, ptr - str);
+    if (!val) {
+        free(key);
+        return ERROR_NOMEM;
+    }
+
+    if (*ptr == ',')
+        ptr++;
+
+    *keyp = key;
+    *valp = val;
+    *endp = ptr;
+
+    return 0;
+}
+
+static int parse_rdm_policy(XLU_Config *cfg, libxl_rdm_reserve_policy *policy,
+                            const char *str)
+{
+    int ret = libxl_rdm_reserve_policy_from_string(str, policy);
+
+    if (ret)
+        XLU__PCI_ERR(cfg, "Unknown RDM policy: %s", str);
+
+    return ret;
+}
+
+int xlu_pci_parse_bdf(XLU_Config *cfg, libxl_device_pci *pci, const char *str)
+{
+    return parse_bdf(pci, str, NULL);
+}
+
+int xlu_pci_parse_spec_string(XLU_Config *cfg, libxl_device_pci *pci,
+                              const char *str)
+{
+    const char *ptr = str;
+    bool bdf_present = false;
+    int ret;
+
+    /* Attempt to parse 'bdf' as positional parameter */
+    ret = parse_bdf(pci, ptr, &ptr);
+    if (!ret) {
+        bdf_present = true;
+
+        /* Check whether 'vslot' if present */
+        if (*ptr == '@') {
+            ret = parse_vslot(&pci->vdevfn, ++ptr, &ptr);
+            if (ret)
+                return ret;
         }
+        if (*ptr == ',')
+            ptr++;
+        else if (*ptr != '\0')
+            return ERROR_INVAL;
     }
 
-    if ( tok != ptr || state != STATE_TERMINAL )
-        goto parse_error;
+    /* Parse the rest as 'key=val' pairs */
+    while (*ptr != '\0') {
+        char *key, *val;
 
-    assert(dom != INVALID && bus != INVALID && dev != INVALID && func != INVALID);
+        ret = parse_key_val(&key, &val, ptr, &ptr);
+        if (ret)
+            return ret;
 
-    /* Just a pretty way to fill in the values */
-    pci_struct_fill(pci, dom, bus, dev, func, vslot << 3);
+        if (!strcmp(key, "bdf")) {
+            ret = parse_bdf(pci, val, NULL);
+            if (!ret) bdf_present = true;
+        } else if (!strcmp(key, "vslot")) {
+            ret = parse_vslot(&pci->vdevfn, val, NULL);
+        } else if (!strcmp(key, "permissive")) {
+            pci->permissive = atoi(val);
+        } else if (!strcmp(key, "msitranslate")) {
+            pci->msitranslate = atoi(val);
+        } else if (!strcmp(key, "seize")) {
+            pci->seize= atoi(val);
+        } else if (!strcmp(key, "power_mgmt")) {
+            pci->power_mgmt = atoi(val);
+        } else if (!strcmp(key, "rdm_policy")) {
+            ret = parse_rdm_policy(cfg, &pci->rdm_policy, val);
+        } else {
+            XLU__PCI_ERR(cfg, "Unknown PCI_SPEC_STRING option: %s", key);
+            ret = ERROR_INVAL;
+        }
 
-    free(buf2);
+        free(key);
+        free(val);
 
-    return 0;
+        if (ret)
+            return ret;
+    }
 
-parse_error:
-    free(buf2);
-    return ERROR_INVAL;
+    if (!bdf_present)
+        return ERROR_INVAL;
+
+    return 0;
 }
 
 int xlu_rdm_parse(XLU_Config *cfg, libxl_rdm_reserve *rdm, const char *str)
 {
+#define STATE_TYPE           0
+#define STATE_RDM_STRATEGY   1
+#define STATE_RESERVE_POLICY 2
+#define STATE_TERMINAL       3
+
     unsigned state = STATE_TYPE;
     char *buf2, *tok, *ptr, *end;
 
@@ -227,15 +253,8 @@ int xlu_rdm_parse(XLU_Config *cfg, libxl_rdm_reserve *rdm, const char *str)
             if (*ptr == ',' || *ptr == '\0') {
                 state = *ptr == ',' ? STATE_TYPE : STATE_TERMINAL;
                 *ptr = '\0';
-                if (!strcmp(tok, "strict")) {
-                    rdm->policy = LIBXL_RDM_RESERVE_POLICY_STRICT;
-                } else if (!strcmp(tok, "relaxed")) {
-                    rdm->policy = LIBXL_RDM_RESERVE_POLICY_RELAXED;
-                } else {
-                    XLU__PCI_ERR(cfg, "Unknown RDM property policy value: %s",
-                                 tok);
+                if (!parse_rdm_policy(cfg, &rdm->policy, tok))
                     goto parse_error;
-                }
                 tok = ptr + 1;
             }
         default:
@@ -253,6 +272,11 @@ int xlu_rdm_parse(XLU_Config *cfg, libxl_rdm_reserve *rdm, const char *str)
 parse_error:
     free(buf2);
     return ERROR_INVAL;
+
+#undef STATE_TYPE
+#undef STATE_RDM_STRATEGY
+#undef STATE_RESERVE_POLICY
+#undef STATE_TERMINAL
 }
 
 /*
diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
index 6ab5e47da3..30e17a2848 100644
--- a/tools/xl/xl_cmdtable.c
+++ b/tools/xl/xl_cmdtable.c
@@ -90,12 +90,12 @@ struct cmd_spec cmd_table[] = {
     { "pci-attach",
       &main_pciattach, 0, 1,
       "Insert a new pass-through pci device",
-      "<Domain> <BDF> [Virtual Slot]",
+      "<Domain> <PCI_SPEC_STRING>",
     },
     { "pci-detach",
       &main_pcidetach, 0, 1,
       "Remove a domain's pass-through pci device",
-      "<Domain> <BDF>",
+      "<Domain> <PCI_SPEC_STRING>",
     },
     { "pci-list",
       &main_pcilist, 0, 0,
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 4ebf39620a..867e4d068a 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -1487,10 +1487,10 @@ void parse_config_data(const char *config_source,
              * the global policy by default.
              */
             pci->rdm_policy = b_info->u.hvm.rdm.policy;
-            e = xlu_pci_parse_bdf(config, pci, buf);
+            e = xlu_pci_parse_spec_string(config, pci, buf);
             if (e) {
                 fprintf(stderr,
-                        "unable to parse PCI BDF `%s' for passthrough\n",
+                        "unable to parse PCI_SPEC_STRING `%s' for passthrough\n",
                         buf);
                 exit(-e);
             }
diff --git a/tools/xl/xl_pci.c b/tools/xl/xl_pci.c
index f71498cbb5..9a66494bb5 100644
--- a/tools/xl/xl_pci.c
+++ b/tools/xl/xl_pci.c
@@ -54,7 +54,7 @@ int main_pcilist(int argc, char **argv)
     return 0;
 }
 
-static int pcidetach(uint32_t domid, const char *bdf, int force)
+static int pcidetach(uint32_t domid, const char *spec_string, int force)
 {
     libxl_device_pci pci;
     XLU_Config *config;
@@ -65,8 +65,9 @@ static int pcidetach(uint32_t domid, const char *bdf, int force)
     config = xlu_cfg_init(stderr, "command line");
     if (!config) { perror("xlu_cfg_inig"); exit(-1); }
 
-    if (xlu_pci_parse_bdf(config, &pci, bdf)) {
-        fprintf(stderr, "pci-detach: malformed BDF specification \"%s\"\n", bdf);
+    if (xlu_pci_parse_spec_string(config, &pci, spec_string)) {
+        fprintf(stderr, "pci-detach: malformed PCI_SPEC_STRING \"%s\"\n",
+                spec_string);
         exit(2);
     }
     if (force) {
@@ -88,7 +89,7 @@ int main_pcidetach(int argc, char **argv)
     uint32_t domid;
     int opt;
     int force = 0;
-    const char *bdf = NULL;
+    const char *spec_string = NULL;
 
     SWITCH_FOREACH_OPT(opt, "f", NULL, "pci-detach", 2) {
     case 'f':
@@ -97,15 +98,15 @@ int main_pcidetach(int argc, char **argv)
     }
 
     domid = find_domain(argv[optind]);
-    bdf = argv[optind + 1];
+    spec_string = argv[optind + 1];
 
-    if (pcidetach(domid, bdf, force))
+    if (pcidetach(domid, spec_string, force))
         return EXIT_FAILURE;
 
     return EXIT_SUCCESS;
 }
 
-static int pciattach(uint32_t domid, const char *bdf, const char *vs)
+static int pciattach(uint32_t domid, const char *spec_string)
 {
     libxl_device_pci pci;
     XLU_Config *config;
@@ -116,8 +117,9 @@ static int pciattach(uint32_t domid, const char *bdf, const char *vs)
     config = xlu_cfg_init(stderr, "command line");
     if (!config) { perror("xlu_cfg_inig"); exit(-1); }
 
-    if (xlu_pci_parse_bdf(config, &pci, bdf)) {
-        fprintf(stderr, "pci-attach: malformed BDF specification \"%s\"\n", bdf);
+    if (xlu_pci_parse_spec_string(config, &pci, spec_string)) {
+        fprintf(stderr, "pci-attach: malformed PCI_SPEC_STRING \"%s\"\n",
+                spec_string);
         exit(2);
     }
 
@@ -134,19 +136,16 @@ int main_pciattach(int argc, char **argv)
 {
     uint32_t domid;
     int opt;
-    const char *bdf = NULL, *vs = NULL;
+    const char *spec_string = NULL;
 
     SWITCH_FOREACH_OPT(opt, "", NULL, "pci-attach", 2) {
         /* No options */
     }
 
     domid = find_domain(argv[optind]);
-    bdf = argv[optind + 1];
-
-    if (optind + 1 < argc)
-        vs = argv[optind + 2];
+    spec_string = argv[optind + 1];
 
-    if (pciattach(domid, bdf, vs))
+    if (pciattach(domid, spec_string))
         return EXIT_FAILURE;
 
     return EXIT_SUCCESS;
@@ -192,7 +191,7 @@ static int pciassignable_add(const char *bdf, int rebind)
     if (!config) { perror("xlu_cfg_init"); exit(-1); }
 
     if (xlu_pci_parse_bdf(config, &pci, bdf)) {
-        fprintf(stderr, "pci-assignable-add: malformed BDF specification \"%s\"\n", bdf);
+        fprintf(stderr, "pci-assignable-add: malformed BDF \"%s\"\n", bdf);
         exit(2);
     }
 
@@ -234,7 +233,7 @@ static int pciassignable_remove(const char *bdf, int rebind)
     if (!config) { perror("xlu_cfg_init"); exit(-1); }
 
     if (xlu_pci_parse_bdf(config, &pci, bdf)) {
-        fprintf(stderr, "pci-assignable-remove: malformed BDF specification \"%s\"\n", bdf);
+        fprintf(stderr, "pci-assignable-remove: malformed BDF \"%s\"\n", bdf);
         exit(2);
     }
 
-- 
2.11.0



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

* [PATCH v7 3/7] libxl: stop setting 'vdevfn' in pci_struct_fill()
  2021-01-05 17:46 [PATCH v7 0/7] xl / libxl: named PCI pass-through devices Paul Durrant
  2021-01-05 17:46 ` [PATCH v7 1/7] docs/man: modify xl(1) in preparation for naming of assignable devices Paul Durrant
  2021-01-05 17:46 ` [PATCH v7 2/7] libxlu: introduce xlu_pci_parse_spec_string() Paul Durrant
@ 2021-01-05 17:46 ` Paul Durrant
  2021-01-21 14:42   ` Wei Liu
  2021-01-05 17:46 ` [PATCH v7 4/7] libxl: add 'name' field to 'libxl_device_pci' in the IDL Paul Durrant
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 13+ messages in thread
From: Paul Durrant @ 2021-01-05 17:46 UTC (permalink / raw)
  To: xen-devel; +Cc: Paul Durrant, Ian Jackson, Wei Liu, Anthony PERARD

From: Paul Durrant <pdurrant@amazon.com>

There are only two call-sites. One always sets it to 0 (which is unnecessary
as the structure is already initialized to zero) and the other can simply set
the 'vdevfn' field directly (after proper structure initialization), avoiding
the need for a local variable.

A subsequent patch will also make use of pci_struct_fill() in a context
where 'vdevfn' may already have been set.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Ian Jackson <iwj@xenproject.org>
Cc: Wei Liu <wl@xen.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>

v7:
 - New in v7
---
 tools/libs/light/libxl_pci.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
index 74c2196ae3..6feedadc62 100644
--- a/tools/libs/light/libxl_pci.c
+++ b/tools/libs/light/libxl_pci.c
@@ -39,13 +39,12 @@ static unsigned int pci_encode_bdf(libxl_device_pci *pci)
 
 static void pci_struct_fill(libxl_device_pci *pci, unsigned int domain,
                             unsigned int bus, unsigned int dev,
-                            unsigned int func, unsigned int vdevfn)
+                            unsigned int func)
 {
     pci->domain = domain;
     pci->bus = bus;
     pci->dev = dev;
     pci->func = func;
-    pci->vdevfn = vdevfn;
 }
 
 static void libxl_create_pci_backend_device(libxl__gc *gc,
@@ -451,7 +450,7 @@ libxl_device_pci *libxl_device_pci_assignable_list(libxl_ctx *ctx, int *num)
         new = pcis + *num;
 
         libxl_device_pci_init(new);
-        pci_struct_fill(new, dom, bus, dev, func, 0);
+        pci_struct_fill(new, dom, bus, dev, func);
 
         if (pci_info_xs_read(gc, new, "domid")) /* already assigned */
             continue;
@@ -2288,17 +2287,19 @@ static int libxl__device_pci_from_xs_be(libxl__gc *gc,
                                         libxl_devid nr, void *data)
 {
     char *s;
-    unsigned int domain = 0, bus = 0, dev = 0, func = 0, vdevfn = 0;
+    unsigned int domain = 0, bus = 0, dev = 0, func = 0;
     libxl_device_pci *pci = data;
 
+    libxl_device_pci_init(pci);
+
     s = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/dev-%d", be_path, nr));
     sscanf(s, PCI_BDF, &domain, &bus, &dev, &func);
 
+    pci_struct_fill(pci, domain, bus, dev, func);
+
     s = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/vdevfn-%d", be_path, nr));
     if (s)
-        vdevfn = strtol(s, (char **) NULL, 16);
-
-    pci_struct_fill(pci, domain, bus, dev, func, vdevfn);
+        pci->vdevfn = strtol(s, (char **) NULL, 16);
 
     s = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/opts-%d", be_path, nr));
     if (s) {
-- 
2.11.0



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

* [PATCH v7 4/7] libxl: add 'name' field to 'libxl_device_pci' in the IDL...
  2021-01-05 17:46 [PATCH v7 0/7] xl / libxl: named PCI pass-through devices Paul Durrant
                   ` (2 preceding siblings ...)
  2021-01-05 17:46 ` [PATCH v7 3/7] libxl: stop setting 'vdevfn' in pci_struct_fill() Paul Durrant
@ 2021-01-05 17:46 ` Paul Durrant
  2021-01-21 14:45   ` Wei Liu
  2021-01-05 17:46 ` [PATCH v7 5/7] xl: support naming of assignable devices Paul Durrant
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 13+ messages in thread
From: Paul Durrant @ 2021-01-05 17:46 UTC (permalink / raw)
  To: xen-devel; +Cc: Paul Durrant, Ian Jackson, Wei Liu, Anthony PERARD

From: Paul Durrant <pdurrant@amazon.com>

... and modify libxl_pci_bdf_assignable_add/remove/list() to make use of it.

libxl_pci_bdf_assignable_add() will store the name of the device in xenstore
if the field is specified (i.e. non-NULL) and libxl_pci_bdf_assignable_remove()
will remove devices specified only by name, looking up the BDF as necessary.

libxl_pci_bdf_assignable_list() will also populate the 'name' field if a name
was stored by libxl_pci_bdf_assignable_add().

NOTE: This patch also fixes whitespace in the declaration of 'libxl_device_pci'
      in the IDL.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Ian Jackson <iwj@xenproject.org>
Cc: Wei Liu <wl@xen.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>

v7:
 - New in v7
---
 tools/include/libxl.h            |  6 ++++
 tools/libs/light/libxl_pci.c     | 77 ++++++++++++++++++++++++++++++++++++++--
 tools/libs/light/libxl_types.idl | 13 +++----
 3 files changed, 87 insertions(+), 9 deletions(-)

diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index 3433c950f9..3488fbf56f 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -464,6 +464,12 @@
 #define LIBXL_HAVE_DEVICE_PCI_ASSIGNABLE_LIST_FREE 1
 
 /*
+ * LIBXL_HAVE_DEVICE_PCI_NAME indicates that the libxl_device_pci type
+ * has a name field.
+ */
+#define LIBXL_HAVE_DEVICE_PCI_NAME 1
+
+/*
  * libxl ABI compatibility
  *
  * The only guarantee which libxl makes regarding ABI compatibility
diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
index 6feedadc62..9e3a90dcda 100644
--- a/tools/libs/light/libxl_pci.c
+++ b/tools/libs/light/libxl_pci.c
@@ -438,7 +438,9 @@ libxl_device_pci *libxl_device_pci_assignable_list(libxl_ctx *ctx, int *num)
     }
 
     while((de = readdir(dir))) {
-        unsigned dom, bus, dev, func;
+        unsigned int dom, bus, dev, func;
+        char *name;
+
         if (sscanf(de->d_name, PCI_BDF, &dom, &bus, &dev, &func) != 4)
             continue;
 
@@ -455,6 +457,9 @@ libxl_device_pci *libxl_device_pci_assignable_list(libxl_ctx *ctx, int *num)
         if (pci_info_xs_read(gc, new, "domid")) /* already assigned */
             continue;
 
+        name = pci_info_xs_read(gc, new, "name");
+        if (name) new->name = strdup(name);
+
         (*num)++;
     }
 
@@ -742,6 +747,7 @@ static int libxl__device_pci_assignable_add(libxl__gc *gc,
     libxl_ctx *ctx = libxl__gc_owner(gc);
     unsigned dom, bus, dev, func;
     char *spath, *driver_path = NULL;
+    const char *name;
     int rc;
     struct stat st;
 
@@ -750,6 +756,24 @@ static int libxl__device_pci_assignable_add(libxl__gc *gc,
     bus = pci->bus;
     dev = pci->dev;
     func = pci->func;
+    name = pci->name;
+
+    /* Sanitise any name that is set */
+    if (name) {
+        unsigned int i, n = strlen(name);
+
+        if (n > 64) { /* Reasonable upper bound on name length */
+            LOG(ERROR, "Name too long");
+            return ERROR_FAIL;
+        }
+
+        for (i = 0; i < n; i++) {
+            if (!isgraph(name[i])) {
+                LOG(ERROR, "Names may only include printable characters");
+                return ERROR_FAIL;
+            }
+        }
+    }
 
     /* See if the device exists */
     spath = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF, dom, bus, dev, func);
@@ -765,7 +789,7 @@ static int libxl__device_pci_assignable_add(libxl__gc *gc,
     }
     if ( rc ) {
         LOG(WARN, PCI_BDF" already assigned to pciback", dom, bus, dev, func);
-        goto quarantine;
+        goto name;
     }
 
     /* Check to see if there's already a driver that we need to unbind from */
@@ -796,7 +820,12 @@ static int libxl__device_pci_assignable_add(libxl__gc *gc,
         return ERROR_FAIL;
     }
 
-quarantine:
+name:
+    if (name)
+        pci_info_xs_write(gc, pci, "name", name);
+    else
+        pci_info_xs_remove(gc, pci, "name");
+
     /*
      * DOMID_IO is just a sentinel domain, without any actual mappings,
      * so always pass XEN_DOMCTL_DEV_RDM_RELAXED to avoid assignment being
@@ -812,6 +841,40 @@ quarantine:
     return 0;
 }
 
+static int name2bdf(libxl__gc *gc, libxl_device_pci *pci)
+{
+    char **bdfs;
+    unsigned int i, n;
+    int rc = ERROR_NOTFOUND;
+
+    bdfs = libxl__xs_directory(gc, XBT_NULL, PCI_INFO_PATH, &n);
+    if (!n)
+        goto out;
+
+    for (i = 0; i < n; i++) {
+        unsigned dom, bus, dev, func;
+        char *name;
+
+        if (sscanf(bdfs[i], PCI_BDF_XSPATH, &dom, &bus, &dev, &func) != 4)
+            continue;
+
+        pci_struct_fill(pci, dom, bus, dev, func);
+
+        name = pci_info_xs_read(gc, pci, "name");
+        if (name && !strcmp(name, pci->name)) {
+            rc = 0;
+            break;
+        }
+    }
+
+out:
+    if (!rc)
+        LOG(DETAIL, "'%s' -> " PCI_BDF, pci->name, pci->domain,
+            pci->bus, pci->dev, pci->func);
+
+    return rc;
+}
+
 static int libxl__device_pci_assignable_remove(libxl__gc *gc,
                                                libxl_device_pci *pci,
                                                int rebind)
@@ -820,6 +883,12 @@ static int libxl__device_pci_assignable_remove(libxl__gc *gc,
     int rc;
     char *driver_path;
 
+    /* If the device is named then we need to look up the BDF */
+    if (pci->name) {
+        rc = name2bdf(gc, pci);
+        if (rc) return rc;
+    }
+
     /* De-quarantine */
     rc = xc_deassign_device(ctx->xch, DOMID_IO, pci_encode_bdf(pci));
     if ( rc < 0 ) {
@@ -860,6 +929,8 @@ static int libxl__device_pci_assignable_remove(libxl__gc *gc,
         }
     }
 
+    pci_info_xs_remove(gc, pci, "name");
+
     return 0;
 }
 
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index 05324736b7..b4a9076b85 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -771,17 +771,18 @@ libxl_device_nic = Struct("device_nic", [
     ])
 
 libxl_device_pci = Struct("device_pci", [
-    ("func",      uint8),
-    ("dev",       uint8),
-    ("bus",       uint8),
-    ("domain",    integer),
-    ("vdevfn",    uint32),
+    ("func", uint8),
+    ("dev", uint8),
+    ("bus", uint8),
+    ("domain", integer),
+    ("vdevfn", uint32),
     ("vfunc_mask", uint32),
     ("msitranslate", bool),
     ("power_mgmt", bool),
     ("permissive", bool),
     ("seize", bool),
-    ("rdm_policy",      libxl_rdm_reserve_policy),
+    ("rdm_policy", libxl_rdm_reserve_policy),
+    ("name", string),
     ])
 
 libxl_device_rdm = Struct("device_rdm", [
-- 
2.11.0



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

* [PATCH v7 5/7] xl: support naming of assignable devices
  2021-01-05 17:46 [PATCH v7 0/7] xl / libxl: named PCI pass-through devices Paul Durrant
                   ` (3 preceding siblings ...)
  2021-01-05 17:46 ` [PATCH v7 4/7] libxl: add 'name' field to 'libxl_device_pci' in the IDL Paul Durrant
@ 2021-01-05 17:46 ` Paul Durrant
  2021-01-21 14:49   ` Wei Liu
  2021-01-05 17:46 ` [PATCH v7 6/7] docs/man: modify xl-pci-configuration(5) to add 'name' field to PCI_SPEC_STRING Paul Durrant
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 13+ messages in thread
From: Paul Durrant @ 2021-01-05 17:46 UTC (permalink / raw)
  To: xen-devel; +Cc: Paul Durrant, Ian Jackson, Wei Liu, Anthony PERARD

From: Paul Durrant <pdurrant@amazon.com>

With this patch applied 'xl pci-assignable-add' will take an optional '--name'
parameter, 'xl pci-assignable-remove' can be passed either a BDF or a name and
'xl pci-assignable-list' will take a optional '--show-names' flag which
determines whether names are displayed in its output.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Ian Jackson <iwj@xenproject.org>
Cc: Wei Liu <wl@xen.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>

v7:
 - Heavily re-worked, bearing only some resemblance to similarly named v6 patch
---
 tools/xl/xl_cmdtable.c | 12 ++++---
 tools/xl/xl_pci.c      | 89 ++++++++++++++++++++++++++++++++++++--------------
 2 files changed, 73 insertions(+), 28 deletions(-)

diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
index 30e17a2848..bd8af12ff3 100644
--- a/tools/xl/xl_cmdtable.c
+++ b/tools/xl/xl_cmdtable.c
@@ -105,21 +105,25 @@ struct cmd_spec cmd_table[] = {
     { "pci-assignable-add",
       &main_pciassignable_add, 0, 1,
       "Make a device assignable for pci-passthru",
-      "<BDF>",
+      "[options] <BDF>",
+      "-n NAME, --name=NAME    Name the assignable device.\n"
       "-h                      Print this help.\n"
     },
     { "pci-assignable-remove",
       &main_pciassignable_remove, 0, 1,
       "Remove a device from being assignable",
-      "[options] <BDF>",
+      "[options] <BDF>|NAME",
       "-h                      Print this help.\n"
       "-r                      Attempt to re-assign the device to the\n"
-      "                        original driver"
+      "                        original driver."
     },
     { "pci-assignable-list",
       &main_pciassignable_list, 0, 0,
       "List all the assignable pci devices",
-      "",
+      "[options]",
+      "-h                      Print this help.\n"
+      "-n, --show-names        Display assignable device names where\n"
+      "                        supplied.\n"
     },
     { "pause",
       &main_pause, 0, 1,
diff --git a/tools/xl/xl_pci.c b/tools/xl/xl_pci.c
index 9a66494bb5..b1c3ae2a72 100644
--- a/tools/xl/xl_pci.c
+++ b/tools/xl/xl_pci.c
@@ -151,7 +151,7 @@ int main_pciattach(int argc, char **argv)
     return EXIT_SUCCESS;
 }
 
-static void pciassignable_list(void)
+static void pciassignable_list(bool show_names)
 {
     libxl_device_pci *pcis;
     int num, i;
@@ -161,8 +161,12 @@ static void pciassignable_list(void)
     if ( pcis == NULL )
         return;
     for (i = 0; i < num; i++) {
-        printf("%04x:%02x:%02x.%01x\n",
-               pcis[i].domain, pcis[i].bus, pcis[i].dev, pcis[i].func);
+        libxl_device_pci *pci = &pcis[i];
+        char *name = show_names ? pci->name : NULL;
+
+        printf("%04x:%02x:%02x.%01x %s\n",
+               pci->domain, pci->bus, pci->dev, pci->func,
+               name ?: "");
     }
     libxl_device_pci_assignable_list_free(pcis, num);
 }
@@ -170,20 +174,27 @@ static void pciassignable_list(void)
 int main_pciassignable_list(int argc, char **argv)
 {
     int opt;
-
-    SWITCH_FOREACH_OPT(opt, "", NULL, "pci-assignable-list", 0) {
-        /* No options */
+    static struct option opts[] = {
+        {"show-names", 0, 0, 'n'},
+        COMMON_LONG_OPTS
+    };
+    bool show_names = false;
+
+    SWITCH_FOREACH_OPT(opt, "n", opts, "pci-assignable-list", 0) {
+    case 'n':
+        show_names = true;
+        break;
     }
 
-    pciassignable_list();
+    pciassignable_list(show_names);
     return 0;
 }
 
-static int pciassignable_add(const char *bdf, int rebind)
+static int pciassignable_add(const char *bdf, const char *name, int rebind)
 {
     libxl_device_pci pci;
     XLU_Config *config;
-    int r = 0;
+    int r;
 
     libxl_device_pci_init(&pci);
 
@@ -195,8 +206,15 @@ static int pciassignable_add(const char *bdf, int rebind)
         exit(2);
     }
 
-    if (libxl_device_pci_assignable_add(ctx, &pci, rebind))
-        r = 1;
+    if (name) {
+        pci.name = strdup(name);
+        if (!pci.name) {
+            fprintf(stderr, "pci-assignable-add: memory allocation failure\n");
+            exit(2);
+        }
+    }
+
+    r = libxl_device_pci_assignable_add(ctx, &pci, rebind);
 
     libxl_device_pci_dispose(&pci);
     xlu_cfg_destroy(config);
@@ -208,38 +226,61 @@ int main_pciassignable_add(int argc, char **argv)
 {
     int opt;
     const char *bdf = NULL;
-
-    SWITCH_FOREACH_OPT(opt, "", NULL, "pci-assignable-add", 1) {
-        /* No options */
+    static struct option opts[] = {
+        {"name", 1, 0, 'n'},
+        COMMON_LONG_OPTS
+    };
+    const char *name = NULL;
+
+    SWITCH_FOREACH_OPT(opt, "n:", opts, "pci-assignable-add", 1) {
+    case 'n':
+        name = optarg;
+        break;
     }
 
     bdf = argv[optind];
 
-    if (pciassignable_add(bdf, 1))
+    if (pciassignable_add(bdf, name, 1))
         return EXIT_FAILURE;
 
     return EXIT_SUCCESS;
 }
 
-static int pciassignable_remove(const char *bdf, int rebind)
+static int pciassignable_remove(const char *ident, int rebind)
 {
     libxl_device_pci pci;
     XLU_Config *config;
-    int r = 0;
+    int r;
 
     libxl_device_pci_init(&pci);
 
     config = xlu_cfg_init(stderr, "command line");
     if (!config) { perror("xlu_cfg_init"); exit(-1); }
 
-    if (xlu_pci_parse_bdf(config, &pci, bdf)) {
-        fprintf(stderr, "pci-assignable-remove: malformed BDF \"%s\"\n", bdf);
+    /* Try remove-by-name first */
+    pci.name = strdup(ident);
+    if (!pci.name) {
+        fprintf(stderr, "pci-assignable-add: memory allocation failure\n");
         exit(2);
     }
 
-    if (libxl_device_pci_assignable_remove(ctx, &pci, rebind))
-        r = 1;
+    r = libxl_device_pci_assignable_remove(ctx, &pci, rebind);
+    if (!r || r != ERROR_NOTFOUND)
+        goto out;
+
+    /* If remove-by-name failed to find the device, try remove-by-BDF */
+    free(pci.name);
+    pci.name = NULL;
 
+    if (xlu_pci_parse_bdf(config, &pci, ident)) {
+        fprintf(stderr,
+                "pci-assignable-remove: malformed BDF '%s'\n", ident);
+        exit(2);
+    }
+
+    r = libxl_device_pci_assignable_remove(ctx, &pci, rebind);
+
+out:
     libxl_device_pci_dispose(&pci);
     xlu_cfg_destroy(config);
 
@@ -249,7 +290,7 @@ static int pciassignable_remove(const char *bdf, int rebind)
 int main_pciassignable_remove(int argc, char **argv)
 {
     int opt;
-    const char *bdf = NULL;
+    const char *ident = NULL;
     int rebind = 0;
 
     SWITCH_FOREACH_OPT(opt, "r", NULL, "pci-assignable-remove", 1) {
@@ -258,9 +299,9 @@ int main_pciassignable_remove(int argc, char **argv)
         break;
     }
 
-    bdf = argv[optind];
+    ident = argv[optind];
 
-    if (pciassignable_remove(bdf, rebind))
+    if (pciassignable_remove(ident, rebind))
         return EXIT_FAILURE;
 
     return EXIT_SUCCESS;
-- 
2.11.0



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

* [PATCH v7 6/7] docs/man: modify xl-pci-configuration(5) to add 'name' field to PCI_SPEC_STRING
  2021-01-05 17:46 [PATCH v7 0/7] xl / libxl: named PCI pass-through devices Paul Durrant
                   ` (4 preceding siblings ...)
  2021-01-05 17:46 ` [PATCH v7 5/7] xl: support naming of assignable devices Paul Durrant
@ 2021-01-05 17:46 ` Paul Durrant
  2021-01-05 17:46 ` [PATCH v7 7/7] libxl / libxlu: support 'xl pci-attach/detach' by name Paul Durrant
  2021-01-13  8:05 ` [PATCH v7 0/7] xl / libxl: named PCI pass-through devices Oleksandr Andrushchenko
  7 siblings, 0 replies; 13+ messages in thread
From: Paul Durrant @ 2021-01-05 17:46 UTC (permalink / raw)
  To: xen-devel; +Cc: Paul Durrant, Ian Jackson

From: Paul Durrant <pdurrant@amazon.com>

Since assignable devices can be named, a subsequent patch will support use
of a PCI_SPEC_STRING containing a 'name' parameter instead of a 'bdf'. In
this case the name will be used to look up the 'bdf' in the list of assignable
(or assigned) devices.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Acked-by: Wei Liu <wl@xen.org>
---
Cc: Ian Jackson <iwj@xenproject.org>
---
 docs/man/xl-pci-configuration.5.pod | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/docs/man/xl-pci-configuration.5.pod b/docs/man/xl-pci-configuration.5.pod
index 4dd73bc498..db3360307c 100644
--- a/docs/man/xl-pci-configuration.5.pod
+++ b/docs/man/xl-pci-configuration.5.pod
@@ -51,7 +51,7 @@ is not specified, or if it is specified with an empty value (whether
 positionally or explicitly).
 
 B<NOTE>: In context of B<xl pci-detach> (see L<xl(1)>), parameters other than
-B<bdf> will be ignored.
+B<bdf> or B<name> will be ignored.
 
 =head1 Positional Parameters
 
@@ -70,7 +70,11 @@ B<*> to indicate all functions of a multi-function device.
 
 =item Default Value
 
-None. This parameter is mandatory as it identifies the device.
+None. This parameter is mandatory in its positional form. As a non-positional
+parameter it is also mandatory unless a B<name> parameter is present, in
+which case B<bdf> must not be present since the B<name> will be used to find
+the B<bdf> in the list of assignable devices. See L<xl(1)> for more information
+on naming assignable devices.
 
 =back
 
@@ -194,4 +198,21 @@ B<NOTE>: This overrides the global B<rdm> option.
 
 =back
 
+=item B<name>=I<STRING>
+
+=over 4
+
+=item Description
+
+This is the name given when the B<BDF> was made assignable. See L<xl(1)> for
+more information on naming assignable devices.
+
+=item Default Value
+
+None. This parameter must not be present if a B<bdf> parameter is present.
+If a B<bdf> parameter is not present then B<name> is mandatory as it is
+required to look up the B<BDF> in the list of assignable devices.
+
+=back
+
 =back
-- 
2.11.0



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

* [PATCH v7 7/7] libxl / libxlu: support 'xl pci-attach/detach' by name
  2021-01-05 17:46 [PATCH v7 0/7] xl / libxl: named PCI pass-through devices Paul Durrant
                   ` (5 preceding siblings ...)
  2021-01-05 17:46 ` [PATCH v7 6/7] docs/man: modify xl-pci-configuration(5) to add 'name' field to PCI_SPEC_STRING Paul Durrant
@ 2021-01-05 17:46 ` Paul Durrant
  2021-01-21 14:50   ` Wei Liu
  2021-01-13  8:05 ` [PATCH v7 0/7] xl / libxl: named PCI pass-through devices Oleksandr Andrushchenko
  7 siblings, 1 reply; 13+ messages in thread
From: Paul Durrant @ 2021-01-05 17:46 UTC (permalink / raw)
  To: xen-devel; +Cc: Paul Durrant, Wei Liu, Ian Jackson, Anthony PERARD

From: Paul Durrant <pdurrant@amazon.com>

This patch modifies libxlu_pci_parse_spec_string() to parse the new 'name'
parameter of PCI_SPEC_STRING detailed in the updated documention in
xl-pci-configuration(5) and populate the 'name' field of 'libxl_device_pci'.

If the 'name' field is non-NULL then both libxl_device_pci_add() and
libxl_device_pci_remove() will use it to look up the device BDF in
the list of assignable devices.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Wei Liu <wl@xen.org>
Cc: Ian Jackson <iwj@xenproject.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>

v7:
 - Re-worked (mainly because 'libxl_device_pci' already has a 'name' field)
 - Dropped Wei's A-b because of the re-work

v6:
 - Re-base
 - Slight modification to the patch name
 - Kept Wei's A-b since modifications are small
---
 tools/libs/light/libxl_pci.c | 43 ++++++++++++++++++++++++++++++++++++++-----
 tools/libs/util/libxlu_pci.c |  7 ++++++-
 2 files changed, 44 insertions(+), 6 deletions(-)

diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
index 9e3a90dcda..1a1c263080 100644
--- a/tools/libs/light/libxl_pci.c
+++ b/tools/libs/light/libxl_pci.c
@@ -58,6 +58,8 @@ static void libxl_create_pci_backend_device(libxl__gc *gc,
     flexarray_append(back, GCSPRINTF(PCI_BDF, pci->domain, pci->bus, pci->dev, pci->func));
     if (pci->vdevfn)
         flexarray_append_pair(back, GCSPRINTF("vdevfn-%d", num), GCSPRINTF("%x", pci->vdevfn));
+    if (pci->name)
+        flexarray_append_pair(back, GCSPRINTF("name-%d", num), GCSPRINTF("%s", pci->name));
     flexarray_append(back, GCSPRINTF("opts-%d", num));
     flexarray_append(back,
               GCSPRINTF("msitranslate=%d,power_mgmt=%d,permissive=%d,rdm_policy=%s",
@@ -282,6 +284,7 @@ retry_transaction2:
     xs_rm(ctx->xsh, t, GCSPRINTF("%s/vdev-%d", be_path, i));
     xs_rm(ctx->xsh, t, GCSPRINTF("%s/opts-%d", be_path, i));
     xs_rm(ctx->xsh, t, GCSPRINTF("%s/vdevfn-%d", be_path, i));
+    xs_rm(ctx->xsh, t, GCSPRINTF("%s/name-%d", be_path, i));
     libxl__xs_printf(gc, t, num_devs_path, "%d", num - 1);
     for (j = i + 1; j < num; j++) {
         tmppath = GCSPRINTF("%s/state-%d", be_path, j);
@@ -314,6 +317,12 @@ retry_transaction2:
             xs_write(ctx->xsh, t, GCSPRINTF("%s/vdevfn-%d", be_path, j - 1), tmp, strlen(tmp));
             xs_rm(ctx->xsh, t, tmppath);
         }
+        tmppath = GCSPRINTF("%s/name-%d", be_path, j);
+        tmp = libxl__xs_read(gc, t, tmppath);
+        if (tmp) {
+            xs_write(ctx->xsh, t, GCSPRINTF("%s/name-%d", be_path, j - 1), tmp, strlen(tmp));
+            xs_rm(ctx->xsh, t, tmppath);
+        }
     }
     if (!xs_transaction_end(ctx->xsh, t, 0))
         if (errno == EAGAIN)
@@ -1589,6 +1598,12 @@ void libxl__device_pci_add(libxl__egc *egc, uint32_t domid,
     libxl_device_pci_copy(CTX, &pas->pci, pci);
     pci = &pas->pci;
 
+    /* If the device is named then we need to look up the BDF */
+    if (pci->name) {
+        rc = name2bdf(gc, pci);
+        if (rc) goto out;
+    }
+
     pas->starting = starting;
     pas->callback = device_pci_add_stubdom_done;
 
@@ -1739,11 +1754,19 @@ static void device_pci_add_done(libxl__egc *egc,
     libxl_device_pci *pci = &pas->pci;
 
     if (rc) {
-        LOGD(ERROR, domid,
-             "libxl__device_pci_add  failed for "
-             "PCI device %x:%x:%x.%x (rc %d)",
-             pci->domain, pci->bus, pci->dev, pci->func,
-             rc);
+        if (pci->name) {
+            LOGD(ERROR, domid,
+                 "libxl__device_pci_add failed for "
+                 "PCI device '%s' (rc %d)",
+                 pci->name,
+                 rc);
+        } else {
+            LOGD(ERROR, domid,
+                 "libxl__device_pci_add failed for "
+                 "PCI device %x:%x:%x.%x (rc %d)",
+                 pci->domain, pci->bus, pci->dev, pci->func,
+                 rc);
+        }
         pci_info_xs_remove(gc, pci, "domid");
     }
     libxl_device_pci_dispose(pci);
@@ -2250,6 +2273,12 @@ static void libxl__device_pci_remove_common(libxl__egc *egc,
     libxl_device_pci_copy(CTX, &prs->pci, pci);
     pci = &prs->pci;
 
+    /* If the device is named then we need to look up the BDF */
+    if (pci->name) {
+        rc = name2bdf(gc, pci);
+        if (rc) goto out;
+    }
+
     prs->force = force;
     libxl__xswait_init(&prs->xswait);
     libxl__ev_qmp_init(&prs->qmp);
@@ -2372,6 +2401,10 @@ static int libxl__device_pci_from_xs_be(libxl__gc *gc,
     if (s)
         pci->vdevfn = strtol(s, (char **) NULL, 16);
 
+    s = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/name-%d", be_path, nr));
+    if (s)
+        pci->name = strdup(s);
+
     s = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/opts-%d", be_path, nr));
     if (s) {
         char *saveptr;
diff --git a/tools/libs/util/libxlu_pci.c b/tools/libs/util/libxlu_pci.c
index 05472a0bd1..ad88fee088 100644
--- a/tools/libs/util/libxlu_pci.c
+++ b/tools/libs/util/libxlu_pci.c
@@ -147,6 +147,7 @@ int xlu_pci_parse_spec_string(XLU_Config *cfg, libxl_device_pci *pci,
 {
     const char *ptr = str;
     bool bdf_present = false;
+    bool name_present = false;
     int ret;
 
     /* Attempt to parse 'bdf' as positional parameter */
@@ -189,6 +190,10 @@ int xlu_pci_parse_spec_string(XLU_Config *cfg, libxl_device_pci *pci,
             pci->power_mgmt = atoi(val);
         } else if (!strcmp(key, "rdm_policy")) {
             ret = parse_rdm_policy(cfg, &pci->rdm_policy, val);
+        } else if (!strcmp(key, "name")) {
+            name_present = true;
+            pci->name = strdup(val);
+            if (!pci->name) ret = ERROR_NOMEM;
         } else {
             XLU__PCI_ERR(cfg, "Unknown PCI_SPEC_STRING option: %s", key);
             ret = ERROR_INVAL;
@@ -201,7 +206,7 @@ int xlu_pci_parse_spec_string(XLU_Config *cfg, libxl_device_pci *pci,
             return ret;
     }
 
-    if (!bdf_present)
+    if (!(bdf_present ^ name_present))
         return ERROR_INVAL;
 
     return 0;
-- 
2.11.0



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

* Re: [PATCH v7 0/7] xl / libxl: named PCI pass-through devices
  2021-01-05 17:46 [PATCH v7 0/7] xl / libxl: named PCI pass-through devices Paul Durrant
                   ` (6 preceding siblings ...)
  2021-01-05 17:46 ` [PATCH v7 7/7] libxl / libxlu: support 'xl pci-attach/detach' by name Paul Durrant
@ 2021-01-13  8:05 ` Oleksandr Andrushchenko
  7 siblings, 0 replies; 13+ messages in thread
From: Oleksandr Andrushchenko @ 2021-01-13  8:05 UTC (permalink / raw)
  To: Wei Liu; +Cc: Paul Durrant, xen-devel, Paul Durrant

Hi, Wei!

Is this series good to go? Could you please tell if it breaks anything?

Thank you in advance,

Oleksandr

On 1/5/21 7:46 PM, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
>
> These are re-worked versions of the remaining patches from v6 of the series
> that were reverted by commit ac6a0af3870b "Revert patches that break libxl
> API".
>
> Paul Durrant (7):
>    docs/man: modify xl(1) in preparation for naming of assignable devices
>    libxlu: introduce xlu_pci_parse_spec_string()
>    libxl: stop setting 'vdevfn' in pci_struct_fill()
>    libxl: add 'name' field to 'libxl_device_pci' in the IDL...
>    xl: support naming of assignable devices
>    docs/man: modify xl-pci-configuration(5) to add 'name' field to
>      PCI_SPEC_STRING
>    libxl / libxlu: support 'xl pci-attach/detach' by name
>
>   docs/man/xl-pci-configuration.5.pod |  25 ++-
>   docs/man/xl.1.pod.in                |  19 +-
>   tools/include/libxl.h               |   6 +
>   tools/include/libxlutil.h           |   8 +-
>   tools/libs/light/libxl_pci.c        | 133 ++++++++++++--
>   tools/libs/light/libxl_types.idl    |  13 +-
>   tools/libs/util/libxlu_pci.c        | 353 +++++++++++++++++++-----------------
>   tools/xl/xl_cmdtable.c              |  16 +-
>   tools/xl/xl_parse.c                 |   4 +-
>   tools/xl/xl_pci.c                   | 120 ++++++++----
>   10 files changed, 457 insertions(+), 240 deletions(-)
>

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

* Re: [PATCH v7 3/7] libxl: stop setting 'vdevfn' in pci_struct_fill()
  2021-01-05 17:46 ` [PATCH v7 3/7] libxl: stop setting 'vdevfn' in pci_struct_fill() Paul Durrant
@ 2021-01-21 14:42   ` Wei Liu
  0 siblings, 0 replies; 13+ messages in thread
From: Wei Liu @ 2021-01-21 14:42 UTC (permalink / raw)
  To: Paul Durrant
  Cc: xen-devel, Paul Durrant, Ian Jackson, Wei Liu, Anthony PERARD

On Tue, Jan 05, 2021 at 05:46:38PM +0000, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
> 
> There are only two call-sites. One always sets it to 0 (which is unnecessary
> as the structure is already initialized to zero) and the other can simply set
> the 'vdevfn' field directly (after proper structure initialization), avoiding
> the need for a local variable.
> 
> A subsequent patch will also make use of pci_struct_fill() in a context
> where 'vdevfn' may already have been set.
> 
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>

Acked-by: Wei Liu <wl@xen.org>


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

* Re: [PATCH v7 4/7] libxl: add 'name' field to 'libxl_device_pci' in the IDL...
  2021-01-05 17:46 ` [PATCH v7 4/7] libxl: add 'name' field to 'libxl_device_pci' in the IDL Paul Durrant
@ 2021-01-21 14:45   ` Wei Liu
  0 siblings, 0 replies; 13+ messages in thread
From: Wei Liu @ 2021-01-21 14:45 UTC (permalink / raw)
  To: Paul Durrant
  Cc: xen-devel, Paul Durrant, Ian Jackson, Wei Liu, Anthony PERARD

On Tue, Jan 05, 2021 at 05:46:39PM +0000, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
> 
> ... and modify libxl_pci_bdf_assignable_add/remove/list() to make use of it.
> 
> libxl_pci_bdf_assignable_add() will store the name of the device in xenstore
> if the field is specified (i.e. non-NULL) and libxl_pci_bdf_assignable_remove()
> will remove devices specified only by name, looking up the BDF as necessary.
> 
> libxl_pci_bdf_assignable_list() will also populate the 'name' field if a name
> was stored by libxl_pci_bdf_assignable_add().
> 
> NOTE: This patch also fixes whitespace in the declaration of 'libxl_device_pci'
>       in the IDL.
> 
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>

Acked-by: Wei Liu <wl@xen.org>


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

* Re: [PATCH v7 5/7] xl: support naming of assignable devices
  2021-01-05 17:46 ` [PATCH v7 5/7] xl: support naming of assignable devices Paul Durrant
@ 2021-01-21 14:49   ` Wei Liu
  0 siblings, 0 replies; 13+ messages in thread
From: Wei Liu @ 2021-01-21 14:49 UTC (permalink / raw)
  To: Paul Durrant
  Cc: xen-devel, Paul Durrant, Ian Jackson, Wei Liu, Anthony PERARD

On Tue, Jan 05, 2021 at 05:46:40PM +0000, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
> 
> With this patch applied 'xl pci-assignable-add' will take an optional '--name'
> parameter, 'xl pci-assignable-remove' can be passed either a BDF or a name and
> 'xl pci-assignable-list' will take a optional '--show-names' flag which
> determines whether names are displayed in its output.
> 
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>

Acked-by: Wei Liu <wl@xen.org>


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

* Re: [PATCH v7 7/7] libxl / libxlu: support 'xl pci-attach/detach' by name
  2021-01-05 17:46 ` [PATCH v7 7/7] libxl / libxlu: support 'xl pci-attach/detach' by name Paul Durrant
@ 2021-01-21 14:50   ` Wei Liu
  0 siblings, 0 replies; 13+ messages in thread
From: Wei Liu @ 2021-01-21 14:50 UTC (permalink / raw)
  To: Paul Durrant
  Cc: xen-devel, Paul Durrant, Wei Liu, Ian Jackson, Anthony PERARD

On Tue, Jan 05, 2021 at 05:46:42PM +0000, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
> 
> This patch modifies libxlu_pci_parse_spec_string() to parse the new 'name'
> parameter of PCI_SPEC_STRING detailed in the updated documention in
> xl-pci-configuration(5) and populate the 'name' field of 'libxl_device_pci'.
> 
> If the 'name' field is non-NULL then both libxl_device_pci_add() and
> libxl_device_pci_remove() will use it to look up the device BDF in
> the list of assignable devices.
> 
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>

Acked-by: Wei Liu <wl@xen.org>


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

end of thread, other threads:[~2021-01-21 14:50 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-05 17:46 [PATCH v7 0/7] xl / libxl: named PCI pass-through devices Paul Durrant
2021-01-05 17:46 ` [PATCH v7 1/7] docs/man: modify xl(1) in preparation for naming of assignable devices Paul Durrant
2021-01-05 17:46 ` [PATCH v7 2/7] libxlu: introduce xlu_pci_parse_spec_string() Paul Durrant
2021-01-05 17:46 ` [PATCH v7 3/7] libxl: stop setting 'vdevfn' in pci_struct_fill() Paul Durrant
2021-01-21 14:42   ` Wei Liu
2021-01-05 17:46 ` [PATCH v7 4/7] libxl: add 'name' field to 'libxl_device_pci' in the IDL Paul Durrant
2021-01-21 14:45   ` Wei Liu
2021-01-05 17:46 ` [PATCH v7 5/7] xl: support naming of assignable devices Paul Durrant
2021-01-21 14:49   ` Wei Liu
2021-01-05 17:46 ` [PATCH v7 6/7] docs/man: modify xl-pci-configuration(5) to add 'name' field to PCI_SPEC_STRING Paul Durrant
2021-01-05 17:46 ` [PATCH v7 7/7] libxl / libxlu: support 'xl pci-attach/detach' by name Paul Durrant
2021-01-21 14:50   ` Wei Liu
2021-01-13  8:05 ` [PATCH v7 0/7] xl / libxl: named PCI pass-through devices Oleksandr Andrushchenko

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.