[PATCH RFC 0/2] Support for hwloc

[PATCH RFC 0/2] Support for hwloc

[Andrew Cooper]

These two patches have been developed in combination with a Xen pluging for
hwloc, to allow a toolstack domain to gather the system topology rather than
the virtual topology.

An experimental hwloc branch can be found here:

   git://xenbits.xen.org/people/andrewcoop/hwloc.git hwloc-xen-topology-v5
   http://xenbits.xen.org/gitweb/?p=people/andrewcoop/hwloc.git;a=shortlog;h=refs/heads/hwloc-xen-topology-v5

and depends on these two patches to function.

Patch 1 is extending xc_{topology,numa}info() functions to include a version
which performs correct hypercall bounce buffering.

Patch 2 introduced SYSCTL_xen_cpuid which allows a toolstack to perform
arbitrary cpuid instructions on specific physical cpus.  hwloc uses this to
enumerate the cpuid cache leaves without resorting to tactics of pinning vcpus
to specific pcpus and running native cpuid.

Comments welcome,

~Andrew

[PATCH RFC 1/2] tools/libxc: Improved xc_{topology, numa}info functions.

[Andrew Cooper]

These two new functions provide a substantially easier-to-use API, where libxc
itself takes care of all the appropriate bounce buffering.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxc/xc_misc.c |   85 +++++++++++++++++++++++++++++++++++++++++++++++++
 tools/libxc/xenctrl.h |   49 ++++++++++++++++++++++++++++
 2 files changed, 134 insertions(+)

diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c
index 3303454..4f672ce 100644
--- a/tools/libxc/xc_misc.c
+++ b/tools/libxc/xc_misc.c
@@ -195,6 +195,46 @@ int xc_topologyinfo(xc_interface *xch,
     return 0;
 }
 
+int xc_topologyinfo_bounced(xc_interface *xch,
+                            uint32_t *max_cpu_index,
+                            uint32_t *cpu_to_core,
+                            uint32_t *cpu_to_socket,
+                            uint32_t *cpu_to_node)
+{
+    int ret = -1;
+    size_t sz = sizeof(uint32_t) * (*max_cpu_index + 1);
+
+    DECLARE_HYPERCALL_BOUNCE(cpu_to_core, sz, XC_HYPERCALL_BUFFER_BOUNCE_OUT);
+    DECLARE_HYPERCALL_BOUNCE(cpu_to_socket, sz, XC_HYPERCALL_BUFFER_BOUNCE_OUT);
+    DECLARE_HYPERCALL_BOUNCE(cpu_to_node, sz, XC_HYPERCALL_BUFFER_BOUNCE_OUT);
+    DECLARE_SYSCTL;
+
+    if ( xc_hypercall_bounce_pre(xch, cpu_to_core) ||
+         xc_hypercall_bounce_pre(xch, cpu_to_socket) ||
+         xc_hypercall_bounce_pre(xch, cpu_to_node) )
+        goto out;
+
+    sysctl.cmd = XEN_SYSCTL_topologyinfo;
+    sysctl.u.topologyinfo.max_cpu_index = *max_cpu_index;
+
+    set_xen_guest_handle(sysctl.u.topologyinfo.cpu_to_core, cpu_to_core);
+    set_xen_guest_handle(sysctl.u.topologyinfo.cpu_to_socket, cpu_to_socket);
+    set_xen_guest_handle(sysctl.u.topologyinfo.cpu_to_node, cpu_to_node);
+
+    ret = do_sysctl(xch, &sysctl);
+
+    if ( ret )
+        goto out;
+
+    *max_cpu_index = sysctl.u.topologyinfo.max_cpu_index;
+
+out:
+    xc_hypercall_bounce_post(xch, cpu_to_node);
+    xc_hypercall_bounce_post(xch, cpu_to_socket);
+    xc_hypercall_bounce_post(xch, cpu_to_core);
+    return ret;
+}
+
 int xc_numainfo(xc_interface *xch,
                 xc_numainfo_t *put_info)
 {
@@ -213,6 +253,51 @@ int xc_numainfo(xc_interface *xch,
     return 0;
 }
 
+int xc_numainfo_bounced(xc_interface *xch,
+                        uint32_t *max_node_index,
+                        uint64_t *node_to_memsize,
+                        uint64_t *node_to_memfree,
+                        uint32_t *node_to_node_distance)
+{
+    int ret = -1;
+    size_t mem_sz = sizeof(uint64_t) * (*max_node_index + 1);
+    size_t distance_sz = (sizeof(uint32_t) * (*max_node_index + 1) *
+                          (*max_node_index + 1));
+
+    DECLARE_HYPERCALL_BOUNCE(node_to_memsize, mem_sz,
+                             XC_HYPERCALL_BUFFER_BOUNCE_OUT);
+    DECLARE_HYPERCALL_BOUNCE(node_to_memfree, mem_sz,
+                             XC_HYPERCALL_BUFFER_BOUNCE_OUT);
+    DECLARE_HYPERCALL_BOUNCE(node_to_node_distance, distance_sz,
+                             XC_HYPERCALL_BUFFER_BOUNCE_OUT);
+    DECLARE_SYSCTL;
+
+    if ( xc_hypercall_bounce_pre(xch, node_to_memsize) ||
+         xc_hypercall_bounce_pre(xch, node_to_memfree) ||
+         xc_hypercall_bounce_pre(xch, node_to_node_distance) )
+        goto out;
+
+    sysctl.cmd = XEN_SYSCTL_numainfo;
+    sysctl.u.numainfo.max_node_index = *max_node_index;
+
+    set_xen_guest_handle(sysctl.u.numainfo.node_to_memsize, node_to_memsize);
+    set_xen_guest_handle(sysctl.u.numainfo.node_to_memfree, node_to_memfree);
+    set_xen_guest_handle(sysctl.u.numainfo.node_to_node_distance,
+                         node_to_node_distance);
+
+    ret = do_sysctl(xch, &sysctl);
+
+    if ( ret )
+        goto out;
+
+    *max_node_index = sysctl.u.numainfo.max_node_index;
+
+out:
+    xc_hypercall_bounce_post(xch, node_to_node_distance);
+    xc_hypercall_bounce_post(xch, node_to_memfree);
+    xc_hypercall_bounce_post(xch, node_to_memsize);
+    return ret;
+}
 
 int xc_sched_id(xc_interface *xch,
                 int *sched_id)
diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
index 13f816b..50126ae 100644
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -1144,9 +1144,58 @@ 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);
+
+/* Query Xen for the cpu topology information.  The caller is responsible for
+ * ensuring correct hypercall buffering. */
 int xc_topologyinfo(xc_interface *xch, xc_topologyinfo_t *info);
+
+/**
+ * Query Xen for the cpu topology information.  The library shall ensure
+ * correct bounce buffering is performed.
+ *
+ * The following parameters behave exactly as described in Xen's public
+ * sysctl.h.  Arrays may be NULL if the information is not wanted.
+ *
+ * Each array should have (max_cpu_index + 1) elements.
+ *
+ * @param [in/out] max_cpu_index
+ * @param [out] cpu_to_core
+ * @param [out] cpu_to_socket
+ * @param [out] cpu_to_node
+ * @returns 0 on success, -1 and sets errno on error.
+ */
+int xc_topologyinfo_bounced(xc_interface *xch,
+                            uint32_t *max_cpu_index,
+                            uint32_t *cpu_to_core,
+                            uint32_t *cpu_to_socket,
+                            uint32_t *cpu_to_node);
+
+/* Query Xen for the memory NUMA information.  The caller is responsible for
+ * ensuring correct hypercall buffering. */
 int xc_numainfo(xc_interface *xch, xc_numainfo_t *info);
 
+/**
+ * Query Xen for the memory NUMA information.  The library shall ensure
+ * correct bounce buffering is performed.
+ *
+ * The following parameters behave exactly as described in Xen's public
+ * sysctl.h.  Arrays may be NULL if the information is not wanted.
+ *
+ * node_to_mem{size,free} should have (max_node_index + 1) elements
+ * node_to_node_distance should have (max_node_index + 1)^2 elements
+ *
+ * @param [in/out] max_node_index
+ * @param [out] node_to_memsize
+ * @param [out] node_to_memfree
+ * @param [out] node_to_node_distance
+ * @returns 0 on success, -1 and sets errno on error.
+ */
+int xc_numainfo_bounced(xc_interface *xch,
+                        uint32_t *max_node_index,
+                        uint64_t *node_to_memsize,
+                        uint64_t *node_to_memfree,
+                        uint32_t *node_to_node_distance);
+
 int xc_sched_id(xc_interface *xch,
                 int *sched_id);
 
-- 
1.7.10.4

[PATCH RFC 2/2] SYSCTL subop to execute cpuid on a specified pcpu

[Andrew Cooper]

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
 tools/libxc/xc_misc.c       |   25 +++++++++++++++++++++++++
 tools/libxc/xenctrl.h       |    7 +++++++
 xen/arch/x86/sysctl.c       |   30 ++++++++++++++++++++++++++++++
 xen/include/public/sysctl.h |    9 +++++++++
 4 files changed, 71 insertions(+)

diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c
index 4f672ce..48ef33e 100644
--- a/tools/libxc/xc_misc.c
+++ b/tools/libxc/xc_misc.c
@@ -774,6 +774,31 @@ int xc_hvm_inject_trap(
     return rc;
 }
 
+int xc_xen_cpuid(xc_interface *xch, uint32_t cpu, uint32_t *eax,
+                 uint32_t *ebx, uint32_t *ecx, uint32_t *edx)
+{
+    int ret;
+    DECLARE_SYSCTL;
+
+    sysctl.cmd = XEN_SYSCTL_cpuid;
+    sysctl.u.cpuid.cpu = cpu;
+    sysctl.u.cpuid.eax = *eax;
+    sysctl.u.cpuid.ebx = *ebx;
+    sysctl.u.cpuid.ecx = *ecx;
+    sysctl.u.cpuid.edx = *edx;
+
+    ret = do_sysctl(xch, &sysctl);
+    if ( ret )
+        return ret;
+
+    *eax = sysctl.u.cpuid.eax;
+    *ebx = sysctl.u.cpuid.ebx;
+    *ecx = sysctl.u.cpuid.ecx;
+    *edx = sysctl.u.cpuid.edx;
+
+    return 0;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
index 50126ae..a714156 100644
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -1801,6 +1801,13 @@ int xc_hvm_inject_trap(
     uint64_t cr2);
 
 /*
+ * Run the cpuid instruction on a specificied physical cpu with the given
+ * parameters.
+ */
+int xc_xen_cpuid(xc_interface *xch, uint32_t cpu, uint32_t *eax,
+                 uint32_t *ebx, uint32_t *ecx, uint32_t *edx);
+
+/*
  *  LOGGING AND ERROR REPORTING
  */
 
diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
index 15d4b91..dbad1ee 100644
--- a/xen/arch/x86/sysctl.c
+++ b/xen/arch/x86/sysctl.c
@@ -66,6 +66,18 @@ void arch_do_physinfo(xen_sysctl_physinfo_t *pi)
         pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm_directio;
 }
 
+void sysctl_cpuid_helper(void *data)
+{
+    struct xen_sysctl_cpuid *cpuid = data;
+
+    ASSERT(smp_processor_id() == cpuid->cpu);
+    asm volatile ("cpuid"
+                  : "=a" (cpuid->eax), "=b" (cpuid->ebx),
+                    "=c" (cpuid->ecx), "=d" (cpuid->edx)
+                  : "a" (cpuid->eax), "b" (cpuid->ebx),
+                    "c" (cpuid->ecx), "d" (cpuid->edx) );
+}
+
 long arch_do_sysctl(
     struct xen_sysctl *sysctl, XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
 {
@@ -101,6 +113,24 @@ long arch_do_sysctl(
     }
     break;
 
+    case XEN_SYSCTL_cpuid:
+    {
+        int cpu = sysctl->u.cpuid.cpu;
+
+        if ( cpu == smp_processor_id() )
+            sysctl_cpuid_helper(&sysctl->u.cpuid);
+        else if ( cpu >= nr_cpu_ids || !cpu_online(cpu) )
+            ret = -EINVAL;
+        else
+            on_selected_cpus(cpumask_of(cpu),
+                             sysctl_cpuid_helper,
+                             &sysctl->u.cpuid, 1);
+
+        if ( !ret && __copy_to_guest(u_sysctl, sysctl, 1) )
+            ret = -EFAULT;
+    }
+    break;
+
     default:
         ret = -ENOSYS;
         break;
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 8437d31..afa8a73 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -633,6 +633,13 @@ typedef struct xen_sysctl_coverage_op xen_sysctl_coverage_op_t;
 DEFINE_XEN_GUEST_HANDLE(xen_sysctl_coverage_op_t);
 
 
+struct xen_sysctl_cpuid {
+    uint32_t cpu; /* IN - Pcpu to execute on */
+    uint32_t eax, ebx, ecx, edx; /* IN/OUT - Parameters to `cpuid` */
+};
+typedef struct xen_sysctl_cpuid xen_sysctl_cpuid_t;
+DEFINE_XEN_GUEST_HANDLE(xen_sysctl_cpuid_t);
+
 struct xen_sysctl {
     uint32_t cmd;
 #define XEN_SYSCTL_readconsole                    1
@@ -654,6 +661,7 @@ struct xen_sysctl {
 #define XEN_SYSCTL_cpupool_op                    18
 #define XEN_SYSCTL_scheduler_op                  19
 #define XEN_SYSCTL_coverage_op                   20
+#define XEN_SYSCTL_cpuid                         21
     uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */
     union {
         struct xen_sysctl_readconsole       readconsole;
@@ -675,6 +683,7 @@ struct xen_sysctl {
         struct xen_sysctl_cpupool_op        cpupool_op;
         struct xen_sysctl_scheduler_op      scheduler_op;
         struct xen_sysctl_coverage_op       coverage_op;
+        struct xen_sysctl_cpuid             cpuid;
         uint8_t                             pad[128];
     } u;
 };
-- 
1.7.10.4

[PATCH RFC 2/2] xen/x86: Introduce XEN_SYSCTL_cpuid hypercall

[Andrew Cooper]

which permits a toolstack to execute an arbitrary cpuid instruction on a
specified physical cpu.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxc/xc_misc.c       |   25 +++++++++++++++++++++++++
 tools/libxc/xenctrl.h       |    7 +++++++
 xen/arch/x86/sysctl.c       |   30 ++++++++++++++++++++++++++++++
 xen/include/public/sysctl.h |    9 +++++++++
 4 files changed, 71 insertions(+)

diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c
index 4f672ce..48ef33e 100644
--- a/tools/libxc/xc_misc.c
+++ b/tools/libxc/xc_misc.c
@@ -774,6 +774,31 @@ int xc_hvm_inject_trap(
     return rc;
 }
 
+int xc_xen_cpuid(xc_interface *xch, uint32_t cpu, uint32_t *eax,
+                 uint32_t *ebx, uint32_t *ecx, uint32_t *edx)
+{
+    int ret;
+    DECLARE_SYSCTL;
+
+    sysctl.cmd = XEN_SYSCTL_cpuid;
+    sysctl.u.cpuid.cpu = cpu;
+    sysctl.u.cpuid.eax = *eax;
+    sysctl.u.cpuid.ebx = *ebx;
+    sysctl.u.cpuid.ecx = *ecx;
+    sysctl.u.cpuid.edx = *edx;
+
+    ret = do_sysctl(xch, &sysctl);
+    if ( ret )
+        return ret;
+
+    *eax = sysctl.u.cpuid.eax;
+    *ebx = sysctl.u.cpuid.ebx;
+    *ecx = sysctl.u.cpuid.ecx;
+    *edx = sysctl.u.cpuid.edx;
+
+    return 0;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
index 50126ae..a714156 100644
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -1801,6 +1801,13 @@ int xc_hvm_inject_trap(
     uint64_t cr2);
 
 /*
+ * Run the cpuid instruction on a specificied physical cpu with the given
+ * parameters.
+ */
+int xc_xen_cpuid(xc_interface *xch, uint32_t cpu, uint32_t *eax,
+                 uint32_t *ebx, uint32_t *ecx, uint32_t *edx);
+
+/*
  *  LOGGING AND ERROR REPORTING
  */
 
diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
index 15d4b91..dbad1ee 100644
--- a/xen/arch/x86/sysctl.c
+++ b/xen/arch/x86/sysctl.c
@@ -66,6 +66,18 @@ void arch_do_physinfo(xen_sysctl_physinfo_t *pi)
         pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm_directio;
 }
 
+void sysctl_cpuid_helper(void *data)
+{
+    struct xen_sysctl_cpuid *cpuid = data;
+
+    ASSERT(smp_processor_id() == cpuid->cpu);
+    asm volatile ("cpuid"
+                  : "=a" (cpuid->eax), "=b" (cpuid->ebx),
+                    "=c" (cpuid->ecx), "=d" (cpuid->edx)
+                  : "a" (cpuid->eax), "b" (cpuid->ebx),
+                    "c" (cpuid->ecx), "d" (cpuid->edx) );
+}
+
 long arch_do_sysctl(
     struct xen_sysctl *sysctl, XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
 {
@@ -101,6 +113,24 @@ long arch_do_sysctl(
     }
     break;
 
+    case XEN_SYSCTL_cpuid:
+    {
+        int cpu = sysctl->u.cpuid.cpu;
+
+        if ( cpu == smp_processor_id() )
+            sysctl_cpuid_helper(&sysctl->u.cpuid);
+        else if ( cpu >= nr_cpu_ids || !cpu_online(cpu) )
+            ret = -EINVAL;
+        else
+            on_selected_cpus(cpumask_of(cpu),
+                             sysctl_cpuid_helper,
+                             &sysctl->u.cpuid, 1);
+
+        if ( !ret && __copy_to_guest(u_sysctl, sysctl, 1) )
+            ret = -EFAULT;
+    }
+    break;
+
     default:
         ret = -ENOSYS;
         break;
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 8437d31..afa8a73 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -633,6 +633,13 @@ typedef struct xen_sysctl_coverage_op xen_sysctl_coverage_op_t;
 DEFINE_XEN_GUEST_HANDLE(xen_sysctl_coverage_op_t);
 
 
+struct xen_sysctl_cpuid {
+    uint32_t cpu; /* IN - Pcpu to execute on */
+    uint32_t eax, ebx, ecx, edx; /* IN/OUT - Parameters to `cpuid` */
+};
+typedef struct xen_sysctl_cpuid xen_sysctl_cpuid_t;
+DEFINE_XEN_GUEST_HANDLE(xen_sysctl_cpuid_t);
+
 struct xen_sysctl {
     uint32_t cmd;
 #define XEN_SYSCTL_readconsole                    1
@@ -654,6 +661,7 @@ struct xen_sysctl {
 #define XEN_SYSCTL_cpupool_op                    18
 #define XEN_SYSCTL_scheduler_op                  19
 #define XEN_SYSCTL_coverage_op                   20
+#define XEN_SYSCTL_cpuid                         21
     uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */
     union {
         struct xen_sysctl_readconsole       readconsole;
@@ -675,6 +683,7 @@ struct xen_sysctl {
         struct xen_sysctl_cpupool_op        cpupool_op;
         struct xen_sysctl_scheduler_op      scheduler_op;
         struct xen_sysctl_coverage_op       coverage_op;
+        struct xen_sysctl_cpuid             cpuid;
         uint8_t                             pad[128];
     } u;
 };
-- 
1.7.10.4

Re: [PATCH RFC 2/2] xen/x86: Introduce XEN_SYSCTL_cpuid hypercall

[Jan Beulich]

>>> On 27.02.14 at 12:11, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> which permits a toolstack to execute an arbitrary cpuid instruction on a
> specified physical cpu.

For one - is it a good idea to expose the unprocessed CPUID to
any guest code? After all even the Dom0 kernel only gets to see
processed values, and the fact the without CPUID faulting apps
in PV guests can inadvertently use the raw values is known to be
a problem, not a feature.

And then - if you already have access to control operations, I
don't think you need the hyypervisor to help you: Limit your
vCPU's affinity to the particular pCPU you care about, and do
what you need doing from the kernel (by also setting the
processes affinity to the particular CPU you could achieve the
same even from user land).

Jan

Re: [PATCH RFC 2/2] xen/x86: Introduce XEN_SYSCTL_cpuid hypercall

[Andrew Cooper]

On 27/02/14 11:58, Jan Beulich wrote:
>>>> On 27.02.14 at 12:11, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> which permits a toolstack to execute an arbitrary cpuid instruction on a
>> specified physical cpu.
> For one - is it a good idea to expose the unprocessed CPUID to
> any guest code? After all even the Dom0 kernel only gets to see
> processed values, and the fact the without CPUID faulting apps
> in PV guests can inadvertently use the raw values is known to be
> a problem, not a feature.

Any toolstack which uses this specific hypercall to find out information
normally hidden from dom0 using faulting/masking/policy can only shoot
itself.

The usecase is for enumerating the real cache leaves, which are normally
faked up in the policy anyway, so of no use.

>
> And then - if you already have access to control operations, I
> don't think you need the hyypervisor to help you: Limit your
> vCPU's affinity to the particular pCPU you care about, and do
> what you need doing from the kernel (by also setting the
> processes affinity to the particular CPU you could achieve the
> same even from user land).
>
> Jan
>

Having a toolstack rely on being able to pin its vcpus around so some
userspace can enumerate the cache leaves is horrific.

Apart from forcibly messing with a balanced numa setup, what about cpu
pools, or toolstack disaggregation where pinning is restricted?

~Andrew

Re: [PATCH RFC 2/2] xen/x86: Introduce XEN_SYSCTL_cpuid hypercall

[Jan Beulich]

>>> On 27.02.14 at 13:11, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> Apart from forcibly messing with a balanced numa setup, what about cpu
> pools, or toolstack disaggregation where pinning is restricted?

There no messing with a balanced setup here - everything should
of course be transient, i.e. get restored to previous values right
after. And I can't see a reason not to permit the hardware domain
to temporarily escape its enclave, as it can do worse to the overall
system anyway.

The new hypercall is simple enough (yet very x86-centric) that I'm
not really against it; what I'm against is adding functionality to the
hypervisor that is already available by other means.

Jan

Re: [PATCH RFC 2/2] xen/x86: Introduce XEN_SYSCTL_cpuid hypercall

[Andrew Cooper]

On 27/02/14 12:26, Jan Beulich wrote:
>>>> On 27.02.14 at 13:11, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> Apart from forcibly messing with a balanced numa setup, what about cpu
>> pools, or toolstack disaggregation where pinning is restricted?
> There no messing with a balanced setup here - everything should
> of course be transient, i.e. get restored to previous values right
> after.

But anything else happening on that vcpu at the same time is transiently
out of balance.

>  And I can't see a reason not to permit the hardware domain
> to temporarily escape its enclave, as it can do worse to the overall
> system anyway.

True

>
> The new hypercall is simple enough (yet very x86-centric) that I'm
> not really against it; what I'm against is adding functionality to the
> hypervisor that is already available by other means.
>
> Jan
>

As I said before, the cache information is faked up by the cpuid policy,
so might be subject to policy depending on faulting or non-dom0 hardware
domian, or PVH dom0 in the near future. 

I think there is a legitimate case for "I really truely need the real
hardware values, without anything in the policy getting in the way". 
'cpuid' is indeed very x86 specific, but it is not information readily
available by other means.

~Andrew

Re: [PATCH RFC 1/2] tools/libxc: Improved xc_{topology, numa}info functions.

[Dario Faggioli]

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

On Thu, 2014-02-27 at 11:11 +0000, Andrew Cooper wrote:
> These two new functions provide a substantially easier-to-use API, where libxc
> itself takes care of all the appropriate bounce buffering.
> 
So, because of the stalled discussion on patch 2 of this series, this
patch was hardly considered, I guess, is that so?

Personally, I think this is an improvement in its own right, and I'd
like to have it, independently from the CPUID stuff. Are you up,
perhaps, for submitting it as a separate patch?

One thing I'm not sure is why you're introducing new *_bounced variants,
rather than going ahead and modifying xc_topologyinfo() and
xc_numainfo()  directly. libxc is not an API we committed to keep
stable, is it?

Thanks and Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

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

Re: [PATCH RFC 1/2] tools/libxc: Improved xc_{topology, numa}info functions.

[Andrew Cooper]

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

On 12/03/14 08:34, Dario Faggioli wrote:
> On Thu, 2014-02-27 at 11:11 +0000, Andrew Cooper wrote:
>> These two new functions provide a substantially easier-to-use API,
where libxc
>> itself takes care of all the appropriate bounce buffering.
>>
> So, because of the stalled discussion on patch 2 of this series, this
> patch was hardly considered, I guess, is that so?
>
> Personally, I think this is an improvement in its own right, and I'd
> like to have it, independently from the CPUID stuff. Are you up,
> perhaps, for submitting it as a separate patch?
>
> One thing I'm not sure is why you're introducing new *_bounced variants,
> rather than going ahead and modifying xc_topologyinfo() and
> xc_numainfo()  directly. libxc is not an API we committed to keep
> stable, is it?
>
> Thanks and Regards,
> Dario
>

I certainly can modify the original ones directly.  When developing, it
in the 4.4 freeze, and I wanted my changes to easily co-exist with
libxc.  (If you notice, these patches are in XenServer trunk for easy
deployment against our entire set of weird & wacky hardware).

If there general agreement that making the old xc_{topology,numa}info()
functions have the prototype and behaviour of my _bounced variants, then
I will happy do that, and send some fixup to make libxl work against it.

~Andrew


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

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

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

Re: [PATCH RFC 1/2] tools/libxc: Improved xc_{topology, numa}info functions.

[Dario Faggioli]

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

On mer, 2014-03-12 at 10:41 +0000, Andrew Cooper wrote:
> On 12/03/14 08:34, Dario Faggioli wrote:
> I certainly can modify the original ones directly.  When developing,
> it in the 4.4 freeze, and I wanted my changes to easily co-exist with
> libxc.  (If you notice, these patches are in XenServer trunk for easy
> deployment against our entire set of weird & wacky hardware).
> 
Sure, I know. That's why I sent this 'kind-of-ping' :-)

> If there general agreement that making the old
> xc_{topology,numa}info() functions have the prototype and behaviour of
> my _bounced variants, then I will happy do that, and send some fixup
> to make libxl work against it.
> 
Of course. Let's hear maintainers. FWIW, I vote for it. As you already
say in the cover letter of the series, the current implementation in
both libxc and libxl is a pain to use and maintain, and set a very ba
example for people wanting to do similar things.

In fact, it was me that, as one of my first contributions to Xen,
(re)implemented libxl_get_numainfo(), out of inspiration from
libxl_get_cpu_topology()! :-/

So, to summarize my view, let's switch the existing to yours ASAP.
Ian-s? :-)

Thanks and Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

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

Re: [PATCH RFC 1/2] tools/libxc: Improved xc_{topology, numa}info functions.

[Ian Campbell]

On Wed, 2014-03-12 at 10:41 +0000, Andrew Cooper wrote:
> If there general agreement that making the old
> xc_{topology,numa}info() functions have the prototype and behaviour of
> my _bounced variants, then I will happy do that,

I think that is the correct thing to do, thanks.

In general hiding the bouncing inside libxc is nicer for everyone I
think, unless they are called incredibly frequently (like the inner loop
of a migrate), I don't think these fit that profile

The only question is whether the function params should explode
xc_topology_info or whether that struct should be redefined without
guest handles for the callers to use (bouncing into the matching fields
in xen_sysctl_topologyinfo_t). That's up to you, I assume it will become
obvious which to pick when you look at the callers.

> and send some fixup to make libxl work against it.

and Python's xc bindings too I'm afraid.

Ian.

Re: [PATCH RFC 2/2] xen/x86: Introduce XEN_SYSCTL_cpuid hypercall

[Ian Campbell]

On Thu, 2014-02-27 at 11:11 +0000, Andrew Cooper wrote:
> which permits a toolstack to execute an arbitrary cpuid instruction on a
> specified physical cpu.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
>  tools/libxc/xc_misc.c       |   25 +++++++++++++++++++++++++
>  tools/libxc/xenctrl.h       |    7 +++++++

These are a pretty straightforward encapsulation of the hypercall, so
subject to agreement with the h/w maintainers about the hypercall
interface the toolside is:
Acked-by: Ian Campbell <ian.campbell@citrix.com>

Ian.