[PATCH v8 0/5] Display IO topology when PXM data is available (plus some cleanup)

[PATCH v8 0/5] Display IO topology when PXM data is available (plus some cleanup)

[Boris Ostrovsky]

Changes in v8:
* Change sysctl's pcitopo interface such that if device is not found then XEN_INVALID_DEV
  is returned in nodes array. Changes to patches 1 and 5.
* Get rid of first_dev and use num_devs in xen_sysctl_pcitopoinfo  as an OUT argument
  that reports how many devices have been processed. Changes to patches 1 and 5.
  -- Add set_xen_guest_handle_offset macro to help toolstack handle this (new patch 4)
* Replace LOGEV with LOGE (patches 2, 3, 5)

Changes in v7:
* Allow one of arguments to NUMA info sysctls to be NULL, in which case only
  the non-NULL buffer will be filled in by hypervisor (patches 1 and 4)
* Properly handle -ENODEVS in PCI topology sysctl (patch 2)
* Error handling changes in patch 5

Changes in v6:
* PCI topology interface changes: no continuations, userspace will be dealing
  with "unfinished" sysctl (patches 2 and 5)
* Unknown device will cause ENODEV in sysctl
* No NULL tests in libxc
* Loop control initialization fix (similar to commit 26da081ac91a)
* Other minor changes (see per-patch notes)

Changes in v5:
* Make CPU topology and NUMA info sysctls behave more like XEN_DOMCTL_get_vcpu_msrs
  when passed NULL buffers. This required toolstack changes as well
* Don't use 8-bit data types in interfaces
* Fold interface version update into patch#3

Changes in v4:
* Split cputopology and NUMA info changes into separate patches
* Added patch#1 (partly because patch#4 needs to know when when distance is invalid,
  i.e. NUMA_NO_DISTANCE)
* Split sysctl version update into a separate patch
* Other changes are listed in each patch
* NOTE: I did not test python's xc changes since I don't think I know how.

Changes in v3:
* Added patch #1 to more consistently define nodes as a u8 and properly
  use NUMA_NO_NODE.
* Make changes to xen_sysctl_numainfo, similar to those made to
  xen_sysctl_topologyinfo. (Q: I kept both sets of changes in the same
  patch #3 to avoid bumping interface version twice. Perhaps it's better
  to split it into two?)
* Instead of copying data for each loop index allocate a buffer and copy
  once for all three queries in sysctl.c.
* Move hypercall buffer management from libxl to libxc (as requested by
  Dario, patches #5 and #6).
* Report topology info for offlined CPUs as well
* Added LIBXL_HAVE_PCITOPO macro

Changes in v2:
* Split topology sysctls into two --- one for CPU topology and the other
  for devices
* Avoid long loops in the hypervisor by using continuations. (I am not
  particularly happy about using first_dev in the interface, suggestions
  for a better interface would be appreciated)
* Use proper libxl conventions for interfaces
* Avoid hypervisor stack corruption when copying PXM data from guest



A few patches that add interface for querying hypervisor about device
topology and allow 'xl info -n' display this information if PXM object
is provided by ACPI.

This series also makes some optimizations and cleanup of current CPU
topology and NUMA sysctl queries.


Boris Ostrovsky (5):
  sysctl: Add sysctl interface for querying PCI topology
  libxl/libxc: Move libxl_get_cpu_topology()'s hypercall buffer
    management to libxc
  libxl/libxc: Move libxl_get_numainfo()'s hypercall buffer management
    to libxc
  libxc: Provide set_xen_guest_handle_offset macro
  libxl: Add interface for querying hypervisor about PCI topology

 tools/libxc/include/xenctrl.h       |   23 +++++--
 tools/libxc/xc_misc.c               |   92 +++++++++++++++++++++----
 tools/libxl/libxl.c                 |  129 +++++++++++++++++++----------------
 tools/libxl/libxl.h                 |   12 +++
 tools/libxl/libxl_freebsd.c         |   12 +++
 tools/libxl/libxl_internal.h        |    5 ++
 tools/libxl/libxl_linux.c           |   70 +++++++++++++++++++
 tools/libxl/libxl_netbsd.c          |   12 +++
 tools/libxl/libxl_types.idl         |    7 ++
 tools/libxl/libxl_utils.c           |    8 ++
 tools/libxl/xl_cmdimpl.c            |   40 +++++++++--
 tools/misc/xenpm.c                  |   51 ++++++--------
 tools/python/xen/lowlevel/xc/xc.c   |   58 ++++++----------
 xen/common/sysctl.c                 |   55 +++++++++++++++
 xen/include/public/sysctl.h         |   27 +++++++
 xen/xsm/flask/hooks.c               |    1 +
 xen/xsm/flask/policy/access_vectors |    1 +
 17 files changed, 453 insertions(+), 150 deletions(-)

[PATCH v8 1/5] sysctl: Add sysctl interface for querying PCI topology

[Boris Ostrovsky]

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
Changes in v8
* Report XEN_INVALID_DEV for unknown devices
* Get rid of first_dev and use num_devs as an OUT argument that reports
  how many devices have been processed
* Update comments in sysctl.h
* Drop changes to xsm-flask.txt

 xen/common/sysctl.c                 |   55 +++++++++++++++++++++++++++++++++++
 xen/include/public/sysctl.h         |   27 +++++++++++++++++
 xen/xsm/flask/hooks.c               |    1 +
 xen/xsm/flask/policy/access_vectors |    1 +
 4 files changed, 84 insertions(+), 0 deletions(-)

diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index 7361064..2a48e45 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -412,6 +412,61 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
         break;
 #endif
 
+#ifdef HAS_PCI
+    case XEN_SYSCTL_pcitopoinfo:
+    {
+        xen_sysctl_pcitopoinfo_t *ti = &op->u.pcitopoinfo;
+        unsigned i = 0;
+
+        if ( guest_handle_is_null(ti->devs) ||
+             guest_handle_is_null(ti->nodes) )
+        {
+            ret = -EINVAL;
+            break;
+        }
+
+        while ( i < ti->num_devs )
+        {
+            physdev_pci_device_t dev;
+            uint32_t node;
+            struct pci_dev *pdev;
+
+            if ( copy_from_guest_offset(&dev, ti->devs, i, 1) )
+            {
+                ret = -EFAULT;
+                break;
+            }
+
+            spin_lock(&pcidevs_lock);
+            pdev = pci_get_pdev(dev.seg, dev.bus, dev.devfn);
+            if ( !pdev )
+                node = XEN_INVALID_DEV;
+            else if ( pdev->node == NUMA_NO_NODE )
+                node = XEN_INVALID_NODE_ID;
+            else
+                node = pdev->node;
+            spin_unlock(&pcidevs_lock);
+
+            if ( copy_to_guest_offset(ti->nodes, i, &node, 1) )
+            {
+                ret = -EFAULT;
+                break;
+            }
+
+            if ( (++i > 0x3f) && hypercall_preempt_check() )
+                break;
+        }
+
+        if ( !ret && (ti->num_devs != i) )
+        {
+            ti->num_devs = i;
+            if ( __copy_field_to_guest(u_sysctl, op, u.pcitopoinfo.num_devs) )
+                ret = -EFAULT;
+        }
+    }
+    break;
+#endif
+
     default:
         ret = arch_do_sysctl(op, u_sysctl);
         copyback = 0;
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 021d505..fbe3c6e 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -33,6 +33,7 @@
 
 #include "xen.h"
 #include "domctl.h"
+#include "physdev.h"
 
 #define XEN_SYSCTL_INTERFACE_VERSION 0x0000000C
 
@@ -669,6 +670,30 @@ struct xen_sysctl_psr_cmt_op {
 typedef struct xen_sysctl_psr_cmt_op xen_sysctl_psr_cmt_op_t;
 DEFINE_XEN_GUEST_HANDLE(xen_sysctl_psr_cmt_op_t);
 
+/* XEN_SYSCTL_pcitopoinfo */
+#define XEN_INVALID_DEV (XEN_INVALID_NODE_ID - 1)
+struct xen_sysctl_pcitopoinfo {
+    /*
+     * IN: Number of elements in 'pcitopo' and 'nodes' arrays.
+     * OUT: Number of processed elements of those arrays.
+     */
+    uint32_t num_devs;
+
+    /* IN: list of devices for which node IDs are requested. */
+    XEN_GUEST_HANDLE_64(physdev_pci_device_t) devs;
+
+    /*
+     * OUT: node identifier for each device.
+     * If information for a particular device is not avalable then
+     * corresponding entry will be set to XEN_INVALID_NODE_ID. If
+     * device is not known to the hypervisor then XEN_INVALID_DEV
+     * will be provided.
+     */
+    XEN_GUEST_HANDLE_64(uint32) nodes;
+};
+typedef struct xen_sysctl_pcitopoinfo xen_sysctl_pcitopoinfo_t;
+DEFINE_XEN_GUEST_HANDLE(xen_sysctl_pcitopoinfo_t);
+
 struct xen_sysctl {
     uint32_t cmd;
 #define XEN_SYSCTL_readconsole                    1
@@ -691,12 +716,14 @@ struct xen_sysctl {
 #define XEN_SYSCTL_scheduler_op                  19
 #define XEN_SYSCTL_coverage_op                   20
 #define XEN_SYSCTL_psr_cmt_op                    21
+#define XEN_SYSCTL_pcitopoinfo                   22
     uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */
     union {
         struct xen_sysctl_readconsole       readconsole;
         struct xen_sysctl_tbuf_op           tbuf_op;
         struct xen_sysctl_physinfo          physinfo;
         struct xen_sysctl_cputopoinfo       cputopoinfo;
+        struct xen_sysctl_pcitopoinfo       pcitopoinfo;
         struct xen_sysctl_numainfo          numainfo;
         struct xen_sysctl_sched_id          sched_id;
         struct xen_sysctl_perfc_op          perfc_op;
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 6215001..32763da 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -783,6 +783,7 @@ static int flask_sysctl(int cmd)
     case XEN_SYSCTL_physinfo:
     case XEN_SYSCTL_cputopoinfo:
     case XEN_SYSCTL_numainfo:
+    case XEN_SYSCTL_pcitopoinfo:
         return domain_has_xen(current->domain, XEN__PHYSINFO);
 
     case XEN_SYSCTL_psr_cmt_op:
diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
index af4a6ae..356ca3b 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -28,6 +28,7 @@ class xen
 # XENPF_microcode_update
     microcode
 # XEN_SYSCTL_physinfo, XEN_SYSCTL_cputopoinfo, XEN_SYSCTL_numainfo
+# XEN_SYSCTL_pcitopoinfo
     physinfo
 # XENPF_platform_quirk
     quirk
-- 
1.7.1

[PATCH v8 2/5] libxl/libxc: Move libxl_get_cpu_topology()'s hypercall buffer management to libxc

[Boris Ostrovsky]

xc_cputopoinfo() is not expected to be used on a hot path and therefore
hypercall buffer management can be pushed into libxc. This will simplify
life for callers.

Also update error reporting macros.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
Changes in v8:
* Replace LOGEV with LOGE

 tools/libxc/include/xenctrl.h     |    5 ++-
 tools/libxc/xc_misc.c             |   23 +++++++++++-----
 tools/libxl/libxl.c               |   37 ++++++++------------------
 tools/misc/xenpm.c                |   51 ++++++++++++++++---------------------
 tools/python/xen/lowlevel/xc/xc.c |   20 ++++++--------
 5 files changed, 61 insertions(+), 75 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 6994c51..c5a74f4 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1229,7 +1229,7 @@ int xc_readconsolering(xc_interface *xch,
 int xc_send_debug_keys(xc_interface *xch, char *keys);
 
 typedef xen_sysctl_physinfo_t xc_physinfo_t;
-typedef xen_sysctl_cputopoinfo_t xc_cputopoinfo_t;
+typedef xen_sysctl_cputopo_t xc_cputopo_t;
 typedef xen_sysctl_numainfo_t xc_numainfo_t;
 
 typedef uint32_t xc_cpu_to_node_t;
@@ -1240,7 +1240,8 @@ typedef uint64_t xc_node_to_memfree_t;
 typedef uint32_t xc_node_to_node_dist_t;
 
 int xc_physinfo(xc_interface *xch, xc_physinfo_t *info);
-int xc_cputopoinfo(xc_interface *xch, xc_cputopoinfo_t *info);
+int xc_cputopoinfo(xc_interface *xch, unsigned *max_cpus,
+                   xc_cputopo_t *cputopo);
 int xc_numainfo(xc_interface *xch, xc_numainfo_t *info);
 
 int xc_sched_id(xc_interface *xch,
diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c
index be68291..630a86c 100644
--- a/tools/libxc/xc_misc.c
+++ b/tools/libxc/xc_misc.c
@@ -177,22 +177,31 @@ int xc_physinfo(xc_interface *xch,
     return 0;
 }
 
-int xc_cputopoinfo(xc_interface *xch,
-                   xc_cputopoinfo_t *put_info)
+int xc_cputopoinfo(xc_interface *xch, unsigned *max_cpus,
+                   xc_cputopo_t *cputopo)
 {
     int ret;
     DECLARE_SYSCTL;
+    DECLARE_HYPERCALL_BOUNCE(cputopo, *max_cpus * sizeof(*cputopo),
+                             XC_HYPERCALL_BUFFER_BOUNCE_OUT);
 
-    sysctl.cmd = XEN_SYSCTL_cputopoinfo;
+    if ( (ret = xc_hypercall_bounce_pre(xch, cputopo)) )
+        goto out;
 
-    memcpy(&sysctl.u.cputopoinfo, put_info, sizeof(*put_info));
+    sysctl.u.cputopoinfo.num_cpus = *max_cpus;
+    set_xen_guest_handle(sysctl.u.cputopoinfo.cputopo, cputopo);
+
+    sysctl.cmd = XEN_SYSCTL_cputopoinfo;
 
     if ( (ret = do_sysctl(xch, &sysctl)) != 0 )
-        return ret;
+        goto out;
 
-    memcpy(put_info, &sysctl.u.cputopoinfo, sizeof(*put_info));
+    *max_cpus = sysctl.u.cputopoinfo.num_cpus;
 
-    return 0;
+out:
+    xc_hypercall_bounce_post(xch, cputopo);
+
+    return ret;
 }
 
 int xc_numainfo(xc_interface *xch,
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 516713e..5f2fcff 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5102,37 +5102,28 @@ int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo *physinfo)
 libxl_cputopology *libxl_get_cpu_topology(libxl_ctx *ctx, int *nb_cpu_out)
 {
     GC_INIT(ctx);
-    xc_cputopoinfo_t tinfo;
-    DECLARE_HYPERCALL_BUFFER(xen_sysctl_cputopo_t, cputopo);
+    xc_cputopo_t *cputopo;
     libxl_cputopology *ret = NULL;
     int i;
+    unsigned num_cpus;
 
-    /* Setting buffer to NULL makes the hypercall return number of CPUs */
-    set_xen_guest_handle(tinfo.cputopo, HYPERCALL_BUFFER_NULL);
-    if (xc_cputopoinfo(ctx->xch, &tinfo) != 0)
+    /* Setting buffer to NULL makes the call return number of CPUs */
+    if (xc_cputopoinfo(ctx->xch, &num_cpus, NULL))
     {
-        LIBXL__LOG(ctx, XTL_ERROR, "Unable to determine number of CPUS");
-        ret = NULL;
+        LOGE(ERROR, "Unable to determine number of CPUS");
         goto out;
     }
 
-    cputopo = xc_hypercall_buffer_alloc(ctx->xch, cputopo,
-                                        sizeof(*cputopo) * tinfo.num_cpus);
-    if (cputopo == NULL) {
-        LIBXL__LOG_ERRNOVAL(ctx, XTL_ERROR, ENOMEM,
-                            "Unable to allocate hypercall arguments");
-        goto fail;
-    }
-    set_xen_guest_handle(tinfo.cputopo, cputopo);
+    cputopo = libxl__zalloc(gc, sizeof(*cputopo) * num_cpus);
 
-    if (xc_cputopoinfo(ctx->xch, &tinfo) != 0) {
-        LIBXL__LOG_ERRNO(ctx, XTL_ERROR, "CPU topology info hypercall failed");
-        goto fail;
+    if (xc_cputopoinfo(ctx->xch, &num_cpus, cputopo)) {
+        LOGE(ERROR, "CPU topology info hypercall failed");
+        goto out;
     }
 
-    ret = libxl__zalloc(NOGC, sizeof(libxl_cputopology) * tinfo.num_cpus);
+    ret = libxl__zalloc(NOGC, sizeof(libxl_cputopology) * num_cpus);
 
-    for (i = 0; i < tinfo.num_cpus; i++) {
+    for (i = 0; i < num_cpus; i++) {
 #define V(map, i, invalid) ( cputopo[i].map == invalid) ? \
    LIBXL_CPUTOPOLOGY_INVALID_ENTRY : cputopo[i].map
         ret[i].core = V(core, i, XEN_INVALID_CORE_ID);
@@ -5141,11 +5132,7 @@ libxl_cputopology *libxl_get_cpu_topology(libxl_ctx *ctx, int *nb_cpu_out)
 #undef V
     }
 
-fail:
-    xc_hypercall_buffer_free(ctx->xch, cputopo);
-
-    if (ret)
-        *nb_cpu_out = tinfo.num_cpus;
+    *nb_cpu_out = num_cpus;
 
  out:
     GC_FREE;
diff --git a/tools/misc/xenpm.c b/tools/misc/xenpm.c
index a5d07de..24ee6ef 100644
--- a/tools/misc/xenpm.c
+++ b/tools/misc/xenpm.c
@@ -355,20 +355,17 @@ static void signal_int_handler(int signo)
     int i, j, k;
     struct timeval tv;
     int cx_cap = 0, px_cap = 0;
-    DECLARE_HYPERCALL_BUFFER(xen_sysctl_cputopo_t, cputopo);
-    xc_cputopoinfo_t info = { 0 };
+    xc_cputopo_t *cputopo;
+    unsigned max_cpus;
 
-    set_xen_guest_handle(info.cputopo, HYPERCALL_BUFFER_NULL);
-    if ( xc_cputopoinfo(xc_handle, &info) != 0 )
+    if ( xc_cputopoinfo(xc_handle, &max_cpus, NULL) != 0 )
     {
         fprintf(stderr, "failed to discover number of CPUs: %s\n",
                 strerror(errno));
         goto out;
     }
 
-    cputopo = xc_hypercall_buffer_alloc(xc_handle, cputopo,
-                                        sizeof(*cputopo) * info.num_cpus);
-
+    cputopo = calloc(max_cpus, sizeof(*cputopo));
     if ( cputopo == NULL )
     {
 	fprintf(stderr, "failed to allocate hypercall buffers\n");
@@ -453,28 +450,26 @@ static void signal_int_handler(int signo)
             printf("  Avg freq\t%d\tKHz\n", avgfreq[i]);
     }
 
-    set_xen_guest_handle(info.cputopo, cputopo);
-
-    if ( cx_cap && !xc_cputopoinfo(xc_handle, &info) )
+    if ( cx_cap && !xc_cputopoinfo(xc_handle, &max_cpus, cputopo) )
     {
         uint32_t socket_ids[MAX_NR_CPU];
         uint32_t core_ids[MAX_NR_CPU];
         uint32_t socket_nr = 0;
         uint32_t core_nr = 0;
 
-        if ( info.num_cpus > MAX_NR_CPU )
-            info.num_cpus = MAX_NR_CPU;
+        if ( max_cpus > MAX_NR_CPU )
+            max_cpus = MAX_NR_CPU;
         /* check validity */
-        for ( i = 0; i < info.num_cpus; i++ )
+        for ( i = 0; i < max_cpus; i++ )
         {
             if ( cputopo[i].core == XEN_INVALID_CORE_ID ||
                  cputopo[i].socket == XEN_INVALID_SOCKET_ID )
                 break;
         }
-        if ( i >= info.num_cpus )
+        if ( i >= max_cpus )
         {
             /* find socket nr & core nr per socket */
-            for ( i = 0; i < info.num_cpus; i++ )
+            for ( i = 0; i < max_cpus; i++ )
             {
                 for ( j = 0; j < socket_nr; j++ )
                     if ( cputopo[i].socket == socket_ids[j] )
@@ -501,7 +496,7 @@ static void signal_int_handler(int signo)
                 unsigned int n;
                 uint64_t res;
 
-                for ( j = 0; j < info.num_cpus; j++ )
+                for ( j = 0; j < max_cpus; j++ )
                 {
                     if ( cputopo[j].socket == socket_ids[i] )
                         break;
@@ -520,7 +515,7 @@ static void signal_int_handler(int signo)
                 }
                 for ( k = 0; k < core_nr; k++ )
                 {
-                    for ( j = 0; j < info.num_cpus; j++ )
+                    for ( j = 0; j < max_cpus; j++ )
                     {
                         if ( cputopo[j].socket == socket_ids[i] &&
                              cputopo[j].core == core_ids[k] )
@@ -558,7 +553,7 @@ static void signal_int_handler(int signo)
     free(sum);
     free(avgfreq);
 out:
-    xc_hypercall_buffer_free(xc_handle, cputopo);
+    free(cputopo);
     xc_interface_close(xc_handle);
     exit(0);
 }
@@ -965,12 +960,11 @@ void scaling_governor_func(int argc, char *argv[])
 
 void cpu_topology_func(int argc, char *argv[])
 {
-    DECLARE_HYPERCALL_BUFFER(xen_sysctl_cputopo_t, cputopo);
-    xc_cputopoinfo_t info = { 0 };
-    int i, rc = ENOMEM;
+    xc_cputopo_t *cputopo;
+    unsigned max_cpus;
+    int i, rc;
 
-    set_xen_guest_handle(info.cputopo, HYPERCALL_BUFFER_NULL);
-    if ( xc_cputopoinfo(xc_handle, &info) )
+    if ( xc_cputopoinfo(xc_handle, &max_cpus, NULL) != 0 )
     {
         rc = errno;
         fprintf(stderr, "failed to discover number of CPUs (%d - %s)\n",
@@ -978,16 +972,15 @@ void cpu_topology_func(int argc, char *argv[])
         goto out;
     }
 
-    cputopo = xc_hypercall_buffer_alloc(xc_handle, cputopo,
-                                        sizeof(*cputopo) * info.num_cpus);
+    cputopo = calloc(max_cpus, sizeof(*cputopo));
     if ( cputopo == NULL )
     {
+	rc = ENOMEM;
 	fprintf(stderr, "failed to allocate hypercall buffers\n");
 	goto out;
     }
 
-    set_xen_guest_handle(info.cputopo, cputopo);
-    if ( xc_cputopoinfo(xc_handle, &info) )
+    if ( xc_cputopoinfo(xc_handle, &max_cpus, cputopo) )
     {
         rc = errno;
         fprintf(stderr, "Cannot get Xen CPU topology (%d - %s)\n",
@@ -996,7 +989,7 @@ void cpu_topology_func(int argc, char *argv[])
     }
 
     printf("CPU\tcore\tsocket\tnode\n");
-    for ( i = 0; i < info.num_cpus; i++ )
+    for ( i = 0; i < max_cpus; i++ )
     {
         if ( cputopo[i].core == XEN_INVALID_CORE_ID )
             continue;
@@ -1005,7 +998,7 @@ void cpu_topology_func(int argc, char *argv[])
     }
     rc = 0;
 out:
-    xc_hypercall_buffer_free(xc_handle, cputopo);
+    free(cputopo);
     if ( rc )
         exit(rc);
 }
diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c
index ba66d55..21ba57d 100644
--- a/tools/python/xen/lowlevel/xc/xc.c
+++ b/tools/python/xen/lowlevel/xc/xc.c
@@ -1220,30 +1220,26 @@ static PyObject *pyxc_getcpuinfo(XcObject *self, PyObject *args, PyObject *kwds)
 
 static PyObject *pyxc_topologyinfo(XcObject *self)
 {
-    xc_cputopoinfo_t tinfo = { 0 };
-    unsigned i;
+    xc_cputopo_t *cputopo = NULL;
+    unsigned i, num_cpus;
     PyObject *ret_obj = NULL;
     PyObject *cpu_to_core_obj, *cpu_to_socket_obj, *cpu_to_node_obj;
-    DECLARE_HYPERCALL_BUFFER(xen_sysctl_cputopo_t, cputopo);
 
-    set_xen_guest_handle(tinfo.cputopo, HYPERCALL_BUFFER_NULL);
-    if ( xc_cputopoinfo(self->xc_handle, &tinfo) != 0 )
+    if ( xc_cputopoinfo(self->xc_handle, &num_cpus, NULL) != 0 )
         goto out;
 
-    cputopo = xc_hypercall_buffer_alloc(self->xc_handle, cputopo,
-                                        sizeof(*cputopo) * tinfo.num_cpus);
+    cputopo = calloc(num_cpus, sizeof(*cputopo));
     if ( cputopo == NULL )
     	goto out;
 
-    set_xen_guest_handle(tinfo.cputopo, cputopo);
-    if ( xc_cputopoinfo(self->xc_handle, &tinfo) != 0 )
+    if ( xc_cputopoinfo(self->xc_handle, &num_cpus, cputopo) != 0 )
         goto out;
 
     /* Construct cpu-to-* lists. */
     cpu_to_core_obj = PyList_New(0);
     cpu_to_socket_obj = PyList_New(0);
     cpu_to_node_obj = PyList_New(0);
-    for ( i = 0; i < tinfo.num_cpus; i++ )
+    for ( i = 0; i < num_cpus; i++ )
     {
         if ( cputopo[i].core == XEN_INVALID_CORE_ID )
         {
@@ -1279,7 +1275,7 @@ static PyObject *pyxc_topologyinfo(XcObject *self)
         }
     }
 
-    ret_obj = Py_BuildValue("{s:i}", "max_cpu_index", tinfo.num_cpus + 1);
+    ret_obj = Py_BuildValue("{s:i}", "max_cpu_index", num_cpus + 1);
 
     PyDict_SetItemString(ret_obj, "cpu_to_core", cpu_to_core_obj);
     Py_DECREF(cpu_to_core_obj);
@@ -1291,7 +1287,7 @@ static PyObject *pyxc_topologyinfo(XcObject *self)
     Py_DECREF(cpu_to_node_obj);
 
 out:
-    xc_hypercall_buffer_free(self->xc_handle, cputopo);
+    free(cputopo);
     return ret_obj ? ret_obj : pyxc_error_to_exception(self->xc_handle);
 }
 
-- 
1.7.1

[PATCH v8 3/5] libxl/libxc: Move libxl_get_numainfo()'s hypercall buffer management to libxc

[Boris Ostrovsky]

xc_numainfo() is not expected to be used on a hot path and therefore
hypercall buffer management can be pushed into libxc. This will simplify
life for callers.

Also update error logging macros.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
Changes in v8:
* Replace LOGEV with LOGE

 tools/libxc/include/xenctrl.h     |    4 ++-
 tools/libxc/xc_misc.c             |   30 ++++++++++++++++-----
 tools/libxl/libxl.c               |   51 ++++++++++++------------------------
 tools/python/xen/lowlevel/xc/xc.c |   38 ++++++++++-----------------
 4 files changed, 57 insertions(+), 66 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index c5a74f4..8c8eae6 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1231,6 +1231,7 @@ int xc_send_debug_keys(xc_interface *xch, char *keys);
 typedef xen_sysctl_physinfo_t xc_physinfo_t;
 typedef xen_sysctl_cputopo_t xc_cputopo_t;
 typedef xen_sysctl_numainfo_t xc_numainfo_t;
+typedef xen_sysctl_meminfo_t xc_meminfo_t;
 
 typedef uint32_t xc_cpu_to_node_t;
 typedef uint32_t xc_cpu_to_socket_t;
@@ -1242,7 +1243,8 @@ typedef uint32_t xc_node_to_node_dist_t;
 int xc_physinfo(xc_interface *xch, xc_physinfo_t *info);
 int xc_cputopoinfo(xc_interface *xch, unsigned *max_cpus,
                    xc_cputopo_t *cputopo);
-int xc_numainfo(xc_interface *xch, xc_numainfo_t *info);
+int xc_numainfo(xc_interface *xch, unsigned *max_nodes,
+                xc_meminfo_t *meminfo, uint32_t *distance);
 
 int xc_sched_id(xc_interface *xch,
                 int *sched_id);
diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c
index 630a86c..94b70b4 100644
--- a/tools/libxc/xc_misc.c
+++ b/tools/libxc/xc_misc.c
@@ -204,22 +204,38 @@ out:
     return ret;
 }
 
-int xc_numainfo(xc_interface *xch,
-                xc_numainfo_t *put_info)
+int xc_numainfo(xc_interface *xch, unsigned *max_nodes,
+                xc_meminfo_t *meminfo, uint32_t *distance)
 {
     int ret;
     DECLARE_SYSCTL;
+    DECLARE_HYPERCALL_BOUNCE(meminfo, *max_nodes * sizeof(*meminfo),
+                             XC_HYPERCALL_BUFFER_BOUNCE_OUT);
+    DECLARE_HYPERCALL_BOUNCE(distance,
+                             *max_nodes * *max_nodes * sizeof(*distance),
+                             XC_HYPERCALL_BUFFER_BOUNCE_OUT);
+
+    if ( (ret = xc_hypercall_bounce_pre(xch, meminfo)) )
+        goto out;
+    if ((ret = xc_hypercall_bounce_pre(xch, distance)) )
+        goto out;
+
+    sysctl.u.numainfo.num_nodes = *max_nodes;
+    set_xen_guest_handle(sysctl.u.numainfo.meminfo, meminfo);
+    set_xen_guest_handle(sysctl.u.numainfo.distance, distance);
 
     sysctl.cmd = XEN_SYSCTL_numainfo;
 
-    memcpy(&sysctl.u.numainfo, put_info, sizeof(*put_info));
+    if ( (ret = do_sysctl(xch, &sysctl)) != 0 )
+        goto out;
 
-    if ((ret = do_sysctl(xch, &sysctl)) != 0)
-        return ret;
+    *max_nodes = sysctl.u.numainfo.num_nodes;
 
-    memcpy(put_info, &sysctl.u.numainfo, sizeof(*put_info));
+out:
+    xc_hypercall_bounce_post(xch, meminfo);
+    xc_hypercall_bounce_post(xch, distance);
 
-    return 0;
+    return ret;
 }
 
 
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 5f2fcff..5c88a88 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5142,61 +5142,44 @@ libxl_cputopology *libxl_get_cpu_topology(libxl_ctx *ctx, int *nb_cpu_out)
 libxl_numainfo *libxl_get_numainfo(libxl_ctx *ctx, int *nr)
 {
     GC_INIT(ctx);
-    xc_numainfo_t ninfo;
-    DECLARE_HYPERCALL_BUFFER(xen_sysctl_meminfo_t, meminfo);
-    DECLARE_HYPERCALL_BUFFER(uint32_t, distance);
+    xc_meminfo_t *meminfo;
+    uint32_t *distance;
     libxl_numainfo *ret = NULL;
     int i, j;
+    unsigned num_nodes;
 
-    set_xen_guest_handle(ninfo.meminfo, HYPERCALL_BUFFER_NULL);
-    set_xen_guest_handle(ninfo.distance, HYPERCALL_BUFFER_NULL);
-    if (xc_numainfo(ctx->xch, &ninfo) != 0) {
-        LIBXL__LOG(ctx, XTL_ERROR, "Unable to determine number of NODES");
-        ret = NULL;
+    if (xc_numainfo(ctx->xch, &num_nodes, NULL, NULL)) {
+        LOGE(ERROR, "Unable to determine number of nodes");
         goto out;
     }
 
-    meminfo = xc_hypercall_buffer_alloc(ctx->xch, meminfo,
-                                        sizeof(*meminfo) * ninfo.num_nodes);
-    distance = xc_hypercall_buffer_alloc(ctx->xch, distance,
-                                         sizeof(*distance) *
-                                         ninfo.num_nodes * ninfo.num_nodes);
-    if ((meminfo == NULL) || (distance == NULL)) {
-        LIBXL__LOG_ERRNOVAL(ctx, XTL_ERROR, ENOMEM,
-                            "Unable to allocate hypercall arguments");
-        goto fail;
-    }
+    meminfo = libxl__zalloc(gc, sizeof(*meminfo) * num_nodes);
+    distance = libxl__zalloc(gc, sizeof(*distance) * num_nodes * num_nodes);
 
-    set_xen_guest_handle(ninfo.meminfo, meminfo);
-    set_xen_guest_handle(ninfo.distance, distance);
-    if (xc_numainfo(ctx->xch, &ninfo) != 0) {
-        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting numainfo");
-        goto fail;
+    if (xc_numainfo(ctx->xch, &num_nodes, meminfo, distance)) {
+        LOGE(ERROR, "getting numainfo");
+        goto out;
     }
 
-    *nr = ninfo.num_nodes;
+    *nr = num_nodes;
 
-    ret = libxl__zalloc(NOGC, sizeof(libxl_numainfo) * ninfo.num_nodes);
-    for (i = 0; i < ninfo.num_nodes; i++)
-        ret[i].dists = libxl__calloc(NOGC, ninfo.num_nodes, sizeof(*distance));
+    ret = libxl__zalloc(NOGC, sizeof(libxl_numainfo) * num_nodes);
+    for (i = 0; i < num_nodes; i++)
+        ret[i].dists = libxl__calloc(NOGC, num_nodes, sizeof(*distance));
 
-    for (i = 0; i < ninfo.num_nodes; i++) {
+    for (i = 0; i < num_nodes; i++) {
 #define V(val, invalid) (val == invalid) ? \
        LIBXL_NUMAINFO_INVALID_ENTRY : val
         ret[i].size = V(meminfo[i].memsize, XEN_INVALID_MEM_SZ);
         ret[i].free = V(meminfo[i].memfree, XEN_INVALID_MEM_SZ);
-        ret[i].num_dists = ninfo.num_nodes;
+        ret[i].num_dists = num_nodes;
         for (j = 0; j < ret[i].num_dists; j++) {
-            unsigned idx = i * ninfo.num_nodes + j;
+            unsigned idx = i * num_nodes + j;
             ret[i].dists[j] = V(distance[idx], XEN_INVALID_NODE_DIST);
         }
 #undef V
     }
 
- fail:
-    xc_hypercall_buffer_free(ctx->xch, meminfo);
-    xc_hypercall_buffer_free(ctx->xch, distance);
-
  out:
     GC_FREE;
     return ret;
diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c
index 21ba57d..fbd93db 100644
--- a/tools/python/xen/lowlevel/xc/xc.c
+++ b/tools/python/xen/lowlevel/xc/xc.c
@@ -1293,33 +1293,23 @@ out:
 
 static PyObject *pyxc_numainfo(XcObject *self)
 {
-    xc_numainfo_t ninfo = { 0 };
-    unsigned i, j;
+    unsigned i, j, num_nodes;
     uint64_t free_heap;
     PyObject *ret_obj = NULL, *node_to_node_dist_list_obj;
     PyObject *node_to_memsize_obj, *node_to_memfree_obj;
     PyObject *node_to_dma32_mem_obj, *node_to_node_dist_obj;
-    DECLARE_HYPERCALL_BUFFER(xen_sysctl_meminfo_t, meminfo);
-    DECLARE_HYPERCALL_BUFFER(uint32_t, distance);
+    xc_meminfo_t *meminfo = NULL;
+    uint32_t *distance = NULL;
 
-    set_xen_guest_handle(ninfo.meminfo, HYPERCALL_BUFFER_NULL);
-    set_xen_guest_handle(ninfo.distance, HYPERCALL_BUFFER_NULL);
-    if ( xc_numainfo(self->xc_handle, &ninfo) != 0 )
+    if ( xc_numainfo(self->xc_handle, &num_nodes, NULL, NULL) != 0 )
         goto out;
 
-    meminfo = xc_hypercall_buffer_alloc(self->xc_handle, meminfo,
-                                        sizeof(*meminfo) * ninfo.num_nodes);
-    if ( meminfo == NULL )
-        goto out;
-    distance = xc_hypercall_buffer_alloc(self->xc_handle, distance,
-                                         sizeof(*distance) *
-                                         ninfo.num_nodes * ninfo.num_nodes);
-    if ( distance == NULL )
+    meminfo = calloc(num_nodes, sizeof(*meminfo));
+    distance = calloc(num_nodes * num_nodes, sizeof(*distance));
+    if ( (meminfo == NULL) || (distance == NULL) )
         goto out;
 
-    set_xen_guest_handle(ninfo.meminfo, meminfo);
-    set_xen_guest_handle(ninfo.distance, distance);
-    if ( xc_numainfo(self->xc_handle, &ninfo) != 0 )
+    if ( xc_numainfo(self->xc_handle, &num_nodes, meminfo, distance) != 0 )
         goto out;
 
     /* Construct node-to-* lists. */
@@ -1327,7 +1317,7 @@ static PyObject *pyxc_numainfo(XcObject *self)
     node_to_memfree_obj = PyList_New(0);
     node_to_dma32_mem_obj = PyList_New(0);
     node_to_node_dist_list_obj = PyList_New(0);
-    for ( i = 0; i < ninfo.num_nodes; i++ )
+    for ( i = 0; i < num_nodes; i++ )
     {
         PyObject *pyint;
         unsigned invalid_node;
@@ -1351,9 +1341,9 @@ static PyObject *pyxc_numainfo(XcObject *self)
         /* Node to Node Distance */
         node_to_node_dist_obj = PyList_New(0);
         invalid_node = (meminfo[i].memsize == XEN_INVALID_MEM_SZ);
-        for ( j = 0; j < ninfo.num_nodes; j++ )
+        for ( j = 0; j < num_nodes; j++ )
         {
-            uint32_t dist = distance[i * ninfo.num_nodes + j];
+            uint32_t dist = distance[i * num_nodes + j];
             if ( invalid_node || (dist == XEN_INVALID_NODE_DIST) )
             {
                 PyList_Append(node_to_node_dist_obj, Py_None);
@@ -1369,7 +1359,7 @@ static PyObject *pyxc_numainfo(XcObject *self)
         Py_DECREF(node_to_node_dist_obj);
     }
 
-    ret_obj = Py_BuildValue("{s:i}", "max_node_index", ninfo.num_nodes + 1);
+    ret_obj = Py_BuildValue("{s:i}", "max_node_index", num_nodes + 1);
 
     PyDict_SetItemString(ret_obj, "node_memsize", node_to_memsize_obj);
     Py_DECREF(node_to_memsize_obj);
@@ -1385,8 +1375,8 @@ static PyObject *pyxc_numainfo(XcObject *self)
     Py_DECREF(node_to_node_dist_list_obj);
 
 out:
-    xc_hypercall_buffer_free(self->xc_handle, meminfo);
-    xc_hypercall_buffer_free(self->xc_handle, distance);
+    free(meminfo);
+    free(distance);
     return ret_obj ? ret_obj : pyxc_error_to_exception(self->xc_handle);
 }
 
-- 
1.7.1

[PATCH v8 4/5] libxc: Provide set_xen_guest_handle_offset macro

[Boris Ostrovsky]

Add set_xen_guest_handle_offset() macro that can be used for setting
xen_guest_handle to an offset into hypercall buffer.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 tools/libxc/include/xenctrl.h |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 8c8eae6..4e6f62c 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -322,16 +322,21 @@ typedef struct xc_hypercall_buffer xc_hypercall_buffer_t;
  * Set a xen_guest_handle in a type safe manner, ensuring that the
  * data pointer has been correctly allocated.
  */
-#undef set_xen_guest_handle
-#define set_xen_guest_handle(_hnd, _val)                        \
+#undef set_xen_guest_handle_offset
+#define set_xen_guest_handle_offset(_hnd, _val, _off)           \
     do {                                                        \
         xc_hypercall_buffer_t _hcbuf_hnd1;                      \
         typeof(XC__HYPERCALL_BUFFER_NAME(_val)) *_hcbuf_hnd2 =  \
                 HYPERCALL_BUFFER(_val);                         \
         (void) (&_hcbuf_hnd1 == _hcbuf_hnd2);                   \
-        set_xen_guest_handle_raw(_hnd, (_hcbuf_hnd2)->hbuf);    \
+        set_xen_guest_handle_raw(_hnd,                          \
+                                 (_hcbuf_hnd2)->hbuf + (_off)); \
     } while (0)
 
+#undef set_xen_guest_handle
+#define set_xen_guest_handle(_hnd, _val) \
+    set_xen_guest_handle_offset(_hnd, _val, 0)
+
 /* Use with set_xen_guest_handle in place of NULL */
 extern xc_hypercall_buffer_t XC__HYPERCALL_BUFFER_NAME(HYPERCALL_BUFFER_NULL);
 
-- 
1.7.1

[PATCH v8 5/5] libxl: Add interface for querying hypervisor about PCI topology

[Boris Ostrovsky]

.. and use this new interface to display it along with CPU topology
and NUMA information when 'xl info -n' command is issued

The output will look like
...
cpu_topology           :
cpu:    core    socket     node
  0:       0        0        0
...
device topology        :
device           node
0000:00:00.0      0
0000:00:01.0      0
...

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
Changes in v8:
* sysctl now returns number of processed devices in num_devs and
  xc_pcitopoinfo() call sysctl again with hypercall buffer adjusted
  with set_xen_guest_handle_offset() (introduced in previous patch)
* Map hypervisor's XEN_INVALID_DEV to LIBXL_PCITOPOLOGY_INVALID_ENTRY
* Replace LOGEV with LOGE

 tools/libxc/include/xenctrl.h |    3 ++
 tools/libxc/xc_misc.c         |   39 +++++++++++++++++++++++
 tools/libxl/libxl.c           |   43 +++++++++++++++++++++++++
 tools/libxl/libxl.h           |   12 +++++++
 tools/libxl/libxl_freebsd.c   |   12 +++++++
 tools/libxl/libxl_internal.h  |    5 +++
 tools/libxl/libxl_linux.c     |   70 +++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_netbsd.c    |   12 +++++++
 tools/libxl/libxl_types.idl   |    7 ++++
 tools/libxl/libxl_utils.c     |    8 +++++
 tools/libxl/xl_cmdimpl.c      |   40 +++++++++++++++++++----
 11 files changed, 244 insertions(+), 7 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 4e6f62c..a65b62d 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1237,6 +1237,7 @@ typedef xen_sysctl_physinfo_t xc_physinfo_t;
 typedef xen_sysctl_cputopo_t xc_cputopo_t;
 typedef xen_sysctl_numainfo_t xc_numainfo_t;
 typedef xen_sysctl_meminfo_t xc_meminfo_t;
+typedef xen_sysctl_pcitopoinfo_t xc_pcitopoinfo_t;
 
 typedef uint32_t xc_cpu_to_node_t;
 typedef uint32_t xc_cpu_to_socket_t;
@@ -1250,6 +1251,8 @@ int xc_cputopoinfo(xc_interface *xch, unsigned *max_cpus,
                    xc_cputopo_t *cputopo);
 int xc_numainfo(xc_interface *xch, unsigned *max_nodes,
                 xc_meminfo_t *meminfo, uint32_t *distance);
+int xc_pcitopoinfo(xc_interface *xch, unsigned num_devs,
+                   physdev_pci_device_t *devs, uint32_t *nodes);
 
 int xc_sched_id(xc_interface *xch,
                 int *sched_id);
diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c
index 94b70b4..828e885 100644
--- a/tools/libxc/xc_misc.c
+++ b/tools/libxc/xc_misc.c
@@ -238,6 +238,45 @@ out:
     return ret;
 }
 
+int xc_pcitopoinfo(xc_interface *xch, unsigned num_devs,
+                   physdev_pci_device_t *devs,
+                   uint32_t *nodes)
+{
+    int ret = 0;
+    unsigned processed = 0;
+    DECLARE_SYSCTL;
+    DECLARE_HYPERCALL_BOUNCE(devs, num_devs * sizeof(*devs),
+                             XC_HYPERCALL_BUFFER_BOUNCE_IN);
+    DECLARE_HYPERCALL_BOUNCE(nodes, num_devs* sizeof(*nodes),
+                             XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
+
+    if ( (ret = xc_hypercall_bounce_pre(xch, devs)) )
+        goto out;
+    if ( (ret = xc_hypercall_bounce_pre(xch, nodes)) )
+        goto out;
+
+    sysctl.cmd = XEN_SYSCTL_pcitopoinfo;
+
+    while ( processed < num_devs )
+    {
+        sysctl.u.pcitopoinfo.num_devs = num_devs - processed;
+        set_xen_guest_handle_offset(sysctl.u.pcitopoinfo.devs, devs,
+                                    processed * sizeof(*devs));
+        set_xen_guest_handle_offset(sysctl.u.pcitopoinfo.nodes, nodes,
+                                    processed * sizeof(*nodes));
+
+        if ( (ret = do_sysctl(xch, &sysctl)) != 0 )
+                break;
+
+        processed += sysctl.u.pcitopoinfo.num_devs;
+    }
+
+ out:
+    xc_hypercall_bounce_post(xch, devs);
+    xc_hypercall_bounce_post(xch, nodes);
+
+    return ret;
+}
 
 int xc_sched_id(xc_interface *xch,
                 int *sched_id)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 5c88a88..f2a0136 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5139,6 +5139,49 @@ libxl_cputopology *libxl_get_cpu_topology(libxl_ctx *ctx, int *nb_cpu_out)
     return ret;
 }
 
+libxl_pcitopology *libxl_get_pci_topology(libxl_ctx *ctx, int *num_devs)
+{
+    GC_INIT(ctx);
+    physdev_pci_device_t *devs;
+    uint32_t *nodes;
+    libxl_pcitopology *ret = NULL;
+    int i;
+
+    *num_devs = libxl__pci_numdevs(gc);
+    if (*num_devs < 0) {
+        LOGE(ERROR, "Unable to determine number of PCI devices");
+        goto out;
+    }
+
+    devs = libxl__zalloc(gc, sizeof(*devs) * *num_devs);
+    nodes = libxl__zalloc(gc, sizeof(*nodes) * *num_devs);
+
+    if (libxl__pci_topology_init(gc, devs, *num_devs)) {
+        LOGE(ERROR, "Cannot initialize PCI hypercall structure");
+        goto out;
+    }
+
+    if (xc_pcitopoinfo(ctx->xch, *num_devs, devs, nodes) != 0) {
+        LOGE(ERROR, "PCI topology info hypercall failed");
+        goto out;
+    }
+
+    ret = libxl__zalloc(NOGC, sizeof(libxl_pcitopology) * *num_devs);
+
+    for (i = 0; i < *num_devs; i++) {
+        ret[i].seg = devs[i].seg;
+        ret[i].bus = devs[i].bus;
+        ret[i].devfn = devs[i].devfn;
+        ret[i].node = ((nodes[i] == XEN_INVALID_NODE_ID) ||
+                       (nodes[i] == XEN_INVALID_DEV)) ?
+            LIBXL_PCITOPOLOGY_INVALID_ENTRY : nodes[i];
+    }
+
+ out:
+    GC_FREE;
+    return ret;
+}
+
 libxl_numainfo *libxl_get_numainfo(libxl_ctx *ctx, int *nr)
 {
     GC_INIT(ctx);
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 44bd8e2..2b50f8e 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -750,6 +750,14 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, libxl_mac *src);
 #define LIBXL_HAVE_PSR_MBM 1
 #endif
 
+/*
+ * LIBXL_HAVE_PCITOPOLOGY
+ *
+ * If this is defined, then interface to query hypervisor about PCI device
+ * topology is available.
+ */
+#define LIBXL_HAVE_PCITOPOLOGY 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);
@@ -1142,6 +1150,10 @@ void libxl_vminfo_list_free(libxl_vminfo *list, int nb_vm);
 libxl_cputopology *libxl_get_cpu_topology(libxl_ctx *ctx, int *nb_cpu_out);
 void libxl_cputopology_list_free(libxl_cputopology *, int nb_cpu);
 
+#define LIBXL_PCITOPOLOGY_INVALID_ENTRY (~(uint32_t)0)
+libxl_pcitopology *libxl_get_pci_topology(libxl_ctx *ctx, int *num_devs);
+void libxl_pcitopology_list_free(libxl_pcitopology *, int num_devs);
+
 #define LIBXL_NUMAINFO_INVALID_ENTRY (~(uint32_t)0)
 libxl_numainfo *libxl_get_numainfo(libxl_ctx *ctx, int *nr);
 void libxl_numainfo_list_free(libxl_numainfo *, int nr);
diff --git a/tools/libxl/libxl_freebsd.c b/tools/libxl/libxl_freebsd.c
index e8b88b3..47c3391 100644
--- a/tools/libxl/libxl_freebsd.c
+++ b/tools/libxl/libxl_freebsd.c
@@ -131,3 +131,15 @@ libxl_device_model_version libxl__default_device_model(libxl__gc *gc)
 {
     return LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN;
 }
+
+int libxl__pci_numdevs(libxl__gc *gc)
+{
+    return ERROR_NI;
+}
+
+int libxl__pci_topology_init(libxl__gc *gc,
+                             physdev_pci_device_t *devs,
+                             int num_devs)
+{
+    return ERROR_NI;
+}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 8eb38aa..8aaa1ad 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1196,6 +1196,11 @@ _hidden int libxl__try_phy_backend(mode_t st_mode);
 
 _hidden char *libxl__devid_to_localdev(libxl__gc *gc, int devid);
 
+_hidden int libxl__pci_numdevs(libxl__gc *gc);
+_hidden int libxl__pci_topology_init(libxl__gc *gc,
+                                     physdev_pci_device_t *devs,
+                                     int num_devs);
+
 /* from libxl_pci */
 
 _hidden int libxl__device_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev, int starting);
diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
index b51930c..f42a89a 100644
--- a/tools/libxl/libxl_linux.c
+++ b/tools/libxl/libxl_linux.c
@@ -279,3 +279,73 @@ libxl_device_model_version libxl__default_device_model(libxl__gc *gc)
 {
     return LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN;
 }
+
+int libxl__pci_numdevs(libxl__gc *gc)
+{
+    DIR *dir;
+    struct dirent *entry;
+    int num_devs = 0;
+
+    dir = opendir("/sys/bus/pci/devices");
+    if (!dir) {
+        LOGE(ERROR, "Cannot open /sys/bus/pci/devices");
+        return ERROR_FAIL;
+    }
+
+    while ((entry = readdir(dir))) {
+        if (entry->d_name[0] == '.')
+            continue;
+        num_devs++;
+    }
+    closedir(dir);
+
+    return num_devs;
+}
+
+int libxl__pci_topology_init(libxl__gc *gc,
+                             physdev_pci_device_t *devs,
+                             int num_devs)
+{
+
+    DIR *dir;
+    struct dirent *entry;
+    int i, err = 0;
+
+    dir = opendir("/sys/bus/pci/devices");
+    if (!dir) {
+        LOGE(ERROR, "Cannot open /sys/bus/pci/devices");
+        return ERROR_FAIL;
+    }
+
+    i = 0;
+    while ((entry = readdir(dir))) {
+        unsigned int dom, bus, dev, func;
+
+        if (entry->d_name[0] == '.')
+            continue;
+
+        if (i == num_devs) {
+            LOG(ERROR, "Too many devices");
+            err = ERROR_FAIL;
+            errno = -ENOSPC;
+            goto out;
+        }
+
+        if (sscanf(entry->d_name, "%x:%x:%x.%d", &dom, &bus, &dev, &func) < 4) {
+            LOGE(ERROR, "Error processing /sys/bus/pci/devices");
+            err = ERROR_FAIL;
+            goto out;
+        }
+
+        devs[i].seg = dom;
+        devs[i].bus = bus;
+        devs[i].devfn = ((dev & 0x1f) << 3) | (func & 7);
+
+        i++;
+    }
+
+ out:
+    closedir(dir);
+
+    return err;
+}
diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
index 898e160..a2a962e 100644
--- a/tools/libxl/libxl_netbsd.c
+++ b/tools/libxl/libxl_netbsd.c
@@ -95,3 +95,15 @@ libxl_device_model_version libxl__default_device_model(libxl__gc *gc)
 {
     return LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL;
 }
+
+int libxl__pci_numdevs(libxl__gc *gc)
+{
+    return ERROR_NI;
+}
+
+int libxl__pci_topology_init(libxl__gc *gc,
+                             physdev_pci_device_t *devs,
+                             int num_devs)
+{
+    return ERROR_NI;
+}
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 117b61d..3d7d46a 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -656,6 +656,13 @@ libxl_cputopology = Struct("cputopology", [
     ("node", uint32),
     ], dir=DIR_OUT)
 
+libxl_pcitopology = Struct("pcitopology", [
+    ("seg", uint16),
+    ("bus", uint8),
+    ("devfn", uint8),
+    ("node", uint32),
+    ], dir=DIR_OUT)
+
 libxl_sched_credit_params = Struct("sched_credit_params", [
     ("tslice_ms", integer),
     ("ratelimit_us", integer),
diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index 67c0b1c..f6be2d7 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -952,6 +952,14 @@ void libxl_cputopology_list_free(libxl_cputopology *list, int nr)
     free(list);
 }
 
+void libxl_pcitopology_list_free(libxl_pcitopology *list, int nr)
+{
+    int i;
+    for (i = 0; i < nr; i++)
+        libxl_pcitopology_dispose(&list[i]);
+    free(list);
+}
+
 void libxl_numainfo_list_free(libxl_numainfo *list, int nr)
 {
     int i;
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 648ca08..3990dd4 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5418,12 +5418,15 @@ static void output_numainfo(void)
 
 static void output_topologyinfo(void)
 {
-    libxl_cputopology *info;
+    libxl_cputopology *cpuinfo;
     int i, nr;
+    libxl_pcitopology *pciinfo;
+    int valid_devs;
 
-    info = libxl_get_cpu_topology(ctx, &nr);
-    if (info == NULL) {
-        fprintf(stderr, "libxl_get_topologyinfo failed.\n");
+
+    cpuinfo = libxl_get_cpu_topology(ctx, &nr);
+    if (cpuinfo == NULL) {
+        fprintf(stderr, "libxl_get_cpu_topology failed.\n");
         return;
     }
 
@@ -5431,12 +5434,35 @@ static void output_topologyinfo(void)
     printf("cpu:    core    socket     node\n");
 
     for (i = 0; i < nr; i++) {
-        if (info[i].core != LIBXL_CPUTOPOLOGY_INVALID_ENTRY)
+        if (cpuinfo[i].core != LIBXL_CPUTOPOLOGY_INVALID_ENTRY)
             printf("%3d:    %4d     %4d     %4d\n", i,
-                   info[i].core, info[i].socket, info[i].node);
+                   cpuinfo[i].core, cpuinfo[i].socket, cpuinfo[i].node);
+    }
+
+    libxl_cputopology_list_free(cpuinfo, nr);
+
+    pciinfo = libxl_get_pci_topology(ctx, &nr);
+    if (pciinfo == NULL) {
+        fprintf(stderr, "libxl_get_pci_topology failed.\n");
+        return;
     }
 
-    libxl_cputopology_list_free(info, nr);
+    printf("device topology        :\n");
+    printf("device           node\n");
+    for (i = 0; i < nr; i++) {
+        if (pciinfo[i].node != LIBXL_PCITOPOLOGY_INVALID_ENTRY) {
+            printf("%04x:%02x:%02x.%01x      %d\n", pciinfo[i].seg,
+                   pciinfo[i].bus,
+                   ((pciinfo[i].devfn >> 3) & 0x1f), (pciinfo[i].devfn & 7),
+                   pciinfo[i].node);
+            valid_devs++;
+        }
+    }
+
+    if (valid_devs == 0)
+        printf("No device topology data available\n");
+
+    libxl_pcitopology_list_free(pciinfo, nr);
 
     return;
 }
-- 
1.7.1

Re: [PATCH v8 1/5] sysctl: Add sysctl interface for querying PCI topology

[Andrew Cooper]

On 06/05/2015 19:14, Boris Ostrovsky wrote:
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH v8 4/5] libxc: Provide set_xen_guest_handle_offset macro

[Ian Campbell]

On Wed, 2015-05-06 at 14:15 -0400, Boris Ostrovsky wrote:
> Add set_xen_guest_handle_offset() macro that can be used for setting
> xen_guest_handle to an offset into hypercall buffer.
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---
>  tools/libxc/include/xenctrl.h |   11 ++++++++---
>  1 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index 8c8eae6..4e6f62c 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -322,16 +322,21 @@ typedef struct xc_hypercall_buffer xc_hypercall_buffer_t;
>   * Set a xen_guest_handle in a type safe manner, ensuring that the
>   * data pointer has been correctly allocated.
>   */
> -#undef set_xen_guest_handle
> -#define set_xen_guest_handle(_hnd, _val)                        \
> +#undef set_xen_guest_handle_offset

This isn't strictly speaking needed since you aren't trying to override
a version of this interface provided by the hypervisor headers.

> +#define set_xen_guest_handle_offset(_hnd, _val, _off)           \
>      do {                                                        \
>          xc_hypercall_buffer_t _hcbuf_hnd1;                      \
>          typeof(XC__HYPERCALL_BUFFER_NAME(_val)) *_hcbuf_hnd2 =  \
>                  HYPERCALL_BUFFER(_val);                         \
>          (void) (&_hcbuf_hnd1 == _hcbuf_hnd2);                   \
> -        set_xen_guest_handle_raw(_hnd, (_hcbuf_hnd2)->hbuf);    \
> +        set_xen_guest_handle_raw(_hnd,                          \
> +                                 (_hcbuf_hnd2)->hbuf + (_off)); \

The hypervisor side guest_handle_add_offset equivalents operate in
multiples of the type. If we can arrange that here too then I think that
would be good.

I think that means 
+                                 (_hcbuf_hnd2)->hbuf + ((sizeof(*_val)*(_off))); \

Then the caller becomes e.g.

set_xen_guest_handle_offset(sysctl.u.pcitopoinfo.devs, devs, processed);

which is much nicer IMHO.

Ian.

Re: [PATCH v8 5/5] libxl: Add interface for querying hypervisor about PCI topology

[Ian Campbell]

On Wed, 2015-05-06 at 14:15 -0400, Boris Ostrovsky wrote:
> .. and use this new interface to display it along with CPU topology
> and NUMA information when 'xl info -n' command is issued
> 
> The output will look like
> ...
> cpu_topology           :
> cpu:    core    socket     node
>   0:       0        0        0
> ...
> device topology        :
> device           node
> 0000:00:00.0      0
> 0000:00:01.0      0
> ...
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[...]
> +        sysctl.u.pcitopoinfo.num_devs = num_devs - processed;
> +        set_xen_guest_handle_offset(sysctl.u.pcitopoinfo.devs, devs,
> +                                    processed * sizeof(*devs));
> +        set_xen_guest_handle_offset(sysctl.u.pcitopoinfo.nodes, nodes,
> +                                    processed * sizeof(*nodes));

Changing these in the obvious way based on my feedback to the previous
patch wouldn't invalidate my ack.

[...]
> +    if (libxl__pci_topology_init(gc, devs, *num_devs)) {
> +        LOGE(ERROR, "Cannot initialize PCI hypercall structure");

libxl__pci_topology_init might be an exception but libxl__* functions
don't typically set errno, they return ERROR_FOO things. So you may want
just LOG and not LOGE. You might also want to arrange to print the error
code.

Ian.

Re: [PATCH v8 4/5] libxc: Provide set_xen_guest_handle_offset macro

[Boris Ostrovsky]

On 05/08/2015 10:57 AM, Ian Campbell wrote:
> On Wed, 2015-05-06 at 14:15 -0400, Boris Ostrovsky wrote:
>> Add set_xen_guest_handle_offset() macro that can be used for setting
>> xen_guest_handle to an offset into hypercall buffer.
>>
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> ---
>>   tools/libxc/include/xenctrl.h |   11 ++++++++---
>>   1 files changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
>> index 8c8eae6..4e6f62c 100644
>> --- a/tools/libxc/include/xenctrl.h
>> +++ b/tools/libxc/include/xenctrl.h
>> @@ -322,16 +322,21 @@ typedef struct xc_hypercall_buffer xc_hypercall_buffer_t;
>>    * Set a xen_guest_handle in a type safe manner, ensuring that the
>>    * data pointer has been correctly allocated.
>>    */
>> -#undef set_xen_guest_handle
>> -#define set_xen_guest_handle(_hnd, _val)                        \
>> +#undef set_xen_guest_handle_offset
> This isn't strictly speaking needed since you aren't trying to override
> a version of this interface provided by the hypervisor headers.
>
>> +#define set_xen_guest_handle_offset(_hnd, _val, _off)           \
>>       do {                                                        \
>>           xc_hypercall_buffer_t _hcbuf_hnd1;                      \
>>           typeof(XC__HYPERCALL_BUFFER_NAME(_val)) *_hcbuf_hnd2 =  \
>>                   HYPERCALL_BUFFER(_val);                         \
>>           (void) (&_hcbuf_hnd1 == _hcbuf_hnd2);                   \
>> -        set_xen_guest_handle_raw(_hnd, (_hcbuf_hnd2)->hbuf);    \
>> +        set_xen_guest_handle_raw(_hnd,                          \
>> +                                 (_hcbuf_hnd2)->hbuf + (_off)); \
> The hypervisor side guest_handle_add_offset equivalents operate in
> multiples of the type. If we can arrange that here too then I think that
> would be good.
>
> I think that means
> +                                 (_hcbuf_hnd2)->hbuf + ((sizeof(*_val)*(_off))); \
>
> Then the caller becomes e.g.
>
> set_xen_guest_handle_offset(sysctl.u.pcitopoinfo.devs, devs, processed);
>
> which is much nicer IMHO.


That's what I started with.

This, however, breaks at least two things:
--  Callers that pass HYPERCALL_BUFFER_NULL to set_xen_guest_handle(). 
This I think can be fixed by declaring a void  *HYPERCALL_BUFFER_NULL 
(currenty HYPERCALL_BUFFER_NULL is just a syntactic building block)

-- DECLARE_NAMED_HYPERCALL_BOUNCE. This makes _val a non-existing symbol 
(it creates a xc__hypercall_buffer_val). That I am not sure I know how 
to fix. Add type size filed to struct xc_hypercall_buffer?

-boris

Re: [PATCH v8 4/5] libxc: Provide set_xen_guest_handle_offset macro

[Ian Campbell]

On Fri, 2015-05-08 at 11:41 -0400, Boris Ostrovsky wrote:
> On 05/08/2015 10:57 AM, Ian Campbell wrote:
> > On Wed, 2015-05-06 at 14:15 -0400, Boris Ostrovsky wrote:
> >> Add set_xen_guest_handle_offset() macro that can be used for setting
> >> xen_guest_handle to an offset into hypercall buffer.
> >>
> >> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> >> ---
> >>   tools/libxc/include/xenctrl.h |   11 ++++++++---
> >>   1 files changed, 8 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> >> index 8c8eae6..4e6f62c 100644
> >> --- a/tools/libxc/include/xenctrl.h
> >> +++ b/tools/libxc/include/xenctrl.h
> >> @@ -322,16 +322,21 @@ typedef struct xc_hypercall_buffer xc_hypercall_buffer_t;
> >>    * Set a xen_guest_handle in a type safe manner, ensuring that the
> >>    * data pointer has been correctly allocated.
> >>    */
> >> -#undef set_xen_guest_handle
> >> -#define set_xen_guest_handle(_hnd, _val)                        \
> >> +#undef set_xen_guest_handle_offset
> > This isn't strictly speaking needed since you aren't trying to override
> > a version of this interface provided by the hypervisor headers.
> >
> >> +#define set_xen_guest_handle_offset(_hnd, _val, _off)           \
> >>       do {                                                        \
> >>           xc_hypercall_buffer_t _hcbuf_hnd1;                      \
> >>           typeof(XC__HYPERCALL_BUFFER_NAME(_val)) *_hcbuf_hnd2 =  \
> >>                   HYPERCALL_BUFFER(_val);                         \
> >>           (void) (&_hcbuf_hnd1 == _hcbuf_hnd2);                   \
> >> -        set_xen_guest_handle_raw(_hnd, (_hcbuf_hnd2)->hbuf);    \
> >> +        set_xen_guest_handle_raw(_hnd,                          \
> >> +                                 (_hcbuf_hnd2)->hbuf + (_off)); \
> > The hypervisor side guest_handle_add_offset equivalents operate in
> > multiples of the type. If we can arrange that here too then I think that
> > would be good.
> >
> > I think that means
> > +                                 (_hcbuf_hnd2)->hbuf + ((sizeof(*_val)*(_off))); \
> >
> > Then the caller becomes e.g.
> >
> > set_xen_guest_handle_offset(sysctl.u.pcitopoinfo.devs, devs, processed);
> >
> > which is much nicer IMHO.
> 
> 
> That's what I started with.
> 
> This, however, breaks at least two things:
> --  Callers that pass HYPERCALL_BUFFER_NULL to set_xen_guest_handle(). 
> This I think can be fixed by declaring a void  *HYPERCALL_BUFFER_NULL 
> (currenty HYPERCALL_BUFFER_NULL is just a syntactic building block)
> 
> -- DECLARE_NAMED_HYPERCALL_BOUNCE. This makes _val a non-existing symbol 
> (it creates a xc__hypercall_buffer_val). That I am not sure I know how 
> to fix. Add type size filed to struct xc_hypercall_buffer?

How about refactoring such that this macro becomes an internal helper
used by both set_xen_guest_handle and set_xen_guest_handle_offset and
takes the (_hcbuf_hnd2)->hbuf or (_hcbuf_hnd2)->hbuf +
((sizeof(*_val)*(_off)) as appropriate? (iow don't implement set as
set_offset(0))

Re: [PATCH v8 4/5] libxc: Provide set_xen_guest_handle_offset macro

[Boris Ostrovsky]

On 05/08/2015 11:56 AM, Ian Campbell wrote:
> On Fri, 2015-05-08 at 11:41 -0400, Boris Ostrovsky wrote:
>> On 05/08/2015 10:57 AM, Ian Campbell wrote:
>>> On Wed, 2015-05-06 at 14:15 -0400, Boris Ostrovsky wrote:
>>>> Add set_xen_guest_handle_offset() macro that can be used for setting
>>>> xen_guest_handle to an offset into hypercall buffer.
>>>>
>>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>> ---
>>>>    tools/libxc/include/xenctrl.h |   11 ++++++++---
>>>>    1 files changed, 8 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
>>>> index 8c8eae6..4e6f62c 100644
>>>> --- a/tools/libxc/include/xenctrl.h
>>>> +++ b/tools/libxc/include/xenctrl.h
>>>> @@ -322,16 +322,21 @@ typedef struct xc_hypercall_buffer xc_hypercall_buffer_t;
>>>>     * Set a xen_guest_handle in a type safe manner, ensuring that the
>>>>     * data pointer has been correctly allocated.
>>>>     */
>>>> -#undef set_xen_guest_handle
>>>> -#define set_xen_guest_handle(_hnd, _val)                        \
>>>> +#undef set_xen_guest_handle_offset
>>> This isn't strictly speaking needed since you aren't trying to override
>>> a version of this interface provided by the hypervisor headers.
>>>
>>>> +#define set_xen_guest_handle_offset(_hnd, _val, _off)           \
>>>>        do {                                                        \
>>>>            xc_hypercall_buffer_t _hcbuf_hnd1;                      \
>>>>            typeof(XC__HYPERCALL_BUFFER_NAME(_val)) *_hcbuf_hnd2 =  \
>>>>                    HYPERCALL_BUFFER(_val);                         \
>>>>            (void) (&_hcbuf_hnd1 == _hcbuf_hnd2);                   \
>>>> -        set_xen_guest_handle_raw(_hnd, (_hcbuf_hnd2)->hbuf);    \
>>>> +        set_xen_guest_handle_raw(_hnd,                          \
>>>> +                                 (_hcbuf_hnd2)->hbuf + (_off)); \
>>> The hypervisor side guest_handle_add_offset equivalents operate in
>>> multiples of the type. If we can arrange that here too then I think that
>>> would be good.
>>>
>>> I think that means
>>> +                                 (_hcbuf_hnd2)->hbuf + ((sizeof(*_val)*(_off))); \
>>>
>>> Then the caller becomes e.g.
>>>
>>> set_xen_guest_handle_offset(sysctl.u.pcitopoinfo.devs, devs, processed);
>>>
>>> which is much nicer IMHO.
>>
>> That's what I started with.
>>
>> This, however, breaks at least two things:
>> --  Callers that pass HYPERCALL_BUFFER_NULL to set_xen_guest_handle().
>> This I think can be fixed by declaring a void  *HYPERCALL_BUFFER_NULL
>> (currenty HYPERCALL_BUFFER_NULL is just a syntactic building block)
>>
>> -- DECLARE_NAMED_HYPERCALL_BOUNCE. This makes _val a non-existing symbol
>> (it creates a xc__hypercall_buffer_val). That I am not sure I know how
>> to fix. Add type size filed to struct xc_hypercall_buffer?
> How about refactoring such that this macro becomes an internal helper
> used by both set_xen_guest_handle and set_xen_guest_handle_offset and
> takes the (_hcbuf_hnd2)->hbuf or (_hcbuf_hnd2)->hbuf +
> ((sizeof(*_val)*(_off)) as appropriate? (iow don't implement set as
> set_offset(0))

But that will preclude us from using _offset() variant of the macro for 
variables declared with DECLARE_NAMED_HYPERCALL_BOUNCE (and possibly 
others, I haven't checked), won't it? We don't have any such uses now 
but nevertheless...

-boris

Re: [PATCH v8 4/5] libxc: Provide set_xen_guest_handle_offset macro

[Ian Campbell]

On Fri, 2015-05-08 at 12:05 -0400, Boris Ostrovsky wrote:
> On 05/08/2015 11:56 AM, Ian Campbell wrote:
> > On Fri, 2015-05-08 at 11:41 -0400, Boris Ostrovsky wrote:
> >> On 05/08/2015 10:57 AM, Ian Campbell wrote:
> >>> On Wed, 2015-05-06 at 14:15 -0400, Boris Ostrovsky wrote:
> >>>> Add set_xen_guest_handle_offset() macro that can be used for setting
> >>>> xen_guest_handle to an offset into hypercall buffer.
> >>>>
> >>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> >>>> ---
> >>>>    tools/libxc/include/xenctrl.h |   11 ++++++++---
> >>>>    1 files changed, 8 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> >>>> index 8c8eae6..4e6f62c 100644
> >>>> --- a/tools/libxc/include/xenctrl.h
> >>>> +++ b/tools/libxc/include/xenctrl.h
> >>>> @@ -322,16 +322,21 @@ typedef struct xc_hypercall_buffer xc_hypercall_buffer_t;
> >>>>     * Set a xen_guest_handle in a type safe manner, ensuring that the
> >>>>     * data pointer has been correctly allocated.
> >>>>     */
> >>>> -#undef set_xen_guest_handle
> >>>> -#define set_xen_guest_handle(_hnd, _val)                        \
> >>>> +#undef set_xen_guest_handle_offset
> >>> This isn't strictly speaking needed since you aren't trying to override
> >>> a version of this interface provided by the hypervisor headers.
> >>>
> >>>> +#define set_xen_guest_handle_offset(_hnd, _val, _off)           \
> >>>>        do {                                                        \
> >>>>            xc_hypercall_buffer_t _hcbuf_hnd1;                      \
> >>>>            typeof(XC__HYPERCALL_BUFFER_NAME(_val)) *_hcbuf_hnd2 =  \
> >>>>                    HYPERCALL_BUFFER(_val);                         \
> >>>>            (void) (&_hcbuf_hnd1 == _hcbuf_hnd2);                   \
> >>>> -        set_xen_guest_handle_raw(_hnd, (_hcbuf_hnd2)->hbuf);    \
> >>>> +        set_xen_guest_handle_raw(_hnd,                          \
> >>>> +                                 (_hcbuf_hnd2)->hbuf + (_off)); \
> >>> The hypervisor side guest_handle_add_offset equivalents operate in
> >>> multiples of the type. If we can arrange that here too then I think that
> >>> would be good.
> >>>
> >>> I think that means
> >>> +                                 (_hcbuf_hnd2)->hbuf + ((sizeof(*_val)*(_off))); \
> >>>
> >>> Then the caller becomes e.g.
> >>>
> >>> set_xen_guest_handle_offset(sysctl.u.pcitopoinfo.devs, devs, processed);
> >>>
> >>> which is much nicer IMHO.
> >>
> >> That's what I started with.
> >>
> >> This, however, breaks at least two things:
> >> --  Callers that pass HYPERCALL_BUFFER_NULL to set_xen_guest_handle().
> >> This I think can be fixed by declaring a void  *HYPERCALL_BUFFER_NULL
> >> (currenty HYPERCALL_BUFFER_NULL is just a syntactic building block)
> >>
> >> -- DECLARE_NAMED_HYPERCALL_BOUNCE. This makes _val a non-existing symbol
> >> (it creates a xc__hypercall_buffer_val). That I am not sure I know how
> >> to fix. Add type size filed to struct xc_hypercall_buffer?
> > How about refactoring such that this macro becomes an internal helper
> > used by both set_xen_guest_handle and set_xen_guest_handle_offset and
> > takes the (_hcbuf_hnd2)->hbuf or (_hcbuf_hnd2)->hbuf +
> > ((sizeof(*_val)*(_off)) as appropriate? (iow don't implement set as
> > set_offset(0))
> 
> But that will preclude us from using _offset() variant of the macro for 
> variables declared with DECLARE_NAMED_HYPERCALL_BOUNCE (and possibly 
> others, I haven't checked), won't it? We don't have any such uses now 
> but nevertheless...

Lets cross that bridge if/when we come to it.

Ian.