All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/1] x86: centralize default APIC id definition
@ 2021-09-24 19:39 Alex Olson
  2021-09-24 19:39 ` [PATCH 1/1] " Alex Olson
  0 siblings, 1 reply; 7+ messages in thread
From: Alex Olson @ 2021-09-24 19:39 UTC (permalink / raw)
  To: xen-devel
  Cc: Alex Olson, Jan Beulich, Andrew Cooper, Roger Pau Monné,
	Wei Liu, Ian Jackson, George Dunlap, Julien Grall,
	Stefano Stabellini, Juergen Gross, Anthony PERARD

I am interested in making the x86 topology seen by guests more flexible.
This patch keeps the original functionality but allows the APIC identifier
seen by guests for each vCPU to be altered more easily in future revisions.

Since the same mapping of vcpu_id to vlapic id is currently preserved,
the existing adjustments for 'logical processors per package' are left unchanged.

Alex Olson (1):
  x86: centralize default APIC id definition

 tools/firmware/hvmloader/Makefile      |  2 +-
 tools/firmware/hvmloader/config.h      |  1 -
 tools/firmware/hvmloader/hvmloader.c   |  3 +-
 tools/firmware/hvmloader/mp_tables.c   |  3 +-
 tools/firmware/hvmloader/smp.c         |  3 +-
 tools/firmware/hvmloader/topology.c    | 54 ++++++++++++++++++++++++++
 tools/firmware/hvmloader/topology.h    | 33 ++++++++++++++++
 tools/firmware/hvmloader/util.c        |  6 ++-
 tools/include/xenctrl.h                |  6 +++
 tools/libacpi/build.c                  |  4 +-
 tools/libacpi/libacpi.h                |  3 +-
 tools/libs/ctrl/xc_domain.c            | 27 +++++++++++++
 tools/libs/light/libxl_x86_acpi.c      |  9 ++++-
 xen/arch/x86/cpuid.c                   | 14 +++++--
 xen/arch/x86/hvm/hvm.c                 | 36 ++++++++++++++++-
 xen/arch/x86/hvm/vlapic.c              | 18 ++++++---
 xen/include/asm-x86/hvm/vlapic.h       |  4 +-
 xen/include/public/arch-x86/hvm/save.h |  1 +
 xen/include/public/hvm/hvm_op.h        | 17 ++++++++
 19 files changed, 222 insertions(+), 22 deletions(-)
 create mode 100644 tools/firmware/hvmloader/topology.c
 create mode 100644 tools/firmware/hvmloader/topology.h

-- 
2.25.1



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

* [PATCH 1/1] x86: centralize default APIC id definition
  2021-09-24 19:39 [PATCH 0/1] x86: centralize default APIC id definition Alex Olson
@ 2021-09-24 19:39 ` Alex Olson
  2021-10-01 14:19   ` Jan Beulich
  0 siblings, 1 reply; 7+ messages in thread
From: Alex Olson @ 2021-09-24 19:39 UTC (permalink / raw)
  To: xen-devel
  Cc: Alex Olson, Jan Beulich, Andrew Cooper, Roger Pau Monné,
	Wei Liu, Ian Jackson, George Dunlap, Julien Grall,
	Stefano Stabellini, Juergen Gross, Anthony PERARD, Alex Olson

Inspired by an earlier attempt by Chao Gao <chao.gao@intel.com>,
this revision aims to put the hypervisor in control of x86 APIC identifier
definition instead of hard-coding a formula in multiple places
(libxl, hvmloader, hypervisor).

This is intended as a first step toward exposing/altering CPU topology
seen by guests.

Changes:

- Add field to vlapic for holding default ID (on reset)

- add HVMOP_get_vcpu_topology_id hypercall so libxl (for PVH domains)
  can access APIC ids needed for ACPI table definition prior to domain start.

- For HVM guests, hvmloader now also uses the same hypercall.

- Make CPUID code use vlapic ID instead of hard-coded formula
  for runtime reporting to guests

Signed-off-by: Alex Olson <alex.olson@starlab.io>
---
 tools/firmware/hvmloader/Makefile      |  2 +-
 tools/firmware/hvmloader/config.h      |  1 -
 tools/firmware/hvmloader/hvmloader.c   |  3 +-
 tools/firmware/hvmloader/mp_tables.c   |  3 +-
 tools/firmware/hvmloader/smp.c         |  3 +-
 tools/firmware/hvmloader/topology.c    | 54 ++++++++++++++++++++++++++
 tools/firmware/hvmloader/topology.h    | 33 ++++++++++++++++
 tools/firmware/hvmloader/util.c        |  6 ++-
 tools/include/xenctrl.h                |  6 +++
 tools/libacpi/build.c                  |  4 +-
 tools/libacpi/libacpi.h                |  3 +-
 tools/libs/ctrl/xc_domain.c            | 27 +++++++++++++
 tools/libs/light/libxl_x86_acpi.c      |  9 ++++-
 xen/arch/x86/cpuid.c                   | 14 +++++--
 xen/arch/x86/hvm/hvm.c                 | 36 ++++++++++++++++-
 xen/arch/x86/hvm/vlapic.c              | 18 ++++++---
 xen/include/asm-x86/hvm/vlapic.h       |  4 +-
 xen/include/public/arch-x86/hvm/save.h |  1 +
 xen/include/public/hvm/hvm_op.h        | 17 ++++++++
 19 files changed, 222 insertions(+), 22 deletions(-)
 create mode 100644 tools/firmware/hvmloader/topology.c
 create mode 100644 tools/firmware/hvmloader/topology.h

diff --git a/tools/firmware/hvmloader/Makefile b/tools/firmware/hvmloader/Makefile
index e980ce7c5f..158f62b4e6 100644
--- a/tools/firmware/hvmloader/Makefile
+++ b/tools/firmware/hvmloader/Makefile
@@ -29,7 +29,7 @@ CFLAGS += $(CFLAGS_xeninclude)
 CFLAGS += -D__XEN_INTERFACE_VERSION__=__XEN_LATEST_INTERFACE_VERSION__
 
 OBJS  = hvmloader.o mp_tables.o util.o smbios.o 
-OBJS += smp.o cacheattr.o xenbus.o vnuma.o
+OBJS += smp.o cacheattr.o xenbus.o vnuma.o topology.o
 OBJS += e820.o pci.o pir.o ctype.o
 OBJS += hvm_param.o
 OBJS += ovmf.o seabios.o
diff --git a/tools/firmware/hvmloader/config.h b/tools/firmware/hvmloader/config.h
index 844120bc87..91d73c9086 100644
--- a/tools/firmware/hvmloader/config.h
+++ b/tools/firmware/hvmloader/config.h
@@ -50,7 +50,6 @@ extern uint8_t ioapic_version;
 #define IOAPIC_ID           0x01
 
 #define LAPIC_BASE_ADDRESS  0xfee00000
-#define LAPIC_ID(vcpu_id)   ((vcpu_id) * 2)
 
 #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/tools/firmware/hvmloader/hvmloader.c b/tools/firmware/hvmloader/hvmloader.c
index c58841e5b5..250e6779b1 100644
--- a/tools/firmware/hvmloader/hvmloader.c
+++ b/tools/firmware/hvmloader/hvmloader.c
@@ -25,6 +25,7 @@
 #include "pci_regs.h"
 #include "apic_regs.h"
 #include "vnuma.h"
+#include "topology.h"
 #include <acpi2_0.h>
 #include <xen/version.h>
 #include <xen/hvm/params.h>
@@ -225,7 +226,7 @@ static void apic_setup(void)
 
     /* 8259A ExtInts are delivered through IOAPIC pin 0 (Virtual Wire Mode). */
     ioapic_write(0x10, APIC_DM_EXTINT);
-    ioapic_write(0x11, SET_APIC_ID(LAPIC_ID(0)));
+    ioapic_write(0x11, SET_APIC_ID(get_topology_id(0)));
 }
 
 struct bios_info {
diff --git a/tools/firmware/hvmloader/mp_tables.c b/tools/firmware/hvmloader/mp_tables.c
index d207ecbf00..562961ca0b 100644
--- a/tools/firmware/hvmloader/mp_tables.c
+++ b/tools/firmware/hvmloader/mp_tables.c
@@ -29,6 +29,7 @@
 
 #include <stdint.h>
 #include "config.h"
+#include "topology.h"
 
 /* number of non-processor MP table entries */
 #define NR_NONPROC_ENTRIES     18
@@ -199,7 +200,7 @@ static void fill_mp_config_table(struct mp_config_table *mpct, int length)
 static void fill_mp_proc_entry(struct mp_proc_entry *mppe, int vcpu_id)
 {
     mppe->type = ENTRY_TYPE_PROCESSOR;
-    mppe->lapic_id = LAPIC_ID(vcpu_id);
+    mppe->lapic_id = get_topology_id(vcpu_id);
     mppe->lapic_version = 0x11;
     mppe->cpu_flags = CPU_FLAG_ENABLED;
     if ( vcpu_id == 0 )
diff --git a/tools/firmware/hvmloader/smp.c b/tools/firmware/hvmloader/smp.c
index 082b17f138..5091ff1f1f 100644
--- a/tools/firmware/hvmloader/smp.c
+++ b/tools/firmware/hvmloader/smp.c
@@ -22,6 +22,7 @@
 #include "util.h"
 #include "config.h"
 #include "apic_regs.h"
+#include "topology.h"
 
 #define AP_BOOT_EIP 0x1000
 extern char ap_boot_start[], ap_boot_end[];
@@ -86,7 +87,7 @@ static void lapic_wait_ready(void)
 
 static void boot_cpu(unsigned int cpu)
 {
-    unsigned int icr2 = SET_APIC_DEST_FIELD(LAPIC_ID(cpu));
+    unsigned int icr2 = SET_APIC_DEST_FIELD(get_topology_id(cpu));
 
     /* Initialise shared variables. */
     ap_cpuid = cpu;
diff --git a/tools/firmware/hvmloader/topology.c b/tools/firmware/hvmloader/topology.c
new file mode 100644
index 0000000000..f92bbb8b7a
--- /dev/null
+++ b/tools/firmware/hvmloader/topology.c
@@ -0,0 +1,54 @@
+/*
+ * topology.c: obtain vCPU topology from hypervisor
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY AUTHOR AND CONTRIBUTORS ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED.  IN NO EVENT SHALL AUTHOR OR CONTRIBUTORS BE LIABLE
+ * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
+ * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
+ * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
+ * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#include "util.h"
+#include "hypercall.h"
+#include "topology.h"
+#include <xen/hvm/hvm_op.h>
+
+uint32_t get_topology_id(unsigned vcpu_id)
+{
+    int rc;
+    struct xen_vcpu_topology_id tid =
+        { .domid = DOMID_SELF, .vcpu_id = vcpu_id };
+
+    rc = hypercall_hvm_op(HVMOP_get_vcpu_topology_id, &tid);
+    if ( rc < 0 )
+    {
+        printf("Failed to retrieve cpu topology for vcpu=%u, rc = %d\n", vcpu_id, rc);
+        return 0xdeadbeef;
+    }
+    return tid.topology_id;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/tools/firmware/hvmloader/topology.h b/tools/firmware/hvmloader/topology.h
new file mode 100644
index 0000000000..6e37051556
--- /dev/null
+++ b/tools/firmware/hvmloader/topology.h
@@ -0,0 +1,33 @@
+/******************************************************************************
+ * topology.h
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version 2
+ * as published by the Free Software Foundation; or, when distributed
+ * separately from the Linux kernel or incorporated into other
+ * software packages, subject to the following license:
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this source file (the "Software"), to deal in the Software without
+ * restriction, including without limitation the rights to use, copy, modify,
+ * merge, publish, distribute, sublicense, and/or sell copies of the Software,
+ * and to permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+#ifndef __HVMLOADER_TOPOLOGY_H_
+#define __HVMLOADER_TOPOLOGY_H_
+
+uint32_t get_topology_id(unsigned vcpu_id);
+
+#endif /* __HVMLOADER_TOPOLOGY_H__ */
diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
index 581b35e5cf..a5fe15d198 100644
--- a/tools/firmware/hvmloader/util.c
+++ b/tools/firmware/hvmloader/util.c
@@ -22,6 +22,7 @@
 #include "hypercall.h"
 #include "ctype.h"
 #include "vnuma.h"
+#include "topology.h"
 #include <acpi2_0.h>
 #include <libacpi.h>
 #include <stdint.h>
@@ -910,9 +911,9 @@ static void acpi_mem_free(struct acpi_ctxt *ctxt,
     /* ACPI builder currently doesn't free memory so this is just a stub */
 }
 
-static uint32_t acpi_lapic_id(unsigned cpu)
+static uint32_t acpi_lapic_id(unsigned cpu, void *arg)
 {
-    return LAPIC_ID(cpu);
+    return get_topology_id(cpu);
 }
 
 void hvmloader_acpi_build_tables(struct acpi_config *config,
@@ -943,6 +944,7 @@ void hvmloader_acpi_build_tables(struct acpi_config *config,
 
     config->lapic_base_address = LAPIC_BASE_ADDRESS;
     config->lapic_id = acpi_lapic_id;
+    config->lapic_id_arg = NULL;
     config->ioapic_base_address = ioapic_base_address;
     config->ioapic_id = IOAPIC_ID;
     config->pci_isa_irq_mask = PCI_ISA_IRQ_MASK; 
diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index a3063998e0..c15a2bb0c8 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -1262,6 +1262,12 @@ int xc_domain_set_memory_map(xc_interface *xch,
 int xc_get_machine_memory_map(xc_interface *xch,
                               struct e820entry entries[],
                               uint32_t max_entries);
+
+int xc_get_vcpu_topology_id(xc_interface *xch,
+                            uint32_t domid,
+                            unsigned int vcpu_id,
+                            uint32_t *topology_id);
+
 #endif
 
 int xc_reserved_device_memory_map(xc_interface *xch,
diff --git a/tools/libacpi/build.c b/tools/libacpi/build.c
index fe2db66a62..aa5e851172 100644
--- a/tools/libacpi/build.c
+++ b/tools/libacpi/build.c
@@ -156,7 +156,7 @@ static struct acpi_20_madt *construct_madt(struct acpi_ctxt *ctxt,
         lapic->length  = sizeof(*lapic);
         /* Processor ID must match processor-object IDs in the DSDT. */
         lapic->acpi_processor_id = i;
-        lapic->apic_id = config->lapic_id(i);
+        lapic->apic_id = config->lapic_id(i, config->lapic_id_arg);
         lapic->flags = (test_bit(i, hvminfo->vcpu_online)
                         ? ACPI_LOCAL_APIC_ENABLED : 0);
         lapic++;
@@ -244,7 +244,7 @@ static struct acpi_20_srat *construct_srat(struct acpi_ctxt *ctxt,
         processor->type     = ACPI_PROCESSOR_AFFINITY;
         processor->length   = sizeof(*processor);
         processor->domain   = config->numa.vcpu_to_vnode[i];
-        processor->apic_id  = config->lapic_id(i);
+        processor->apic_id  = config->lapic_id(i, config->lapic_id_arg);
         processor->flags    = ACPI_LOCAL_APIC_AFFIN_ENABLED;
         processor++;
     }
diff --git a/tools/libacpi/libacpi.h b/tools/libacpi/libacpi.h
index a2efd23b0b..c66d1f0ea1 100644
--- a/tools/libacpi/libacpi.h
+++ b/tools/libacpi/libacpi.h
@@ -91,7 +91,8 @@ struct acpi_config {
     unsigned long rsdp;
 
     /* x86-specific parameters */
-    uint32_t (*lapic_id)(unsigned cpu);
+    uint32_t (*lapic_id)(unsigned cpu, void *arg);
+    void     *lapic_id_arg;
     uint32_t lapic_base_address;
     uint32_t ioapic_base_address;
     uint16_t pci_isa_irq_mask;
diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c
index 23322b70b5..087196130c 100644
--- a/tools/libs/ctrl/xc_domain.c
+++ b/tools/libs/ctrl/xc_domain.c
@@ -1506,6 +1506,33 @@ int xc_get_hvm_param(xc_interface *handle, uint32_t dom, int param, unsigned lon
     return 0;
 }
 
+
+#if defined (__i386__) || defined (__x86_64__)
+int xc_get_vcpu_topology_id(xc_interface *xch,
+                            uint32_t dom,
+                            unsigned int vcpu_id,
+                            uint32_t *topology_id)
+{
+    DECLARE_HYPERCALL_BUFFER(xen_vcpu_topology_id_t, arg);
+    int rc;
+
+    arg = xc_hypercall_buffer_alloc(xch, arg, sizeof(*arg));
+    if ( arg == NULL )
+        return -1;
+
+    arg->domid = dom;
+    arg->vcpu_id = vcpu_id;
+    arg->topology_id = 0xdeadbeef;
+
+    rc = xencall2(xch->xcall, __HYPERVISOR_hvm_op,
+                  HVMOP_get_vcpu_topology_id,
+                  HYPERCALL_BUFFER_AS_ARG(arg));
+    *topology_id = arg->topology_id;
+    xc_hypercall_buffer_free(xch, arg);
+    return rc;
+}
+#endif
+
 int xc_domain_setdebugging(xc_interface *xch,
                            uint32_t domid,
                            unsigned int enable)
diff --git a/tools/libs/light/libxl_x86_acpi.c b/tools/libs/light/libxl_x86_acpi.c
index 3eca1c7a9f..8e471f222f 100644
--- a/tools/libs/light/libxl_x86_acpi.c
+++ b/tools/libs/light/libxl_x86_acpi.c
@@ -79,9 +79,13 @@ static void acpi_mem_free(struct acpi_ctxt *ctxt,
 {
 }
 
-static uint32_t acpi_lapic_id(unsigned cpu)
+static uint32_t acpi_lapic_id(unsigned cpu, void *arg)
 {
-    return cpu * 2;
+    struct xc_dom_image *dom = (struct xc_dom_image *)arg;
+    uint32_t ret = 0xdeadbeef;
+    int rc;
+    rc = xc_get_vcpu_topology_id(dom->xch, dom->guest_domid, cpu, &ret);
+    return ret;
 }
 
 static int init_acpi_config(libxl__gc *gc, 
@@ -149,6 +153,7 @@ static int init_acpi_config(libxl__gc *gc,
 
     config->lapic_base_address = LAPIC_BASE_ADDRESS;
     config->lapic_id = acpi_lapic_id;
+    config->lapic_id_arg = dom;
     config->acpi_revision = 5;
 
     rc = 0;
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 2079a30ae4..cbed233726 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -867,8 +867,10 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
     case 0x1:
         /* TODO: Rework topology logic. */
         res->b &= 0x00ffffffu;
-        if ( is_hvm_domain(d) )
-            res->b |= (v->vcpu_id * 2) << 24;
+
+#ifdef CONFIG_HVM
+        res->b |= vlapic_get_default_id(v) << 24;
+#endif
 
         /* TODO: Rework vPMU control in terms of toolstack choices. */
         if ( vpmu_available(v) &&
@@ -1049,7 +1051,13 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
             *(uint8_t *)&res->c = subleaf;
 
             /* Fix the x2APIC identifier. */
-            res->d = v->vcpu_id * 2;
+#ifdef CONFIG_HVM
+            res->d = vlapic_get_default_id(v);
+#endif
+        }
+        else
+        {
+            *res = EMPTY_LEAF;
         }
         break;
 
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 7b48a1b925..e93ca8187d 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1555,7 +1555,7 @@ int hvm_vcpu_initialise(struct vcpu *v)
         goto fail1;
 
     /* NB: vlapic_init must be called before hvm_funcs.vcpu_initialise */
-    rc = vlapic_init(v);
+    rc = vlapic_init(v, v->vcpu_id * 2);
     if ( rc != 0 ) /* teardown: vlapic_destroy */
         goto fail2;
 
@@ -5084,6 +5084,40 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
         rc = current->hcall_compat ? compat_altp2m_op(arg) : do_altp2m_op(arg);
         break;
 
+    case HVMOP_get_vcpu_topology_id:
+    {
+        struct domain *d;
+        struct xen_vcpu_topology_id tid;
+
+        if ( copy_from_guest(&tid, arg, 1) )
+            return -EFAULT;
+
+        if (tid.domid != DOMID_SELF && !is_hardware_domain(current->domain))
+            return -EPERM;
+
+        if ( (d = rcu_lock_domain_by_any_id(tid.domid)) == NULL )
+            return -ESRCH;
+
+        if ( !is_hvm_domain(d))
+        {
+            rc = -EOPNOTSUPP;
+            goto get_cpu_topology_failed;
+        }
+
+        if ( tid.vcpu_id >= d->max_vcpus )
+        {
+            rc = -EINVAL;
+            goto get_cpu_topology_failed;
+        }
+        tid.topology_id = vlapic_get_default_id(d->vcpu[tid.vcpu_id]);
+
+        rc = copy_to_guest(arg, &tid, 1) ? -EFAULT : 0;
+
+ get_cpu_topology_failed:
+        rcu_unlock_domain(d);
+        break;
+    }
+
     default:
     {
         gdprintk(XENLOG_DEBUG, "Bad HVM op %ld.\n", op);
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 5e21fb4937..e65b1e8e94 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -1068,12 +1068,18 @@ static const struct hvm_mmio_ops vlapic_mmio_ops = {
     .write = vlapic_mmio_write,
 };
 
+uint32_t vlapic_get_default_id(const struct vcpu *v)
+{
+    const struct vlapic *vlapic = vcpu_vlapic(v);
+    return vlapic->hw.default_id;
+}
+
 static void set_x2apic_id(struct vlapic *vlapic)
 {
-    u32 id = vlapic_vcpu(vlapic)->vcpu_id;
+    u32 id = vlapic->hw.default_id;
     u32 ldr = ((id & ~0xf) << 12) | (1 << (id & 0xf));
 
-    vlapic_set_reg(vlapic, APIC_ID, id * 2);
+    vlapic_set_reg(vlapic, APIC_ID, id);
     vlapic_set_reg(vlapic, APIC_LDR, ldr);
 }
 
@@ -1440,7 +1446,7 @@ void vlapic_reset(struct vlapic *vlapic)
     if ( v->vcpu_id == 0 )
         vlapic->hw.apic_base_msr |= APIC_BASE_BSP;
 
-    vlapic_set_reg(vlapic, APIC_ID, (v->vcpu_id * 2) << 24);
+    vlapic_set_reg(vlapic, APIC_ID, vlapic->hw.default_id << 24);
     vlapic_do_init(vlapic);
 }
 
@@ -1508,7 +1514,7 @@ static void lapic_load_fixup(struct vlapic *vlapic)
          * here, but can be dropped as soon as it is found to conflict with
          * other (future) changes.
          */
-        if ( GET_xAPIC_ID(id) != vlapic_vcpu(vlapic)->vcpu_id * 2 ||
+        if ( GET_xAPIC_ID(id) != vlapic->hw.default_id ||
              id != SET_xAPIC_ID(GET_xAPIC_ID(id)) )
             printk(XENLOG_G_WARNING "%pv: bogus APIC ID %#x loaded\n",
                    vlapic_vcpu(vlapic), id);
@@ -1596,12 +1602,14 @@ HVM_REGISTER_SAVE_RESTORE(LAPIC, lapic_save_hidden,
 HVM_REGISTER_SAVE_RESTORE(LAPIC_REGS, lapic_save_regs,
                           lapic_load_regs, 1, HVMSR_PER_VCPU);
 
-int vlapic_init(struct vcpu *v)
+int vlapic_init(struct vcpu *v, uint32_t apic_id)
 {
     struct vlapic *vlapic = vcpu_vlapic(v);
 
     HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "%d", v->vcpu_id);
 
+    vlapic->hw.default_id = apic_id;
+
     if ( !has_vlapic(v->domain) )
     {
         vlapic->hw.disabled = VLAPIC_HW_DISABLED;
diff --git a/xen/include/asm-x86/hvm/vlapic.h b/xen/include/asm-x86/hvm/vlapic.h
index 8f908928c3..06c9c5cd7d 100644
--- a/xen/include/asm-x86/hvm/vlapic.h
+++ b/xen/include/asm-x86/hvm/vlapic.h
@@ -116,7 +116,7 @@ void vlapic_set_irq(struct vlapic *vlapic, uint8_t vec, uint8_t trig);
 int vlapic_has_pending_irq(struct vcpu *v);
 int vlapic_ack_pending_irq(struct vcpu *v, int vector, bool_t force_ack);
 
-int  vlapic_init(struct vcpu *v);
+int  vlapic_init(struct vcpu *v, uint32_t apic_id);
 void vlapic_destroy(struct vcpu *v);
 
 void vlapic_reset(struct vlapic *vlapic);
@@ -154,4 +154,6 @@ static inline void vlapic_sync_pir_to_irr(struct vcpu *v)
         alternative_vcall(hvm_funcs.sync_pir_to_irr, v);
 }
 
+uint32_t vlapic_get_default_id(const struct vcpu *v);
+
 #endif /* __ASM_X86_HVM_VLAPIC_H__ */
diff --git a/xen/include/public/arch-x86/hvm/save.h b/xen/include/public/arch-x86/hvm/save.h
index 773a380bc2..f960257dbf 100644
--- a/xen/include/public/arch-x86/hvm/save.h
+++ b/xen/include/public/arch-x86/hvm/save.h
@@ -411,6 +411,7 @@ struct hvm_hw_lapic {
     uint32_t             disabled; /* VLAPIC_xx_DISABLED */
     uint32_t             timer_divisor;
     uint64_t             tdt_msr;
+    uint32_t             default_id;
 };
 
 DECLARE_HVM_SAVE_TYPE(LAPIC, 5, struct hvm_hw_lapic);
diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
index 870ec52060..bf7872a4d1 100644
--- a/xen/include/public/hvm/hvm_op.h
+++ b/xen/include/public/hvm/hvm_op.h
@@ -183,6 +183,23 @@ struct xen_hvm_get_mem_type {
 typedef struct xen_hvm_get_mem_type xen_hvm_get_mem_type_t;
 DEFINE_XEN_GUEST_HANDLE(xen_hvm_get_mem_type_t);
 
+/*
+ * HVMOP_get_cpu_topology is used by guest to acquire vcpu topology from
+ * hypervisor.
+ */
+#define HVMOP_get_vcpu_topology_id     16
+
+struct xen_vcpu_topology_id {
+    /* IN */
+    domid_t domid;
+    uint32_t vcpu_id;
+
+    /* OUT */
+    uint32_t topology_id;
+};
+typedef struct xen_vcpu_topology_id xen_vcpu_topology_id_t;
+DEFINE_XEN_GUEST_HANDLE(xen_vcpu_topology_id_t);
+
 /* Following tools-only interfaces may change in future. */
 #if defined(__XEN__) || defined(__XEN_TOOLS__)
 
-- 
2.25.1



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

* Re: [PATCH 1/1] x86: centralize default APIC id definition
  2021-09-24 19:39 ` [PATCH 1/1] " Alex Olson
@ 2021-10-01 14:19   ` Jan Beulich
  2021-10-01 14:29     ` Andrew Cooper
  0 siblings, 1 reply; 7+ messages in thread
From: Jan Beulich @ 2021-10-01 14:19 UTC (permalink / raw)
  To: Alex Olson
  Cc: Andrew Cooper, Roger Pau Monné,
	Wei Liu, Ian Jackson, George Dunlap, Julien Grall,
	Stefano Stabellini, Juergen Gross, Anthony PERARD, Alex Olson,
	xen-devel

On 24.09.2021 21:39, Alex Olson wrote:
> Inspired by an earlier attempt by Chao Gao <chao.gao@intel.com>,
> this revision aims to put the hypervisor in control of x86 APIC identifier
> definition instead of hard-coding a formula in multiple places
> (libxl, hvmloader, hypervisor).
> 
> This is intended as a first step toward exposing/altering CPU topology
> seen by guests.
> 
> Changes:
> 
> - Add field to vlapic for holding default ID (on reset)
> 
> - add HVMOP_get_vcpu_topology_id hypercall so libxl (for PVH domains)
>   can access APIC ids needed for ACPI table definition prior to domain start.
> 
> - For HVM guests, hvmloader now also uses the same hypercall.
> 
> - Make CPUID code use vlapic ID instead of hard-coded formula
>   for runtime reporting to guests

I'm afraid a primary question from back at the time remains: How is
migration of a guest from an old hypervisor to one with this change
in place going to work?

> --- a/tools/libs/light/libxl_x86_acpi.c
> +++ b/tools/libs/light/libxl_x86_acpi.c
> @@ -79,9 +79,13 @@ static void acpi_mem_free(struct acpi_ctxt *ctxt,
>  {
>  }
>  
> -static uint32_t acpi_lapic_id(unsigned cpu)
> +static uint32_t acpi_lapic_id(unsigned cpu, void *arg)
>  {
> -    return cpu * 2;
> +    struct xc_dom_image *dom = (struct xc_dom_image *)arg;

No need for the cast.

> +    uint32_t ret = 0xdeadbeef;
> +    int rc;
> +    rc = xc_get_vcpu_topology_id(dom->xch, dom->guest_domid, cpu, &ret);
> +    return ret;
>  }

No need for the local variable "rc" if you don't evaluate it.

> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -867,8 +867,10 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
>      case 0x1:
>          /* TODO: Rework topology logic. */
>          res->b &= 0x00ffffffu;
> -        if ( is_hvm_domain(d) )
> -            res->b |= (v->vcpu_id * 2) << 24;
> +
> +#ifdef CONFIG_HVM
> +        res->b |= vlapic_get_default_id(v) << 24;
> +#endif

How come you drop the is_hvm_domain() here? There also should be no
need for such an #ifdef here ...

> @@ -1049,7 +1051,13 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
>              *(uint8_t *)&res->c = subleaf;
>  
>              /* Fix the x2APIC identifier. */
> -            res->d = v->vcpu_id * 2;
> +#ifdef CONFIG_HVM
> +            res->d = vlapic_get_default_id(v);
> +#endif

... or here.

> +        }
> +        else
> +        {
> +            *res = EMPTY_LEAF;
>          }

No need for braces in such a case.

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1555,7 +1555,7 @@ int hvm_vcpu_initialise(struct vcpu *v)
>          goto fail1;
>  
>      /* NB: vlapic_init must be called before hvm_funcs.vcpu_initialise */
> -    rc = vlapic_init(v);
> +    rc = vlapic_init(v, v->vcpu_id * 2);

Now that's odd: The goal of the patch is to eliminate such, and
here's you're adding a new instance?

> @@ -5084,6 +5084,40 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>          rc = current->hcall_compat ? compat_altp2m_op(arg) : do_altp2m_op(arg);
>          break;
>  
> +    case HVMOP_get_vcpu_topology_id:
> +    {
> +        struct domain *d;
> +        struct xen_vcpu_topology_id tid;
> +
> +        if ( copy_from_guest(&tid, arg, 1) )
> +            return -EFAULT;
> +
> +        if (tid.domid != DOMID_SELF && !is_hardware_domain(current->domain))

This wants to be a proper XSM check, I suppose, and allow more than
just the hardware domain to access this in case the controller of
the domain doesn't run in Dom0 (see XSM_TARGET).

Also, nit: Style (see all the other if()s).

> +            return -EPERM;
> +
> +        if ( (d = rcu_lock_domain_by_any_id(tid.domid)) == NULL )
> +            return -ESRCH;
> +
> +        if ( !is_hvm_domain(d))

Nit: Style again.

> +        {
> +            rc = -EOPNOTSUPP;
> +            goto get_cpu_topology_failed;
> +        }
> +
> +        if ( tid.vcpu_id >= d->max_vcpus )

Please use domain_vcpu() ...

> +        {
> +            rc = -EINVAL;
> +            goto get_cpu_topology_failed;
> +        }
> +        tid.topology_id = vlapic_get_default_id(d->vcpu[tid.vcpu_id]);

... to guard this array access.

> @@ -1508,7 +1514,7 @@ static void lapic_load_fixup(struct vlapic *vlapic)
>           * here, but can be dropped as soon as it is found to conflict with
>           * other (future) changes.
>           */
> -        if ( GET_xAPIC_ID(id) != vlapic_vcpu(vlapic)->vcpu_id * 2 ||
> +        if ( GET_xAPIC_ID(id) != vlapic->hw.default_id ||
>               id != SET_xAPIC_ID(GET_xAPIC_ID(id)) )
>              printk(XENLOG_G_WARNING "%pv: bogus APIC ID %#x loaded\n",
>                     vlapic_vcpu(vlapic), id);

As to my initial comment - I expect this warning will trigger for
about every vCPU of a guest migrating in from an older hypervisor.

> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -183,6 +183,23 @@ struct xen_hvm_get_mem_type {
>  typedef struct xen_hvm_get_mem_type xen_hvm_get_mem_type_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_hvm_get_mem_type_t);
>  
> +/*
> + * HVMOP_get_cpu_topology is used by guest to acquire vcpu topology from
> + * hypervisor.
> + */
> +#define HVMOP_get_vcpu_topology_id     16

Careful with the number choice here - 16 used to be HVMOP_inject_msi
until dm-op was introduced. Interfaces exposed to guests themselves
need to not invoke unexpected operations on older hypervisors.

> +struct xen_vcpu_topology_id {
> +    /* IN */
> +    domid_t domid;
> +    uint32_t vcpu_id;

Please make padding explict, checking it to be zero on input.

Jan



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

* Re: [PATCH 1/1] x86: centralize default APIC id definition
  2021-10-01 14:19   ` Jan Beulich
@ 2021-10-01 14:29     ` Andrew Cooper
  2021-10-01 15:08       ` Juergen Gross
  0 siblings, 1 reply; 7+ messages in thread
From: Andrew Cooper @ 2021-10-01 14:29 UTC (permalink / raw)
  To: Jan Beulich, Alex Olson
  Cc: Roger Pau Monné,
	Wei Liu, Ian Jackson, George Dunlap, Julien Grall,
	Stefano Stabellini, Juergen Gross, Anthony PERARD, Alex Olson,
	xen-devel

On 01/10/2021 15:19, Jan Beulich wrote:
> On 24.09.2021 21:39, Alex Olson wrote:
>> Inspired by an earlier attempt by Chao Gao <chao.gao@intel.com>,
>> this revision aims to put the hypervisor in control of x86 APIC identifier
>> definition instead of hard-coding a formula in multiple places
>> (libxl, hvmloader, hypervisor).
>>
>> This is intended as a first step toward exposing/altering CPU topology
>> seen by guests.
>>
>> Changes:
>>
>> - Add field to vlapic for holding default ID (on reset)
>>
>> - add HVMOP_get_vcpu_topology_id hypercall so libxl (for PVH domains)
>>   can access APIC ids needed for ACPI table definition prior to domain start.
>>
>> - For HVM guests, hvmloader now also uses the same hypercall.
>>
>> - Make CPUID code use vlapic ID instead of hard-coded formula
>>   for runtime reporting to guests
> I'm afraid a primary question from back at the time remains: How is
> migration of a guest from an old hypervisor to one with this change
> in place going to work?

I'm afraid its not.

Fixing this is incredibly complicated.  I have a vague plan, but it
needs building on the still-pending libxl cpuid work of Rogers.

Both the toolstack and Xen need to learn about how to describe topology
correctly (and I'm afraid this patch isn't correct even for a number of
the simple cases), and know about "every VM booted up until this point
in time" being wrong.

~Andrew


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

* Re: [PATCH 1/1] x86: centralize default APIC id definition
  2021-10-01 14:29     ` Andrew Cooper
@ 2021-10-01 15:08       ` Juergen Gross
  2021-10-01 17:38         ` Andrew Cooper
  0 siblings, 1 reply; 7+ messages in thread
From: Juergen Gross @ 2021-10-01 15:08 UTC (permalink / raw)
  To: Andrew Cooper, Jan Beulich, Alex Olson
  Cc: Roger Pau Monné,
	Wei Liu, Ian Jackson, George Dunlap, Julien Grall,
	Stefano Stabellini, Anthony PERARD, Alex Olson, xen-devel


[-- Attachment #1.1.1: Type: text/plain, Size: 1819 bytes --]

On 01.10.21 16:29, Andrew Cooper wrote:
> On 01/10/2021 15:19, Jan Beulich wrote:
>> On 24.09.2021 21:39, Alex Olson wrote:
>>> Inspired by an earlier attempt by Chao Gao <chao.gao@intel.com>,
>>> this revision aims to put the hypervisor in control of x86 APIC identifier
>>> definition instead of hard-coding a formula in multiple places
>>> (libxl, hvmloader, hypervisor).
>>>
>>> This is intended as a first step toward exposing/altering CPU topology
>>> seen by guests.
>>>
>>> Changes:
>>>
>>> - Add field to vlapic for holding default ID (on reset)
>>>
>>> - add HVMOP_get_vcpu_topology_id hypercall so libxl (for PVH domains)
>>>    can access APIC ids needed for ACPI table definition prior to domain start.
>>>
>>> - For HVM guests, hvmloader now also uses the same hypercall.
>>>
>>> - Make CPUID code use vlapic ID instead of hard-coded formula
>>>    for runtime reporting to guests
>> I'm afraid a primary question from back at the time remains: How is
>> migration of a guest from an old hypervisor to one with this change
>> in place going to work?
> 
> I'm afraid its not.
> 
> Fixing this is incredibly complicated.  I have a vague plan, but it
> needs building on the still-pending libxl cpuid work of Rogers.
> 
> Both the toolstack and Xen need to learn about how to describe topology
> correctly (and I'm afraid this patch isn't correct even for a number of
> the simple cases), and know about "every VM booted up until this point
> in time" being wrong.

What about:

- adding APIC-Id to the migration stream
- adding an optional translation layer for guest APIC-Id to the
   hypervisor
- adding the functionality to set a specific APIC-Id for a vcpu
   (will use the translation layer if not the same as preferred
   by the hypervisor)


Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

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

* Re: [PATCH 1/1] x86: centralize default APIC id definition
  2021-10-01 15:08       ` Juergen Gross
@ 2021-10-01 17:38         ` Andrew Cooper
  2021-10-01 19:07           ` Alex Olson
  0 siblings, 1 reply; 7+ messages in thread
From: Andrew Cooper @ 2021-10-01 17:38 UTC (permalink / raw)
  To: Juergen Gross, Jan Beulich, Alex Olson
  Cc: Roger Pau Monné,
	Wei Liu, Ian Jackson, George Dunlap, Julien Grall,
	Stefano Stabellini, Anthony PERARD, Alex Olson, xen-devel

On 01/10/2021 16:08, Juergen Gross wrote:
> On 01.10.21 16:29, Andrew Cooper wrote:
>> On 01/10/2021 15:19, Jan Beulich wrote:
>>> On 24.09.2021 21:39, Alex Olson wrote:
>>>> Inspired by an earlier attempt by Chao Gao <chao.gao@intel.com>,
>>>> this revision aims to put the hypervisor in control of x86 APIC
>>>> identifier
>>>> definition instead of hard-coding a formula in multiple places
>>>> (libxl, hvmloader, hypervisor).
>>>>
>>>> This is intended as a first step toward exposing/altering CPU topology
>>>> seen by guests.
>>>>
>>>> Changes:
>>>>
>>>> - Add field to vlapic for holding default ID (on reset)
>>>>
>>>> - add HVMOP_get_vcpu_topology_id hypercall so libxl (for PVH domains)
>>>>    can access APIC ids needed for ACPI table definition prior to
>>>> domain start.
>>>>
>>>> - For HVM guests, hvmloader now also uses the same hypercall.
>>>>
>>>> - Make CPUID code use vlapic ID instead of hard-coded formula
>>>>    for runtime reporting to guests
>>> I'm afraid a primary question from back at the time remains: How is
>>> migration of a guest from an old hypervisor to one with this change
>>> in place going to work?
>>
>> I'm afraid its not.
>>
>> Fixing this is incredibly complicated.  I have a vague plan, but it
>> needs building on the still-pending libxl cpuid work of Rogers.
>>
>> Both the toolstack and Xen need to learn about how to describe topology
>> correctly (and I'm afraid this patch isn't correct even for a number of
>> the simple cases), and know about "every VM booted up until this point
>> in time" being wrong.
>
> What about:
>
> - adding APIC-Id to the migration stream
> - adding an optional translation layer for guest APIC-Id to the
>   hypervisor
> - adding the functionality to set a specific APIC-Id for a vcpu
>   (will use the translation layer if not the same as preferred
>   by the hypervisor)

The vCPU APIC IDs are already in the migration stream.  They're just too
late in the stream for any easy fixup.

A second problem we have is that (x)APIC IDs are writeable under Xen,
but writeability of the register is a model specific trait to being
with.  Furthermore, you get potentially differing behaviour depending on
whether APICV is enabled or not.  I plan to fix this by simply outlawing
it - OSes don't renumber the APICs in the first place (just like they
don't move the MMIO window), and all they'll do is break things.

The main topology problem is that we have no interlink between the
CPUID-described data, and the default APIC IDs chosen.  There are 5
different algorithms to choose from (vendor and CPU dependent) and we
implement 0 of them.

The xl config file needs more than just cpuid= data to express the
topology correctly, because for non-power-of-two systems, there need to
be gaps in the APIC_ID space, and this needs communicating to Xen too. 
(For old AMD, we also need a slide, but we can probably leave that as an
exercise to anyone who cares, which seems to be noone so far).

Either way, when the toolstack can reason about topologies correctly, we
can extend the xl json in the stream.  The absense of the marker serves
as "This VM didn't boot with sane topology", which we can use the
fallback logic (see libxl__cpuid_legacy() and the soon to exist
companion in libxc) to re-synthesize the old pattern for when data is
missing in the stream.

Any change to the topology algorithms before the toolstack is capable of
doing everything else leaves us with two[1] different kinds of VMs that
we can't tell apart, and cannot cope with in a compatible way.

~Andrew

[1] Actually 3.  XenServer still has a revert of ca2eee92df44 in the
patchqueue because that broke VMs which migrated across the point.  As
it's from 2008, pre-and-post VMs aren't something we need to care about,
because anyone still running Xen 3.3 has far bigger problems than this
to worry about.



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

* Re: [PATCH 1/1] x86: centralize default APIC id definition
  2021-10-01 17:38         ` Andrew Cooper
@ 2021-10-01 19:07           ` Alex Olson
  0 siblings, 0 replies; 7+ messages in thread
From: Alex Olson @ 2021-10-01 19:07 UTC (permalink / raw)
  To: Andrew Cooper, Juergen Gross, Jan Beulich
  Cc: Roger Pau Monné,
	Wei Liu, Ian Jackson, George Dunlap, Julien Grall,
	Stefano Stabellini, Anthony PERARD, Alex Olson, xen-devel

I hadn't seriously considered the scenario of VM migration from older
Xen versions,  but If I understand Andrew's last statement correctly,
it sounds like my patch will break things in new ways that cannot be
fixed at this time. If that is the case, I'll abandon my efforts with
this patch. 

Thank you for the reviews and detailed feedback,

-Alex

On Fri, 2021-10-01 at 18:38 +0100, Andrew Cooper wrote:
> On 01/10/2021 16:08, Juergen Gross wrote:
> > On 01.10.21 16:29, Andrew Cooper wrote:
> > > On 01/10/2021 15:19, Jan Beulich wrote:
> > > > On 24.09.2021 21:39, Alex Olson wrote:
> > > > > Inspired by an earlier attempt by Chao Gao <
> > > > > chao.gao@intel.com>,
> > > > > this revision aims to put the hypervisor in control of x86
> > > > > APIC
> > > > > identifier
> > > > > definition instead of hard-coding a formula in multiple
> > > > > places
> > > > > (libxl, hvmloader, hypervisor).
> > > > > 
> > > > > This is intended as a first step toward exposing/altering CPU
> > > > > topology
> > > > > seen by guests.
> > > > > 
> > > > > Changes:
> > > > > 
> > > > > - Add field to vlapic for holding default ID (on reset)
> > > > > 
> > > > > - add HVMOP_get_vcpu_topology_id hypercall so libxl (for PVH
> > > > > domains)
> > > > >    can access APIC ids needed for ACPI table definition prior
> > > > > to
> > > > > domain start.
> > > > > 
> > > > > - For HVM guests, hvmloader now also uses the same hypercall.
> > > > > 
> > > > > - Make CPUID code use vlapic ID instead of hard-coded formula
> > > > >    for runtime reporting to guests
> > > > I'm afraid a primary question from back at the time remains:
> > > > How is
> > > > migration of a guest from an old hypervisor to one with this
> > > > change
> > > > in place going to work?
> > > 
> > > I'm afraid its not.
> > > 
> > > Fixing this is incredibly complicated.  I have a vague plan, but
> > > it
> > > needs building on the still-pending libxl cpuid work of Rogers.
> > > 
> > > Both the toolstack and Xen need to learn about how to describe
> > > topology
> > > correctly (and I'm afraid this patch isn't correct even for a
> > > number of
> > > the simple cases), and know about "every VM booted up until this
> > > point
> > > in time" being wrong.
> > 
> > What about:
> > 
> > - adding APIC-Id to the migration stream
> > - adding an optional translation layer for guest APIC-Id to the
> >   hypervisor
> > - adding the functionality to set a specific APIC-Id for a vcpu
> >   (will use the translation layer if not the same as preferred
> >   by the hypervisor)
> 
> The vCPU APIC IDs are already in the migration stream.  They're just
> too
> late in the stream for any easy fixup.
> 
> A second problem we have is that (x)APIC IDs are writeable under Xen,
> but writeability of the register is a model specific trait to being
> with.  Furthermore, you get potentially differing behaviour depending
> on
> whether APICV is enabled or not.  I plan to fix this by simply
> outlawing
> it - OSes don't renumber the APICs in the first place (just like they
> don't move the MMIO window), and all they'll do is break things.
> 
> The main topology problem is that we have no interlink between the
> CPUID-described data, and the default APIC IDs chosen.  There are 5
> different algorithms to choose from (vendor and CPU dependent) and we
> implement 0 of them.
> 
> The xl config file needs more than just cpuid= data to express the
> topology correctly, because for non-power-of-two systems, there need
> to
> be gaps in the APIC_ID space, and this needs communicating to Xen
> too. 
> (For old AMD, we also need a slide, but we can probably leave that as
> an
> exercise to anyone who cares, which seems to be noone so far).
> 
> Either way, when the toolstack can reason about topologies correctly,
> we
> can extend the xl json in the stream.  The absense of the marker
> serves
> as "This VM didn't boot with sane topology", which we can use the
> fallback logic (see libxl__cpuid_legacy() and the soon to exist
> companion in libxc) to re-synthesize the old pattern for when data is
> missing in the stream.
> 
> Any change to the topology algorithms before the toolstack is capable
> of
> doing everything else leaves us with two[1] different kinds of VMs
> that
> we can't tell apart, and cannot cope with in a compatible way.
> 
> ~Andrew
> 
> [1] Actually 3.  XenServer still has a revert of ca2eee92df44 in the
> patchqueue because that broke VMs which migrated across the point. 
> As
> it's from 2008, pre-and-post VMs aren't something we need to care
> about,
> because anyone still running Xen 3.3 has far bigger problems than
> this
> to worry about.
> 



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

end of thread, other threads:[~2021-10-01 19:08 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-24 19:39 [PATCH 0/1] x86: centralize default APIC id definition Alex Olson
2021-09-24 19:39 ` [PATCH 1/1] " Alex Olson
2021-10-01 14:19   ` Jan Beulich
2021-10-01 14:29     ` Andrew Cooper
2021-10-01 15:08       ` Juergen Gross
2021-10-01 17:38         ` Andrew Cooper
2021-10-01 19:07           ` Alex Olson

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