[PATCH RFC 0/8] x86/hvm, libxl: HVM SMT topology support

[PATCH RFC 0/8] x86/hvm, libxl: HVM SMT topology support

[Joao Martins]

Hey!

This series are a follow-up on the thread about the performance
of hard-pinned HVM guests. Here we propose allowing libxl to
change how the CPU topology looks like for the HVM guest, which can 
favor certain workloads as depicted by Elena on this thread [0]. 
It shows around 22-23% gain on io bound workloads having the guest
vCPUs hard pinned to the pCPUs with a matching core+thread.

This series is divided as following:
* Patch 1     : Sets initial apicid to be the vcpuid as opposed
                to vcpuid * 2 for each core;
* Patch 2     : Whitespace cleanup
* Patch 3     : Adds new leafs to describe Intel/AMD cache
                topology. Though it's only internal to libxl;
* Patch 4     : Internal call to set per package CPUID values.
* Patch 5 - 8 : Interfaces for xl and libxl for setting topology.

I couldn't quite figure out which user interface was better so I
included both our "smt" option and full description of the topology
i.e. "sockets", "cores", "threads" option same as the "-smp"
option on QEMU. Note that the latter could also be used on
libvirt since topology is described in their XML configs.

It's also an RFC as AMD support isn't implemented yet.

Any comments are appreciated!

Thanks,
Joao

[0] http://lists.xen.org/archives/html/xen-devel/2016-01/msg02874.html
   

Joao Martins (8):
  x86/hvm: set initial apicid to vcpu_id
  libxl: remove whitespace on libxl_types.idl
  libxl: cpuid: add cache core count support
  libxl: cpuid: add guest topology support
  libxl: introduce smt field
  xl: introduce smt option
  libxl: introduce topology fields
  xl: introduce topology options

 docs/man/xl.cfg.pod.5        | 24 ++++++++++++++++++++++++
 tools/libxc/xc_cpuid_x86.c   | 18 +++++++++++++-----
 tools/libxl/libxl_cpuid.c    | 42 ++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_create.c   |  2 ++
 tools/libxl/libxl_dom.c      | 16 ++++++++++++++++
 tools/libxl/libxl_internal.h |  3 +++
 tools/libxl/libxl_types.idl  | 11 ++++++++---
 tools/libxl/libxl_utils.c    | 17 +++++++++++++++++
 tools/libxl/xl_cmdimpl.c     |  9 +++++++++
 xen/arch/x86/hvm/hvm.c       |  2 +-
 10 files changed, 135 insertions(+), 9 deletions(-)

-- 
2.1.4

[PATCH RFC 1/8] x86/hvm: set initial apicid to vcpu_id

[Joao Martins]

Currently the initial_apicid is set vcpu_id * 2 which makes it difficult
for the toolstack to manage how is the topology seen by the guest.
Instead of forcing procpkg and proccount to be VCPUID * 2, instead we
set it to max vcpuid on proccount to max_vcpu_id + 1 (logical number of
logical cores) and procpkg to max_vcpu_id (max cores minus 1)

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <jbeulich@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
---
 tools/libxc/xc_cpuid_x86.c | 18 +++++++++++++-----
 xen/arch/x86/hvm/hvm.c     |  2 +-
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index c142595..aa1d679 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -45,6 +45,7 @@ struct cpuid_domain_info
     bool hvm;
     bool pvh;
     uint64_t xfeature_mask;
+    uint32_t max_vcpu_id;
 
     /* PV-only information. */
     bool pv64;
@@ -102,6 +103,7 @@ static int get_cpuid_domain_info(xc_interface *xch, domid_t domid,
 
     info->hvm = di.hvm;
     info->pvh = di.pvh;
+    info->max_vcpu_id = di.max_vcpu_id;
 
     /* Get xstate information. */
     domctl.cmd = XEN_DOMCTL_getvcpuextstate;
@@ -245,10 +247,16 @@ static void intel_xc_cpuid_policy(xc_interface *xch,
     case 0x00000004:
         /*
          * EAX[31:26] is Maximum Cores Per Package (minus one).
-         * Update to reflect vLAPIC_ID = vCPU_ID * 2.
          */
-        regs[0] = (((regs[0] & 0x7c000000u) << 1) | 0x04000000u |
-                   (regs[0] & 0x3ffu));
+	{
+                unsigned int procpkg = 0;
+                if ( info->hvm )
+                        procpkg = info->max_vcpu_id << 26;
+                else
+                        procpkg = (regs[0] & 0x7c000000u) << 1;
+
+                regs[0] = (procpkg | 0x04000000u | (regs[0] & 0x3ffu));
+        }
         regs[3] &= 0x3ffu;
         break;
 
@@ -353,9 +361,9 @@ static void xc_cpuid_hvm_policy(xc_interface *xch,
     case 0x00000001:
         /*
          * EBX[23:16] is Maximum Logical Processors Per Package.
-         * Update to reflect vLAPIC_ID = vCPU_ID * 2.
          */
-        regs[1] = (regs[1] & 0x0000ffffu) | ((regs[1] & 0x007f0000u) << 1);
+        regs[1] = (regs[1] & 0x0000ffffu) |
+                   (((info->max_vcpu_id + 1) << 16));
 
         regs[2] &= (bitmaskof(X86_FEATURE_XMM3) |
                     bitmaskof(X86_FEATURE_PCLMULQDQ) |
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index fe382ce..d45e7a9 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4633,7 +4633,7 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx,
     case 0x1:
         /* Fix up VLAPIC details. */
         *ebx &= 0x00FFFFFFu;
-        *ebx |= (v->vcpu_id * 2) << 24;
+        *ebx |= (v->vcpu_id) << 24;
         if ( vlapic_hw_disabled(vcpu_vlapic(v)) )
             __clear_bit(X86_FEATURE_APIC & 31, edx);
 
-- 
2.1.4

[PATCH RFC 2/8] libxl: remove whitespace on libxl_types.idl

[Joao Martins]

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_types.idl | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 9ad7eba..f04279e 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -436,7 +436,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
     ("blkdev_start",    string),
 
     ("vnuma_nodes", Array(libxl_vnode_info, "num_vnuma_nodes")),
-    
+
     ("device_model_version", libxl_device_model_version),
     ("device_model_stubdomain", libxl_defbool),
     # if you set device_model you must set device_model_version too
@@ -497,10 +497,10 @@ libxl_domain_build_info = Struct("domain_build_info",[
                                        ("keymap",           string),
                                        ("sdl",              libxl_sdl_info),
                                        ("spice",            libxl_spice_info),
-                                       
+
                                        ("gfx_passthru",     libxl_defbool),
                                        ("gfx_passthru_kind", libxl_gfx_passthru_kind),
-                                       
+
                                        ("serial",           string),
                                        ("boot",             string),
                                        ("usb",              libxl_defbool),
-- 
2.1.4

[PATCH RFC 3/8] libxl: cpuid: add cache core count support

[Joao Martins]

Intel Cache Topology info is determinted by the leaf 4
its subleaves. So, Only the core count is exposed as the remaining
info in the subleaves are inherited by the host (e.g. cache size)

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_cpuid.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/libxl/libxl_cpuid.c b/tools/libxl/libxl_cpuid.c
index c66e912..deb81d2 100644
--- a/tools/libxl/libxl_cpuid.c
+++ b/tools/libxl/libxl_cpuid.c
@@ -187,6 +187,10 @@ int libxl_cpuid_parse_config(libxl_cpuid_policy_list *cpuid, const char* str)
         {"nx",           0x80000001, NA, CPUID_REG_EDX, 20,  1},
         {"syscall",      0x80000001, NA, CPUID_REG_EDX, 11,  1},
         {"procpkg",      0x00000004,  0, CPUID_REG_EAX, 26,  6},
+        {"proccountl1d", 0x00000004,  0, CPUID_REG_EAX, 14, 12},
+        {"proccountl1i", 0x00000004,  1, CPUID_REG_EAX, 14, 12},
+        {"proccountl2",  0x00000004,  2, CPUID_REG_EAX, 14, 12},
+        {"proccountl3",  0x00000004,  3, CPUID_REG_EAX, 14, 12},
         {"apicidsize",   0x80000008, NA, CPUID_REG_ECX, 12,  4},
         {"nc",           0x80000008, NA, CPUID_REG_ECX,  0,  8},
         {"svm_npt",      0x8000000a, NA, CPUID_REG_EDX,  0,  1},
-- 
2.1.4

[PATCH RFC 4/8] libxl: cpuid: add guest topology support

[Joao Martins]

Introduce internal cpuid routine for setting the topology
as seen by the guest. The topology is made based on
leaf 1 and leaf 4 for Intel, more specifically setting:

Number of logical processors:
	proccount  (CPUID.1:EBX[16:24])

Number of physical cores - 1:
	procpkg    (CPUID.(4,0):EBX[26:32])

cache core count - 1:
	proccountX (CPUID.(4,Y):EBX[14:26])

	given that X is l1d, l1i, l2 or l3
	and Y the correspondent subleave [0-3]

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_cpuid.c    | 38 ++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_internal.h |  2 ++
 2 files changed, 40 insertions(+)

diff --git a/tools/libxl/libxl_cpuid.c b/tools/libxl/libxl_cpuid.c
index deb81d2..e220566 100644
--- a/tools/libxl/libxl_cpuid.c
+++ b/tools/libxl/libxl_cpuid.c
@@ -352,6 +352,44 @@ void libxl_cpuid_set(libxl_ctx *ctx, uint32_t domid,
                      (const char**)(cpuid[i].policy), cpuid_res);
 }
 
+static int libxl_cpuid_parse_list(libxl_cpuid_policy_list *topo,
+                                  char **keys, int *vals, size_t sz)
+{
+    int i, ret = -1;
+
+    for (i = 0; i < sz; i++) {
+        char *str = NULL;
+
+        asprintf(&str, "%s=%d", keys[i], vals[i]);
+
+        ret = libxl_cpuid_parse_config(topo, str);
+        free(str);
+
+        if (ret)
+            break;
+    }
+
+    return ret;
+}
+
+void libxl__cpuid_set_topology(libxl_ctx *ctx, uint32_t domid,
+                               uint32_t cores, uint32_t threads)
+{
+    libxl_cpuid_policy_list topo;
+    char *keys[] = {
+        "htt", "proccount", "procpkg",
+        "proccountl1d", "proccountl1i", "proccountl2", "proccountl3"
+    };
+    int vals[] = {
+        1, cores * threads, --cores,
+        --threads, threads, threads, cores
+    };
+
+    memset(&topo, 0, sizeof(topo));
+    if (!libxl_cpuid_parse_list(&topo, keys, vals, ARRAY_SIZE(keys)))
+        libxl_cpuid_set(ctx, domid, topo);
+}
+
 static const char *input_names[2] = { "leaf", "subleaf" };
 static const char *policy_names[4] = { "eax", "ebx", "ecx", "edx" };
 /*
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 650a958..903ad7b 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3919,6 +3919,8 @@ static inline int libxl__key_value_list_is_empty(libxl_key_value_list *pkvl)
 }
 
 int libxl__cpuid_policy_is_empty(libxl_cpuid_policy_list *pl);
+void libxl__cpuid_set_topology(libxl_ctx *ctx, uint32_t domid,
+                               uint32_t max_vcpus, uint32_t threads);
 
 /* Portability note: a proper flock(2) implementation is required */
 typedef struct {
-- 
2.1.4

[PATCH RFC 5/8] libxl: introduce smt field

[Joao Martins]

By setting it to true, it enables the vcpus set by the guest
to be seen as a SMT-enabled topology. It uses then
libxl__cpuid_set_topology() to change the cpuid accordingly.
This setting is made *before* the cpuid is set so that
any changes could be overwritten. The number of SMT threads
are the ones supported by the host. And for that it adds
another helper routine libxl__count_threads_per_core to fetch
it.

This feature is useful in HT-enabled hosts so that hard
pinned domains can see the right topology (i.e. SMT cores exposed
as SMT cores) which in return would lead guest scheduler making
better decisions.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_create.c   |  2 ++
 tools/libxl/libxl_dom.c      | 10 ++++++++++
 tools/libxl/libxl_internal.h |  1 +
 tools/libxl/libxl_types.idl  |  1 +
 tools/libxl/libxl_utils.c    | 17 +++++++++++++++++
 5 files changed, 31 insertions(+)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index de5d27f..dac15d5 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -195,6 +195,8 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
         b_info->num_vcpu_hard_affinity = b_info->max_vcpus;
     }
 
+    libxl_defbool_setdefault(&b_info->smt, false);
+
     libxl_defbool_setdefault(&b_info->numa_placement, true);
 
     if (b_info->max_memkb == LIBXL_MEMKB_DEFAULT)
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 2269998..ff9356d 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -507,6 +507,16 @@ int libxl__build_post(libxl__gc *gc, uint32_t domid,
     }
 
     libxl_cpuid_apply_policy(ctx, domid);
+    if (info->type == LIBXL_DOMAIN_TYPE_HVM
+        && libxl_defbool_val(info->smt)) {
+
+        uint32_t threads = 0;
+
+        if (!libxl__count_threads_per_core(gc, &threads))
+            libxl__cpuid_set_topology(ctx, domid,
+                                      info->max_vcpus / threads, threads);
+    }
+
     if (info->cpuid != NULL)
         libxl_cpuid_set(ctx, domid, info->cpuid);
 
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 903ad7b..7cc4de7 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -4033,6 +4033,7 @@ void libxl__bitmap_copy_best_effort(libxl__gc *gc, libxl_bitmap *dptr,
                                     const libxl_bitmap *sptr);
 
 int libxl__count_physical_sockets(libxl__gc *gc, int *sockets);
+int libxl__count_threads_per_core(libxl__gc *gc, uint32_t *threads);
 
 
 #define LIBXL_QEMU_USER_PREFIX "xen-qemuuser"
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index f04279e..fa4725a 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -421,6 +421,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
     ("nodemap",         libxl_bitmap),
     ("vcpu_hard_affinity", Array(libxl_bitmap, "num_vcpu_hard_affinity")),
     ("vcpu_soft_affinity", Array(libxl_bitmap, "num_vcpu_soft_affinity")),
+    ("smt",             libxl_defbool),
     ("numa_placement",  libxl_defbool),
     ("tsc_mode",        libxl_tsc_mode),
     ("max_memkb",       MemKB),
diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index e42422a..dd063c8 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -861,6 +861,23 @@ int libxl__count_physical_sockets(libxl__gc *gc, int *sockets)
     return 0;
 }
 
+int libxl__count_threads_per_core(libxl__gc *gc, uint32_t *threads)
+{
+    int rc;
+    libxl_physinfo info;
+
+    libxl_physinfo_init(&info);
+
+    rc = libxl_get_physinfo(CTX, &info);
+    if (rc)
+        return rc;
+
+    *threads = info.threads_per_core;
+
+    libxl_physinfo_dispose(&info);
+    return 0;
+}
+
 int libxl_socket_bitmap_alloc(libxl_ctx *ctx, libxl_bitmap *socketmap,
                               int max_sockets)
 {
-- 
2.1.4

[PATCH RFC 6/8] xl: introduce smt option

[Joao Martins]

This options changes how "vcpus" are seen by dividing
them in the number of SMT threads supported by the host.
This should be used together with "cpus" so that the topology
matches.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 docs/man/xl.cfg.pod.5    | 6 ++++++
 tools/libxl/xl_cmdimpl.c | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index 40690bd..5e614f7 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -104,6 +104,12 @@ Put the guest's vcpus into the named cpu pool.
 
 Start the guest with N vcpus initially online.
 
+=item B<smt=BOOLEAN>
+
+Configures if guests should see SMT topology instead of normal flat topology.
+
+Default is C<0>
+
 =item B<maxvcpus=M>
 
 Allow the guest to bring up a maximum of M vcpus. At start of day if
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index d07ccb2..c09f628 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1365,6 +1365,8 @@ static void parse_config_data(const char *config_source,
             libxl_bitmap_set((&b_info->avail_vcpus), l);
     }
 
+    xlu_cfg_get_defbool(config, "smt", &b_info->smt, 0);
+
     if (!xlu_cfg_get_long (config, "maxvcpus", &l, 0))
         b_info->max_vcpus = l;
 
-- 
2.1.4

[PATCH RFC 7/8] libxl: introduce topology fields

[Joao Martins]

Currently there is "smt" option that changes from a flat core topology
to a core+thread topology. This patch adds more expressive options for
describing the topology as seen by the guest i.e. sockets, cores and
threads to adjust cpu topology as seen by the guest.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_dom.c     | 18 ++++++++++++------
 tools/libxl/libxl_types.idl |  4 ++++
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index ff9356d..1e6d9ab 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -507,14 +507,20 @@ int libxl__build_post(libxl__gc *gc, uint32_t domid,
     }
 
     libxl_cpuid_apply_policy(ctx, domid);
-    if (info->type == LIBXL_DOMAIN_TYPE_HVM
-        && libxl_defbool_val(info->smt)) {
+    if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
 
-        uint32_t threads = 0;
+        uint32_t threads = 0, cores = 0;
+
+        if (libxl_defbool_val(info->smt)
+            && !libxl__count_threads_per_core(gc, &threads))
+            cores = info->max_vcpus / threads;
+        else if (info->topology.cores) {
+            cores = info->topology.cores;
+            threads = info->topology.threads;
+        }
 
-        if (!libxl__count_threads_per_core(gc, &threads))
-            libxl__cpuid_set_topology(ctx, domid,
-                                      info->max_vcpus / threads, threads);
+        if (cores && threads)
+            libxl__cpuid_set_topology(ctx, domid, cores, threads);
     }
 
     if (info->cpuid != NULL)
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index fa4725a..caba626 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -422,6 +422,10 @@ libxl_domain_build_info = Struct("domain_build_info",[
     ("vcpu_hard_affinity", Array(libxl_bitmap, "num_vcpu_hard_affinity")),
     ("vcpu_soft_affinity", Array(libxl_bitmap, "num_vcpu_soft_affinity")),
     ("smt",             libxl_defbool),
+    ("topology",        Struct(None, [("sockets", integer),
+                                      ("cores",   integer),
+                                      ("threads", integer),
+                                      ])),
     ("numa_placement",  libxl_defbool),
     ("tsc_mode",        libxl_tsc_mode),
     ("max_memkb",       MemKB),
-- 
2.1.4

[PATCH RFC 8/8] xl: introduce topology options

[Joao Martins]

Namely sockets, cores and threads to describe the topology as seen
by the guest. This as an alternative to the SMT option that is also
proposed.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 docs/man/xl.cfg.pod.5    | 18 ++++++++++++++++++
 tools/libxl/xl_cmdimpl.c |  7 +++++++
 2 files changed, 25 insertions(+)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index 5e614f7..cb7ebc2 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -110,6 +110,24 @@ Configures if guests should see SMT topology instead of normal flat topology.
 
 Default is C<0>
 
+=item B<sockets=N>
+
+Configures number of sockets as seen by the guest.
+
+Default is C<0>
+
+=item B<cores=N>
+
+Configures number of physical cores per package as seen by the guest.
+
+Default is C<0>
+
+=item B<threads=N>
+
+Configures number of SMT threads per core as seen by the guest.
+
+Default is C<0>
+
 =item B<maxvcpus=M>
 
 Allow the guest to bring up a maximum of M vcpus. At start of day if
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index c09f628..965d60b 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1367,6 +1367,13 @@ static void parse_config_data(const char *config_source,
 
     xlu_cfg_get_defbool(config, "smt", &b_info->smt, 0);
 
+    if (!xlu_cfg_get_long(config, "sockets", &l, 0))
+        b_info->topology.sockets = l;
+    if (!xlu_cfg_get_long(config, "cores",   &l, 0))
+        b_info->topology.cores = l;
+    if (!xlu_cfg_get_long(config, "threads", &l, 0))
+        b_info->topology.threads = l;
+
     if (!xlu_cfg_get_long (config, "maxvcpus", &l, 0))
         b_info->max_vcpus = l;
 
-- 
2.1.4

Re: [PATCH RFC 2/8] libxl: remove whitespace on libxl_types.idl

[Wei Liu]

On Mon, Feb 22, 2016 at 09:02:08PM +0000, Joao Martins wrote:
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

> ---
> CC: Ian Jackson <ian.jackson@eu.citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxl/libxl_types.idl | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 9ad7eba..f04279e 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -436,7 +436,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>      ("blkdev_start",    string),
>  
>      ("vnuma_nodes", Array(libxl_vnode_info, "num_vnuma_nodes")),
> -    
> +
>      ("device_model_version", libxl_device_model_version),
>      ("device_model_stubdomain", libxl_defbool),
>      # if you set device_model you must set device_model_version too
> @@ -497,10 +497,10 @@ libxl_domain_build_info = Struct("domain_build_info",[
>                                         ("keymap",           string),
>                                         ("sdl",              libxl_sdl_info),
>                                         ("spice",            libxl_spice_info),
> -                                       
> +
>                                         ("gfx_passthru",     libxl_defbool),
>                                         ("gfx_passthru_kind", libxl_gfx_passthru_kind),
> -                                       
> +
>                                         ("serial",           string),
>                                         ("boot",             string),
>                                         ("usb",              libxl_defbool),
> -- 
> 2.1.4
> 

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

Re: [PATCH RFC 4/8] libxl: cpuid: add guest topology support

[Wei Liu]

On Mon, Feb 22, 2016 at 09:02:10PM +0000, Joao Martins wrote:
> Introduce internal cpuid routine for setting the topology
> as seen by the guest. The topology is made based on
> leaf 1 and leaf 4 for Intel, more specifically setting:
> 
> Number of logical processors:
> 	proccount  (CPUID.1:EBX[16:24])
> 
> Number of physical cores - 1:
> 	procpkg    (CPUID.(4,0):EBX[26:32])
> 
> cache core count - 1:
> 	proccountX (CPUID.(4,Y):EBX[14:26])
> 
> 	given that X is l1d, l1i, l2 or l3
> 	and Y the correspondent subleave [0-3]
> 
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
> CC: Ian Jackson <ian.jackson@eu.citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxl/libxl_cpuid.c    | 38 ++++++++++++++++++++++++++++++++++++++
>  tools/libxl/libxl_internal.h |  2 ++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/tools/libxl/libxl_cpuid.c b/tools/libxl/libxl_cpuid.c
> index deb81d2..e220566 100644
> --- a/tools/libxl/libxl_cpuid.c
> +++ b/tools/libxl/libxl_cpuid.c
> @@ -352,6 +352,44 @@ void libxl_cpuid_set(libxl_ctx *ctx, uint32_t domid,
>                       (const char**)(cpuid[i].policy), cpuid_res);
>  }
>  
> +static int libxl_cpuid_parse_list(libxl_cpuid_policy_list *topo,
> +                                  char **keys, int *vals, size_t sz)

Just call it cpuid_parse_list is fine.

Wei.

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

Re: [PATCH RFC 5/8] libxl: introduce smt field

[Wei Liu]

On Mon, Feb 22, 2016 at 09:02:11PM +0000, Joao Martins wrote:
> By setting it to true, it enables the vcpus set by the guest
> to be seen as a SMT-enabled topology. It uses then
> libxl__cpuid_set_topology() to change the cpuid accordingly.
> This setting is made *before* the cpuid is set so that
> any changes could be overwritten. The number of SMT threads
> are the ones supported by the host. And for that it adds
> another helper routine libxl__count_threads_per_core to fetch
> it.
> 
> This feature is useful in HT-enabled hosts so that hard
> pinned domains can see the right topology (i.e. SMT cores exposed
> as SMT cores) which in return would lead guest scheduler making
> better decisions.
> 

I think we need more sophisticated configuration options then just a
bool. If we're doing this, there is no reason why we shouldn't allow
administrator to set arbitrary topology for the guest.

Wei.

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

Re: [PATCH RFC 7/8] libxl: introduce topology fields

[Wei Liu]

On Mon, Feb 22, 2016 at 09:02:13PM +0000, Joao Martins wrote:
> Currently there is "smt" option that changes from a flat core topology
> to a core+thread topology. This patch adds more expressive options for
> describing the topology as seen by the guest i.e. sockets, cores and
> threads to adjust cpu topology as seen by the guest.
> 
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
> CC: Ian Jackson <ian.jackson@eu.citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxl/libxl_dom.c     | 18 ++++++++++++------
>  tools/libxl/libxl_types.idl |  4 ++++
>  2 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index ff9356d..1e6d9ab 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -507,14 +507,20 @@ int libxl__build_post(libxl__gc *gc, uint32_t domid,
>      }
>  
>      libxl_cpuid_apply_policy(ctx, domid);
> -    if (info->type == LIBXL_DOMAIN_TYPE_HVM
> -        && libxl_defbool_val(info->smt)) {
> +    if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
>  
> -        uint32_t threads = 0;
> +        uint32_t threads = 0, cores = 0;
> +
> +        if (libxl_defbool_val(info->smt)
> +            && !libxl__count_threads_per_core(gc, &threads))
> +            cores = info->max_vcpus / threads;
> +        else if (info->topology.cores) {
> +            cores = info->topology.cores;
> +            threads = info->topology.threads;
> +        }
>  
> -        if (!libxl__count_threads_per_core(gc, &threads))
> -            libxl__cpuid_set_topology(ctx, domid,
> -                                      info->max_vcpus / threads, threads);
> +        if (cores && threads)
> +            libxl__cpuid_set_topology(ctx, domid, cores, threads);
>      }
>  
>      if (info->cpuid != NULL)
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index fa4725a..caba626 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -422,6 +422,10 @@ libxl_domain_build_info = Struct("domain_build_info",[
>      ("vcpu_hard_affinity", Array(libxl_bitmap, "num_vcpu_hard_affinity")),
>      ("vcpu_soft_affinity", Array(libxl_bitmap, "num_vcpu_soft_affinity")),
>      ("smt",             libxl_defbool),
> +    ("topology",        Struct(None, [("sockets", integer),
> +                                      ("cores",   integer),
> +                                      ("threads", integer),
> +                                      ])),

Maybe having topology is a good enough sophisticated configuration
interface? (See my previous email)

Wei.

>      ("numa_placement",  libxl_defbool),
>      ("tsc_mode",        libxl_tsc_mode),
>      ("max_memkb",       MemKB),
> -- 
> 2.1.4
> 

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

Re: [PATCH RFC 1/8] x86/hvm: set initial apicid to vcpu_id

[Jan Beulich]

>>> On 22.02.16 at 22:02, <joao.m.martins@oracle.com> wrote:
> Currently the initial_apicid is set vcpu_id * 2 which makes it difficult
> for the toolstack to manage how is the topology seen by the guest.
> Instead of forcing procpkg and proccount to be VCPUID * 2, instead we
> set it to max vcpuid on proccount to max_vcpu_id + 1 (logical number of
> logical cores) and procpkg to max_vcpu_id (max cores minus 1)

I'm afraid it takes more than this to explain why the change is
needed or at least desirable. In particular I'd like to suggest that
you do some archeology to understand why things are the way
they are.

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4633,7 +4633,7 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, 
> unsigned int *ebx,
>      case 0x1:
>          /* Fix up VLAPIC details. */
>          *ebx &= 0x00FFFFFFu;
> -        *ebx |= (v->vcpu_id * 2) << 24;
> +        *ebx |= (v->vcpu_id) << 24;
>          if ( vlapic_hw_disabled(vcpu_vlapic(v)) )
>              __clear_bit(X86_FEATURE_APIC & 31, edx);

In no case is this sufficient as adjustment to the hypervisor side,
as it gets things out of sync with e.g. hvm/vlapic.c.

Jan


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

Re: [PATCH RFC 0/8] x86/hvm, libxl: HVM SMT topology support

[Andrew Cooper]

On 22/02/16 21:02, Joao Martins wrote:
> Hey!
>
> This series are a follow-up on the thread about the performance
> of hard-pinned HVM guests. Here we propose allowing libxl to
> change how the CPU topology looks like for the HVM guest, which can 
> favor certain workloads as depicted by Elena on this thread [0]. 
> It shows around 22-23% gain on io bound workloads having the guest
> vCPUs hard pinned to the pCPUs with a matching core+thread.
>
> This series is divided as following:
> * Patch 1     : Sets initial apicid to be the vcpuid as opposed
>                 to vcpuid * 2 for each core;
> * Patch 2     : Whitespace cleanup
> * Patch 3     : Adds new leafs to describe Intel/AMD cache
>                 topology. Though it's only internal to libxl;
> * Patch 4     : Internal call to set per package CPUID values.
> * Patch 5 - 8 : Interfaces for xl and libxl for setting topology.
>
> I couldn't quite figure out which user interface was better so I
> included both our "smt" option and full description of the topology
> i.e. "sockets", "cores", "threads" option same as the "-smp"
> option on QEMU. Note that the latter could also be used on
> libvirt since topology is described in their XML configs.
>
> It's also an RFC as AMD support isn't implemented yet.
>
> Any comments are appreciated!

Hey.  Sorry I am late getting to this - I am currently swamped.  Some
general observations.

The cpuid policy code in Xen was never re-thought through after
multi-vcpu guests were introduced, which means they have no
understanding of per-package, per-core and per-thread values.

As part of my further cpuid work, I will need to fix this.  I was
planning to fix it by requiring full cpu topology information to be
passed as part of the domaincreate or max_vcpus hypercall  (not chosen
which yet).  This would include cores-per-package, threads-per-core etc,
and allow Xen to correctly fill in the per-core cpuid values in leaves
4, 0xB and 80000008.

In particular, I am concerned about giving the toolstack the ability to
blindly control the APIC IDs.  Their layout is very closely linked to
topology, and in particular to the HTT flag.

Overall, I want to avoid any possibility of generating APIC layouts
(including the emulated IOAPIC with HVM guests) which don't conform to
the appropriate AMD/Intel manuals.

~Andrew

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

Re: [PATCH RFC 0/8] x86/hvm, libxl: HVM SMT topology support

[Dario Faggioli]

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

On Thu, 2016-02-25 at 17:21 +0000, Andrew Cooper wrote:
> On 22/02/16 21:02, Joao Martins wrote:
> > 
> > Any comments are appreciated!
> Hey.  Sorry I am late getting to this - I am currently swamped.  Some
> general observations.
> 
Hi,

I'm also looking forward to find the time to look at this series, but
that will have to wait a few days more, I'm afraid.

However, one thing (coming from Andrew's comment).

> As part of my further cpuid work, I will need to fix this.  I was
> planning to fix it by requiring full cpu topology information to be
> passed as part of the domaincreate or max_vcpus hypercall  (not
> chosen
> which yet).  
>
If that means that, when creating a multi vcpus guest, it will be
necessary to provide Xen with all the information about the
relationship between these multiple vcpus (and that there will be some
sensible default, of course), this would be *awesome*. :-)

At that point, one can just build on top of that, in order to achieve
something like what is implemented in this series, or any other variant
of it, which would indeed be *awesome* (did I said that already? :-D).

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: 181 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 0/8] x86/hvm, libxl: HVM SMT topology support

[Konrad Rzeszutek Wilk]

On Fri, Feb 26, 2016 at 04:03:46PM +0100, Dario Faggioli wrote:
> On Thu, 2016-02-25 at 17:21 +0000, Andrew Cooper wrote:
> > On 22/02/16 21:02, Joao Martins wrote:
> > > 
> > > Any comments are appreciated!
> > Hey.  Sorry I am late getting to this - I am currently swamped.  Some
> > general observations.
> > 
> Hi,
> 
> I'm also looking forward to find the time to look at this series, but
> that will have to wait a few days more, I'm afraid.
> 
> However, one thing (coming from Andrew's comment).
> 
> > As part of my further cpuid work, I will need to fix this.  I was
> > planning to fix it by requiring full cpu topology information to be
> > passed as part of the domaincreate or max_vcpus hypercall  (not
> > chosen
> > which yet).  


You may not want to make a full CPU topology to be exposed to the guest.

Elena (CCed) found some oddities and it actually looked like the guest
performed _worst_ when it had this exposed and was floating (not-pinned)
on machines with SMT enabled.

Elena, could you confirm please? I can't recall the details..


> >
> If that means that, when creating a multi vcpus guest, it will be
> necessary to provide Xen with all the information about the
> relationship between these multiple vcpus (and that there will be some
> sensible default, of course), this would be *awesome*. :-)
> 
> At that point, one can just build on top of that, in order to achieve
> something like what is implemented in this series, or any other variant
> of it, which would indeed be *awesome* (did I said that already? :-D).

Maybe you should lay off the coffee for a bit ..

/me backs slowly away.

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

Re: [PATCH RFC 0/8] x86/hvm, libxl: HVM SMT topology support

[Dario Faggioli]

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

On Fri, 2016-02-26 at 10:27 -0500, Konrad Rzeszutek Wilk wrote:
> On Fri, Feb 26, 2016 at 04:03:46PM +0100, Dario Faggioli wrote:
> > 
> > > As part of my further cpuid work, I will need to fix this.  I was
> > > planning to fix it by requiring full cpu topology information to
> > > be
> > > passed as part of the domaincreate or max_vcpus hypercall  (not
> > > chosen
> > > which yet).  
> 
> You may not want to make a full CPU topology to be exposed to the
> guest.
> 
Right, but if I understood correctly (and, actually, to confirm that is
what I'm asking), what's Andrew is saying does not mean "always expose
the host topology". It means that a guest must have a (virtual)
topology, and that topology can be Xen's default one, toolstack's
default one, toolstack's default one when vcpu pinning is on, one that
the administrator like, etc.

> Elena (CCed) found some oddities and it actually looked like the
> guest
> performed _worst_ when it had this exposed and was floating (not-
> pinned)
> on machines with SMT enabled.
> 
Well, sure, that's something I would entirely expect. And in fact, more
than better or worst, I remember the numbers you (well, she) posted to
prove that exposing topology and letting the vcpu free to float leads
to inconsistency and lack of predictability, which again is what I'd
have predicted. :-)

> > At that point, one can just build on top of that, in order to
> > achieve
> > something like what is implemented in this series, or any other
> > variant
> > of it, which would indeed be *awesome* (did I said that already? :-
> > D).
> Maybe you should lay off the coffee for a bit ..
> 
I also think that... but, you know, I'm in Italy, and here it's a crime
not to have at least 4 coffees (== espresso, of course!) during the
day! ;-)

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: 181 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 0/8] x86/hvm, libxl: HVM SMT topology support

[Andrew Cooper]

On 26/02/16 15:42, Dario Faggioli wrote:
> On Fri, 2016-02-26 at 10:27 -0500, Konrad Rzeszutek Wilk wrote:
>> On Fri, Feb 26, 2016 at 04:03:46PM +0100, Dario Faggioli wrote:
>>>  
>>>> As part of my further cpuid work, I will need to fix this.  I was
>>>> planning to fix it by requiring full cpu topology information to
>>>> be
>>>> passed as part of the domaincreate or max_vcpus hypercall  (not
>>>> chosen
>>>> which yet).  
>> You may not want to make a full CPU topology to be exposed to the
>> guest.
>>
> Right, but if I understood correctly (and, actually, to confirm that is
> what I'm asking), what's Andrew is saying does not mean "always expose
> the host topology". It means that a guest must have a (virtual)
> topology, and that topology can be Xen's default one, toolstack's
> default one, toolstack's default one when vcpu pinning is on, one that
> the administrator like, etc.

Correct.  As far as this goes, I don't care whether the topology the
guest sees matches the host topology or not.  What I care about is the
guest seeing a plausible topology, conforming to the rules set out by
the Intel/AMD manuals.

~Andrew

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

Re: [PATCH RFC 1/8] x86/hvm: set initial apicid to vcpu_id

[Joao Martins]

On 02/25/2016 05:03 PM, Jan Beulich wrote:
>>>> On 22.02.16 at 22:02, <joao.m.martins@oracle.com> wrote:
>> Currently the initial_apicid is set vcpu_id * 2 which makes it difficult
>> for the toolstack to manage how is the topology seen by the guest.
>> Instead of forcing procpkg and proccount to be VCPUID * 2, instead we
>> set it to max vcpuid on proccount to max_vcpu_id + 1 (logical number of
>> logical cores) and procpkg to max_vcpu_id (max cores minus 1)
> 
> I'm afraid it takes more than this to explain why the change is
> needed or at least desirable.
Apologies for my clumsiness in the commit message. I should have explained
properly why we need this for this series in the first place.

Currently use initial_apicid as vcpu_id * 2,
and doubled the leafs 1 and 4 values (proccount and procpkg) which means we will
address 8 LAPICIDs (tohugh only 4 will be used). Example topology and algorithm
below to facilitate discussion:

# Maximum logical addressable IDs (logical processors in a package)
proccount = CPUID.1:EBX[23:16]

# Maximum core addressable IDs - 1 (maximum cores in a package - 1)
procpkg = CPUID.(4,0):EAX[31:26]

# MSB (Calculate most significant bit)
SMT_Mask_width = MSB(proccount / (procpkg + 1))
Core_Mask_width = MSB(procpkg + 1)
CoreSMT_Mask_width = SMT_Mask_width + Core_Mask_width
Pkg_Mask_width = 1 << CoreSMT_Mask_width

SMT_ID = APICID & ((1 << SMT_Mask_width) - 1)
Core_ID = (APICID >> SMT_Mask_width) & ((1 << Core_Mask_width) - 1)
Pkg_ID = (APICID & Pkg_Mask_width) >> CoreSMT_Mask_width

So as it is right now, the topology on a 4 vcpu HVM guest looks like:

=> topology(proccount = 16, procpkg = 7) # current
APICID=0 SMT_ID=0 CORE_ID=0 PKG_ID=0 # VCPU 0
APICID=1 SMT_ID=1 CORE_ID=0 PKG_ID=0
APICID=2 SMT_ID=0 CORE_ID=1 PKG_ID=0 # VCPU 1
APICID=3 SMT_ID=1 CORE_ID=1 PKG_ID=0
APICID=4 SMT_ID=0 CORE_ID=2 PKG_ID=0 # VCPU 2
APICID=5 SMT_ID=1 CORE_ID=2 PKG_ID=0
APICID=6 SMT_ID=0 CORE_ID=3 PKG_ID=0 # VCPU 3
APICID=7 SMT_ID=1 CORE_ID=3 PKG_ID=0
[...]
APICID=14 SMT_ID=0 CORE_ID=7 PKG_ID=0
APICID=15 SMT_ID=1 CORE_ID=7 PKG_ID=0

As you know, APICID describes the SMT, Core and PKG. Problem with having APICID
in even numbers (0, 2, 4, ... N) is that we can't describe the SMT/siblings in
the topology. Thus turning the APICID ID space into (0, 1, 2 .. N) like this
patch proposes means we can know calculate all possibilities on both
topology kinds. Note that is a prerequisite patch so that a later
patch in this series sets the proccount and procpkg to enable seeing some cores
as SMT siblings.

=> topology(proccount = 4, procpkg = 3) # with this patch
APICID=0 SMT_ID=0 CORE_ID=0 PKG_ID=0
APICID=1 SMT_ID=0 CORE_ID=1 PKG_ID=0
APICID=2 SMT_ID=0 CORE_ID=2 PKG_ID=0
APICID=3 SMT_ID=0 CORE_ID=3 PKG_ID=0

x2APIC isn't addressed here for this RFC but it has the same issue (and
consequently exposure of FEATURE_XTOPOLOGY, CPUID.(EAX=0xB, ECX=N)). One
difference is that the SMT,Core,Pkg mask widths are fetched from each subleaf
directly as opposed to a calculation between procpkg and proccount.

> In particular I'd like to suggest that
> you do some archeology to understand why things are the way
> they are.
Digging in the history and threads, this behavior seems to be introduced by
commit c21d85b ("[HVM] Change VCPU->LAPIC_ID mapping so that VCPU0 has ID0")
where the main issue looked like a conflict between VCPU 0 LAPICID and IOAPIC
ID. Previous commits (a41ba62, facdf41) made IOAPIC on 0 and vLAPIC on 1..N but
it broke on old kernels (for the lack of LAPIC 0), so it ended up having a
vLAPIC ID space with 0, 2, 4, 6 and assign vIOAPICID = 1. This way all of
{L,IO}APICs have unique IDs - this thread
(http://lists.xen.org/archives/html/xen-devel/2008-09/msg00986.html) seems to
mention something along these lines too.

But the manuals aren't exactly clear on this ID uniqueness between LAPICs and
I/O APICs on more recent processors. Any lights on this would be great.

Intel 82093AA (IOAPIC) datasheet [0] says the following:

"This register contains the 4-bit APIC ID. The ID serves as a physical name of
the IOAPIC. All APIC devices using the APIC bus should have a unique APIC ID."

Though looking at the Intel SDM Volume 3, Chapter 10.1, Figure 10-2 and 10-3,
the APIC bus seems to be used only up to P6 family processors (Figure 10-2)
and it's indeed shared between I/OAPIC and LAPIC . For its successor (Pentium 4
and later) it's no longer the case (Figure 10-3).

My Broadwell machine in fact have conflicting APIC IDs between IOAPIC and LAPIC
in my MADT table. And it does seem that it's the case too for SeaBIOS (commit
e39b938 ("report real I/O APIC ID (0) on MADT and MP-table (v3)") ) and QEMU.
Though it wouldn't justify as reason for doing this on Xen.

[0] http://www.intel.com/design/chipsets/datashts/29056601.pdf

>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -4633,7 +4633,7 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, 
>> unsigned int *ebx,
>>      case 0x1:
>>          /* Fix up VLAPIC details. */
>>          *ebx &= 0x00FFFFFFu;
>> -        *ebx |= (v->vcpu_id * 2) << 24;
>> +        *ebx |= (v->vcpu_id) << 24;
>>          if ( vlapic_hw_disabled(vcpu_vlapic(v)) )
>>              __clear_bit(X86_FEATURE_APIC & 31, edx);
> 
> In no case is this sufficient as adjustment to the hypervisor side,
> as it gets things out of sync with e.g. hvm/vlapic.c.
Yes, and also the firmware like this chunk below (tested too):

diff --git a/tools/firmware/hvmloader/config.h b/tools/firmware/hvmloader/config.h
index b838cf9..9c1f2f7 100644
--- a/tools/firmware/hvmloader/config.h
+++ b/tools/firmware/hvmloader/config.h
@@ -47,7 +47,7 @@ extern struct bios_config ovmf_config;
 #define IOAPIC_VERSION      0x11

 #define LAPIC_BASE_ADDRESS  0xfee00000
-#define LAPIC_ID(vcpu_id)   ((vcpu_id) * 2)
+#define LAPIC_ID(vcpu_id)   (vcpu_id)

 #define PCI_ISA_DEVFN       0x08    /* dev 1, fn 0 */
 #define PCI_ISA_IRQ_MASK    0x0c20U /* ISA IRQs 5,10,11 are PCI connected */
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 01a8430..80fc6a1 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -1206,7 +1206,7 @@ void vlapic_reset(struct vlapic *vlapic)
     if ( !has_vlapic(v->domain) )
         return;

-    vlapic_set_reg(vlapic, APIC_ID,  (v->vcpu_id * 2) << 24);
+    vlapic_set_reg(vlapic, APIC_ID,  v->vcpu_id << 24);
     vlapic_set_reg(vlapic, APIC_LVR, VLAPIC_VERSION);

     for ( i = 0; i < 8; i++ )

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

Re: [PATCH RFC 2/8] libxl: remove whitespace on libxl_types.idl

[Joao Martins]

On 02/25/2016 04:28 PM, Wei Liu wrote:
> On Mon, Feb 22, 2016 at 09:02:08PM +0000, Joao Martins wrote:
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> 
> Acked-by: Wei Liu <wei.liu2@citrix.com>
> 
Thanks!

>> ---
>> CC: Ian Jackson <ian.jackson@eu.citrix.com>
>> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>> CC: Ian Campbell <ian.campbell@citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> ---
>>  tools/libxl/libxl_types.idl | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
>> index 9ad7eba..f04279e 100644
>> --- a/tools/libxl/libxl_types.idl
>> +++ b/tools/libxl/libxl_types.idl
>> @@ -436,7 +436,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>>      ("blkdev_start",    string),
>>  
>>      ("vnuma_nodes", Array(libxl_vnode_info, "num_vnuma_nodes")),
>> -    
>> +
>>      ("device_model_version", libxl_device_model_version),
>>      ("device_model_stubdomain", libxl_defbool),
>>      # if you set device_model you must set device_model_version too
>> @@ -497,10 +497,10 @@ libxl_domain_build_info = Struct("domain_build_info",[
>>                                         ("keymap",           string),
>>                                         ("sdl",              libxl_sdl_info),
>>                                         ("spice",            libxl_spice_info),
>> -                                       
>> +
>>                                         ("gfx_passthru",     libxl_defbool),
>>                                         ("gfx_passthru_kind", libxl_gfx_passthru_kind),
>> -                                       
>> +
>>                                         ("serial",           string),
>>                                         ("boot",             string),
>>                                         ("usb",              libxl_defbool),
>> -- 
>> 2.1.4
>>

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

Re: [PATCH RFC 4/8] libxl: cpuid: add guest topology support

[Joao Martins]

On 02/25/2016 04:29 PM, Wei Liu wrote:
> On Mon, Feb 22, 2016 at 09:02:10PM +0000, Joao Martins wrote:
>> Introduce internal cpuid routine for setting the topology
>> as seen by the guest. The topology is made based on
>> leaf 1 and leaf 4 for Intel, more specifically setting:
>>
>> Number of logical processors:
>> 	proccount  (CPUID.1:EBX[16:24])
>>
>> Number of physical cores - 1:
>> 	procpkg    (CPUID.(4,0):EBX[26:32])
>>
>> cache core count - 1:
>> 	proccountX (CPUID.(4,Y):EBX[14:26])
>>
>> 	given that X is l1d, l1i, l2 or l3
>> 	and Y the correspondent subleave [0-3]
>>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> ---
>> CC: Ian Jackson <ian.jackson@eu.citrix.com>
>> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>> CC: Ian Campbell <ian.campbell@citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> ---
>>  tools/libxl/libxl_cpuid.c    | 38 ++++++++++++++++++++++++++++++++++++++
>>  tools/libxl/libxl_internal.h |  2 ++
>>  2 files changed, 40 insertions(+)
>>
>> diff --git a/tools/libxl/libxl_cpuid.c b/tools/libxl/libxl_cpuid.c
>> index deb81d2..e220566 100644
>> --- a/tools/libxl/libxl_cpuid.c
>> +++ b/tools/libxl/libxl_cpuid.c
>> @@ -352,6 +352,44 @@ void libxl_cpuid_set(libxl_ctx *ctx, uint32_t domid,
>>                       (const char**)(cpuid[i].policy), cpuid_res);
>>  }
>>  
>> +static int libxl_cpuid_parse_list(libxl_cpuid_policy_list *topo,
>> +                                  char **keys, int *vals, size_t sz)
> 
> Just call it cpuid_parse_list is fine.
> 
OK.

> Wei.
> 

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

Re: [PATCH RFC 7/8] libxl: introduce topology fields

[Joao Martins]

On 02/25/2016 04:29 PM, Wei Liu wrote:
> On Mon, Feb 22, 2016 at 09:02:13PM +0000, Joao Martins wrote:
>> Currently there is "smt" option that changes from a flat core topology
>> to a core+thread topology. This patch adds more expressive options for
>> describing the topology as seen by the guest i.e. sockets, cores and
>> threads to adjust cpu topology as seen by the guest.
>>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> ---
>> CC: Ian Jackson <ian.jackson@eu.citrix.com>
>> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>> CC: Ian Campbell <ian.campbell@citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> ---
>>  tools/libxl/libxl_dom.c     | 18 ++++++++++++------
>>  tools/libxl/libxl_types.idl |  4 ++++
>>  2 files changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
>> index ff9356d..1e6d9ab 100644
>> --- a/tools/libxl/libxl_dom.c
>> +++ b/tools/libxl/libxl_dom.c
>> @@ -507,14 +507,20 @@ int libxl__build_post(libxl__gc *gc, uint32_t domid,
>>      }
>>  
>>      libxl_cpuid_apply_policy(ctx, domid);
>> -    if (info->type == LIBXL_DOMAIN_TYPE_HVM
>> -        && libxl_defbool_val(info->smt)) {
>> +    if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
>>  
>> -        uint32_t threads = 0;
>> +        uint32_t threads = 0, cores = 0;
>> +
>> +        if (libxl_defbool_val(info->smt)
>> +            && !libxl__count_threads_per_core(gc, &threads))
>> +            cores = info->max_vcpus / threads;
>> +        else if (info->topology.cores) {
>> +            cores = info->topology.cores;
>> +            threads = info->topology.threads;
>> +        }
>>  
>> -        if (!libxl__count_threads_per_core(gc, &threads))
>> -            libxl__cpuid_set_topology(ctx, domid,
>> -                                      info->max_vcpus / threads, threads);
>> +        if (cores && threads)
>> +            libxl__cpuid_set_topology(ctx, domid, cores, threads);
>>      }
>>  
>>      if (info->cpuid != NULL)
>> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
>> index fa4725a..caba626 100644
>> --- a/tools/libxl/libxl_types.idl
>> +++ b/tools/libxl/libxl_types.idl
>> @@ -422,6 +422,10 @@ libxl_domain_build_info = Struct("domain_build_info",[
>>      ("vcpu_hard_affinity", Array(libxl_bitmap, "num_vcpu_hard_affinity")),
>>      ("vcpu_soft_affinity", Array(libxl_bitmap, "num_vcpu_soft_affinity")),
>>      ("smt",             libxl_defbool),
>> +    ("topology",        Struct(None, [("sockets", integer),
>> +                                      ("cores",   integer),
>> +                                      ("threads", integer),
>> +                                      ])),
> 
> Maybe having topology is a good enough sophisticated configuration
> interface? (See my previous email)
Yeah. That's what I think too, though I proposed both to see what you folks
would prefer. For other potentially users of libxl (libvirt) the topology
field(s) is also a better fit. BTW, the reason for the smt parameter was mostly
for the default case, where it's a single flat topology.

>>      ("numa_placement",  libxl_defbool),
>>      ("tsc_mode",        libxl_tsc_mode),
>>      ("max_memkb",       MemKB),
>> -- 
>> 2.1.4
>>

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

Re: [PATCH RFC 0/8] x86/hvm, libxl: HVM SMT topology support

[Joao Martins]

On 02/25/2016 05:21 PM, Andrew Cooper wrote:
> On 22/02/16 21:02, Joao Martins wrote:
>> Hey!
>>
>> This series are a follow-up on the thread about the performance
>> of hard-pinned HVM guests. Here we propose allowing libxl to
>> change how the CPU topology looks like for the HVM guest, which can 
>> favor certain workloads as depicted by Elena on this thread [0]. 
>> It shows around 22-23% gain on io bound workloads having the guest
>> vCPUs hard pinned to the pCPUs with a matching core+thread.
>>
>> This series is divided as following:
>> * Patch 1     : Sets initial apicid to be the vcpuid as opposed
>>                 to vcpuid * 2 for each core;
>> * Patch 2     : Whitespace cleanup
>> * Patch 3     : Adds new leafs to describe Intel/AMD cache
>>                 topology. Though it's only internal to libxl;
>> * Patch 4     : Internal call to set per package CPUID values.
>> * Patch 5 - 8 : Interfaces for xl and libxl for setting topology.
>>
>> I couldn't quite figure out which user interface was better so I
>> included both our "smt" option and full description of the topology
>> i.e. "sockets", "cores", "threads" option same as the "-smp"
>> option on QEMU. Note that the latter could also be used on
>> libvirt since topology is described in their XML configs.
>>
>> It's also an RFC as AMD support isn't implemented yet.
>>
>> Any comments are appreciated!
> 
> Hey.  Sorry I am late getting to this - I am currently swamped.  Some
> general observations.
Hey Andrew, Thanks for the pointers!

> 
> The cpuid policy code in Xen was never re-thought through after
> multi-vcpu guests were introduced, which means they have no
> understanding of per-package, per-core and per-thread values.
> 
> As part of my further cpuid work, I will need to fix this.  I was
> planning to fix it by requiring full cpu topology information to be
> passed as part of the domaincreate or max_vcpus hypercall  (not chosen
> which yet).  This would include cores-per-package, threads-per-core etc,
> and allow Xen to correctly fill in the per-core cpuid values in leaves
> 4, 0xB and 80000008.
FWIW CPU topology on domaincreate sounds nice. Or would max_vcpus hypercall
serve other purposes too? (CPU hotplug, migration)

> 
> In particular, I am concerned about giving the toolstack the ability to
> blindly control the APIC IDs.  Their layout is very closely linked to
> topology, and in particular to the HTT flag.
> 
> Overall, I want to avoid any possibility of generating APIC layouts
> (including the emulated IOAPIC with HVM guests) which don't conform to
> the appropriate AMD/Intel manuals.
I see so overall having Xen control the topology would be a better approach that
"mangling" the APICIDs in the cpuid policy as I am proposing. One good thing
about Xen handling the topology bits would be for Intel CPUs with CPUID faulting
support where PV guests could also see the topology info. And given that the
word 10 of hw_caps won't be exposed (as per your CPUID), handling the PV case on
cpuid policy wouldn't be as clean.

Joao

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

Re: [PATCH RFC 0/8] x86/hvm, libxl: HVM SMT topology support

[Andrew Cooper]

On 02/03/16 19:18, Joao Martins wrote:
>
> On 02/25/2016 05:21 PM, Andrew Cooper wrote:
>> On 22/02/16 21:02, Joao Martins wrote:
>>> Hey!
>>>
>>> This series are a follow-up on the thread about the performance
>>> of hard-pinned HVM guests. Here we propose allowing libxl to
>>> change how the CPU topology looks like for the HVM guest, which can 
>>> favor certain workloads as depicted by Elena on this thread [0]. 
>>> It shows around 22-23% gain on io bound workloads having the guest
>>> vCPUs hard pinned to the pCPUs with a matching core+thread.
>>>
>>> This series is divided as following:
>>> * Patch 1     : Sets initial apicid to be the vcpuid as opposed
>>>                 to vcpuid * 2 for each core;
>>> * Patch 2     : Whitespace cleanup
>>> * Patch 3     : Adds new leafs to describe Intel/AMD cache
>>>                 topology. Though it's only internal to libxl;
>>> * Patch 4     : Internal call to set per package CPUID values.
>>> * Patch 5 - 8 : Interfaces for xl and libxl for setting topology.
>>>
>>> I couldn't quite figure out which user interface was better so I
>>> included both our "smt" option and full description of the topology
>>> i.e. "sockets", "cores", "threads" option same as the "-smp"
>>> option on QEMU. Note that the latter could also be used on
>>> libvirt since topology is described in their XML configs.
>>>
>>> It's also an RFC as AMD support isn't implemented yet.
>>>
>>> Any comments are appreciated!
>> Hey.  Sorry I am late getting to this - I am currently swamped.  Some
>> general observations.
> Hey Andrew, Thanks for the pointers!
>
>> The cpuid policy code in Xen was never re-thought through after
>> multi-vcpu guests were introduced, which means they have no
>> understanding of per-package, per-core and per-thread values.
>>
>> As part of my further cpuid work, I will need to fix this.  I was
>> planning to fix it by requiring full cpu topology information to be
>> passed as part of the domaincreate or max_vcpus hypercall  (not chosen
>> which yet).  This would include cores-per-package, threads-per-core etc,
>> and allow Xen to correctly fill in the per-core cpuid values in leaves
>> 4, 0xB and 80000008.
> FWIW CPU topology on domaincreate sounds nice. Or would max_vcpus hypercall
> serve other purposes too? (CPU hotplug, migration)

With cpu hotplug, a guest is still limited at max_vcpus, and this
hypercall is the second action during domain creation.

With migration, an empty domain must already be created for the contents
of the stream to be inserted into.  At a minimum, this is createdomain
and max_vcpus, usually with a max_mem to avoid it getting arbitrarily large.

One (mis)feature I want to fix is that currently, the cpuid policy is
regenerated by the toolstack on the destination of the migration, after
the cpu state has been reloaded in Xen.  This causes a chicken and egg
problem between checking the validity of guest state, such as %cr4
against the guest cpuid policy.

I wish to fix this by putting the domain cpuid policy at the head of the
migration stream, which allows the receiving side to first verify that
the domains cpuid policy is compatible with the host, and then verify
all further migration state against the policy.

Even with this, there will be a chicken and egg situation when it comes
to specifying topology.  The best that we can do is let the toolstack
recreate it from scratch (from what is hopefully the same domain
configuration at a higher level), then verify consistency when the
policy is loaded.

>
>> In particular, I am concerned about giving the toolstack the ability to
>> blindly control the APIC IDs.  Their layout is very closely linked to
>> topology, and in particular to the HTT flag.
>>
>> Overall, I want to avoid any possibility of generating APIC layouts
>> (including the emulated IOAPIC with HVM guests) which don't conform to
>> the appropriate AMD/Intel manuals.
> I see so overall having Xen control the topology would be a better approach that
> "mangling" the APICIDs in the cpuid policy as I am proposing. One good thing
> about Xen handling the topology bits would be for Intel CPUs with CPUID faulting
> support where PV guests could also see the topology info. And given that the
> word 10 of hw_caps won't be exposed (as per your CPUID), handling the PV case on
> cpuid policy wouldn't be as clean.

Which word do you mean here?  Even before my series, Xen only had 9
words in hw_cap.

~Andrew

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

Re: [PATCH RFC 0/8] x86/hvm, libxl: HVM SMT topology support

[Joao Martins]

On 03/02/2016 08:03 PM, Andrew Cooper wrote:
> On 02/03/16 19:18, Joao Martins wrote:
>>
>> On 02/25/2016 05:21 PM, Andrew Cooper wrote:
>>> On 22/02/16 21:02, Joao Martins wrote:
>>>> Hey!
>>>>
>>>> This series are a follow-up on the thread about the performance
>>>> of hard-pinned HVM guests. Here we propose allowing libxl to
>>>> change how the CPU topology looks like for the HVM guest, which can 
>>>> favor certain workloads as depicted by Elena on this thread [0]. 
>>>> It shows around 22-23% gain on io bound workloads having the guest
>>>> vCPUs hard pinned to the pCPUs with a matching core+thread.
>>>>
>>>> This series is divided as following:
>>>> * Patch 1     : Sets initial apicid to be the vcpuid as opposed
>>>>                 to vcpuid * 2 for each core;
>>>> * Patch 2     : Whitespace cleanup
>>>> * Patch 3     : Adds new leafs to describe Intel/AMD cache
>>>>                 topology. Though it's only internal to libxl;
>>>> * Patch 4     : Internal call to set per package CPUID values.
>>>> * Patch 5 - 8 : Interfaces for xl and libxl for setting topology.
>>>>
>>>> I couldn't quite figure out which user interface was better so I
>>>> included both our "smt" option and full description of the topology
>>>> i.e. "sockets", "cores", "threads" option same as the "-smp"
>>>> option on QEMU. Note that the latter could also be used on
>>>> libvirt since topology is described in their XML configs.
>>>>
>>>> It's also an RFC as AMD support isn't implemented yet.
>>>>
>>>> Any comments are appreciated!
>>> Hey.  Sorry I am late getting to this - I am currently swamped.  Some
>>> general observations.
>> Hey Andrew, Thanks for the pointers!
>>
>>> The cpuid policy code in Xen was never re-thought through after
>>> multi-vcpu guests were introduced, which means they have no
>>> understanding of per-package, per-core and per-thread values.
>>>
>>> As part of my further cpuid work, I will need to fix this.  I was
>>> planning to fix it by requiring full cpu topology information to be
>>> passed as part of the domaincreate or max_vcpus hypercall  (not chosen
>>> which yet).  This would include cores-per-package, threads-per-core etc,
>>> and allow Xen to correctly fill in the per-core cpuid values in leaves
>>> 4, 0xB and 80000008.
>> FWIW CPU topology on domaincreate sounds nice. Or would max_vcpus hypercall
>> serve other purposes too? (CPU hotplug, migration)
> 
> With cpu hotplug, a guest is still limited at max_vcpus, and this
> hypercall is the second action during domain creation.
OK

> 
> With migration, an empty domain must already be created for the contents
> of the stream to be inserted into.  At a minimum, this is createdomain
> and max_vcpus, usually with a max_mem to avoid it getting arbitrarily large.
> 
> One (mis)feature I want to fix is that currently, the cpuid policy is
> regenerated by the toolstack on the destination of the migration, after
> the cpu state has been reloaded in Xen.  This causes a chicken and egg
> problem between checking the validity of guest state, such as %cr4
> against the guest cpuid policy.
> 
> I wish to fix this by putting the domain cpuid policy at the head of the
> migration stream, which allows the receiving side to first verify that
> the domains cpuid policy is compatible with the host, and then verify
> all further migration state against the policy.
> 
> Even with this, there will be a chicken and egg situation when it comes
> to specifying topology.  The best that we can do is let the toolstack
> recreate it from scratch (from what is hopefully the same domain
> configuration at a higher level), then verify consistency when the
> policy is loaded.
/nods Thanks for educating on this.

> 
>>
>>> In particular, I am concerned about giving the toolstack the ability to
>>> blindly control the APIC IDs.  Their layout is very closely linked to
>>> topology, and in particular to the HTT flag.
>>>
>>> Overall, I want to avoid any possibility of generating APIC layouts
>>> (including the emulated IOAPIC with HVM guests) which don't conform to
>>> the appropriate AMD/Intel manuals.
>> I see so overall having Xen control the topology would be a better approach that
>> "mangling" the APICIDs in the cpuid policy as I am proposing. One good thing
>> about Xen handling the topology bits would be for Intel CPUs with CPUID faulting
>> support where PV guests could also see the topology info. And given that the
>> word 10 of hw_caps won't be exposed (as per your CPUID), handling the PV case on
>> cpuid policy wouldn't be as clean.
> 
> Which word do you mean here?  Even before my series, Xen only had 9
> words in hw_cap.
Hm, I used the wrong nomenclature here: what I meant was the 10th feature word
from x86_boot_capability (since the sysctl/libxl are capped to 8 words only)
which in the header files is word 9 on your series (previously moved from word
3). It's the one meant for "Other features, Linux-defined mapping", where
X86_FEATURE_CPUID_FAULTING is defined.

Joao

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

Re: [PATCH RFC 0/8] x86/hvm, libxl: HVM SMT topology support

[Andrew Cooper]

On 03/03/16 09:52, Joao Martins wrote:
>
>>>> In particular, I am concerned about giving the toolstack the ability to
>>>> blindly control the APIC IDs.  Their layout is very closely linked to
>>>> topology, and in particular to the HTT flag.
>>>>
>>>> Overall, I want to avoid any possibility of generating APIC layouts
>>>> (including the emulated IOAPIC with HVM guests) which don't conform to
>>>> the appropriate AMD/Intel manuals.
>>> I see so overall having Xen control the topology would be a better approach that
>>> "mangling" the APICIDs in the cpuid policy as I am proposing. One good thing
>>> about Xen handling the topology bits would be for Intel CPUs with CPUID faulting
>>> support where PV guests could also see the topology info. And given that the
>>> word 10 of hw_caps won't be exposed (as per your CPUID), handling the PV case on
>>> cpuid policy wouldn't be as clean.
>> Which word do you mean here?  Even before my series, Xen only had 9
>> words in hw_cap.
> Hm, I used the wrong nomenclature here: what I meant was the 10th feature word
> from x86_boot_capability (since the sysctl/libxl are capped to 8 words only)
> which in the header files is word 9 on your series (previously moved from word
> 3). It's the one meant for "Other features, Linux-defined mapping", where
> X86_FEATURE_CPUID_FAULTING is defined.

Ah - so the word of synthetic values.

I don't see how the lack of that word makes policy handling any harder? 
All information in there is gathered from other sources.

~Andrew

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

Re: [PATCH RFC 0/8] x86/hvm, libxl: HVM SMT topology support

[Joao Martins]

On 03/03/2016 10:24 AM, Andrew Cooper wrote:
> On 03/03/16 09:52, Joao Martins wrote:
>>
>>>>> In particular, I am concerned about giving the toolstack the ability to
>>>>> blindly control the APIC IDs.  Their layout is very closely linked to
>>>>> topology, and in particular to the HTT flag.
>>>>>
>>>>> Overall, I want to avoid any possibility of generating APIC layouts
>>>>> (including the emulated IOAPIC with HVM guests) which don't conform to
>>>>> the appropriate AMD/Intel manuals.
>>>> I see so overall having Xen control the topology would be a better approach that
>>>> "mangling" the APICIDs in the cpuid policy as I am proposing. One good thing
>>>> about Xen handling the topology bits would be for Intel CPUs with CPUID faulting
>>>> support where PV guests could also see the topology info. And given that the
>>>> word 10 of hw_caps won't be exposed (as per your CPUID), handling the PV case on
>>>> cpuid policy wouldn't be as clean.
>>> Which word do you mean here?  Even before my series, Xen only had 9
>>> words in hw_cap.
>> Hm, I used the wrong nomenclature here: what I meant was the 10th feature word
>> from x86_boot_capability (since the sysctl/libxl are capped to 8 words only)
>> which in the header files is word 9 on your series (previously moved from word
>> 3). It's the one meant for "Other features, Linux-defined mapping", where
>> X86_FEATURE_CPUID_FAULTING is defined.
> 
> Ah - so the word of synthetic values.
> 
> I don't see how the lack of that word makes policy handling any harder? 
> All information in there is gathered from other sources.
I meant specifically with the approach of my series i.e. changing cpuid policy
for HVM guests. PV guests cpuid is only trapped with CPUID_FAULTING so adding
support for PV guests topology on this toolstack-based approach we would need to
perhaps expose CPUID_FAULTING support somehow in physinfo so that the toolstack
know when to safely set the guest topology.

Joao

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

Re: [PATCH RFC 0/8] x86/hvm, libxl: HVM SMT topology support

[Andrew Cooper]

On 03/03/16 12:23, Joao Martins wrote:
>
> On 03/03/2016 10:24 AM, Andrew Cooper wrote:
>> On 03/03/16 09:52, Joao Martins wrote:
>>>>>> In particular, I am concerned about giving the toolstack the ability to
>>>>>> blindly control the APIC IDs.  Their layout is very closely linked to
>>>>>> topology, and in particular to the HTT flag.
>>>>>>
>>>>>> Overall, I want to avoid any possibility of generating APIC layouts
>>>>>> (including the emulated IOAPIC with HVM guests) which don't conform to
>>>>>> the appropriate AMD/Intel manuals.
>>>>> I see so overall having Xen control the topology would be a better approach that
>>>>> "mangling" the APICIDs in the cpuid policy as I am proposing. One good thing
>>>>> about Xen handling the topology bits would be for Intel CPUs with CPUID faulting
>>>>> support where PV guests could also see the topology info. And given that the
>>>>> word 10 of hw_caps won't be exposed (as per your CPUID), handling the PV case on
>>>>> cpuid policy wouldn't be as clean.
>>>> Which word do you mean here?  Even before my series, Xen only had 9
>>>> words in hw_cap.
>>> Hm, I used the wrong nomenclature here: what I meant was the 10th feature word
>>> from x86_boot_capability (since the sysctl/libxl are capped to 8 words only)
>>> which in the header files is word 9 on your series (previously moved from word
>>> 3). It's the one meant for "Other features, Linux-defined mapping", where
>>> X86_FEATURE_CPUID_FAULTING is defined.
>> Ah - so the word of synthetic values.
>>
>> I don't see how the lack of that word makes policy handling any harder? 
>> All information in there is gathered from other sources.
> I meant specifically with the approach of my series i.e. changing cpuid policy
> for HVM guests. PV guests cpuid is only trapped with CPUID_FAULTING so adding
> support for PV guests topology on this toolstack-based approach we would need to
> perhaps expose CPUID_FAULTING support somehow in physinfo so that the toolstack
> know when to safely set the guest topology.

My v1 series contained

http://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=commitdiff;h=c1b38a3b3b77ebe4e801906a3f67db9faa4c8003

which was intended to provide this information for the toolstack.  It
got reviewed out in v2 due to a lack of a consumer, but I would prefer
to reintroduce it if possible.  I guess I will.

~Andrew

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