Re: [Xen-devel] [PATCH linux 2/8] xen: introduce xen_vcpu_id mapping

[Vitaly Kuznetsov <vkuznets@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2286 bytes --]

Vitaly Kuznetsov <vkuznets@redhat.com> writes:

> Andrew Cooper <andrew.cooper3@citrix.com> writes:
>
>> On 29/06/16 13:16, Vitaly Kuznetsov wrote:
>>> Andrew Cooper <andrew.cooper3@citrix.com> writes:
>>>
>>>> On 28/06/16 17:47, Vitaly Kuznetsov wrote:
>>>>> @@ -1808,6 +1822,8 @@ static int xen_hvm_cpu_notify(struct notifier_block *self, unsigned long action,
>>>>>  	int cpu = (long)hcpu;
>>>>>  	switch (action) {
>>>>>  	case CPU_UP_PREPARE:
>>>>> +		/* vLAPIC_ID == Xen's vCPU_ID * 2 for HVM guests */
>>>>> +		per_cpu(xen_vcpu_id, cpu) = cpu_physical_id(cpu) / 2;
>>>> Please do not assume or propagate this brokenness.  It is incorrect in
>>>> the general case, and I will be fixing in the hypervisor in due course.
>>>>
>>>> Always read the APIC_ID from the LAPIC, per regular hardware.
>>> (I'm probbaly missing something important - please bear with me)
>>>
>>> The problem here is that I need to get _other_ CPU's id before any code
>>> is executed on that CPU (or, at least, this is the current state of
>>> affairs if you look at xen_hvm_cpu_up()) so I can't use CPUID/do MSR
>>> reads/... The only option I see here is to rely on ACPI (MADT) data
>>> which is stored in x86_cpu_to_apicid (and that's what cpu_physical_id()
>>> gives us). MADT also has processor id which connects it to DSDT but I'm
>>> not sure Linux keeps this data. But this is something fixable I guess.
>>
>> Hmm yes - that is a tricky issue.
>>
>> It is not safe or correct to assume that xen_vcpu_id is APICID / 2.
>>
>> This is currently the case for most modern versions of Xen, but isn't
>> the case for older versions, and won't be the case in the future when I
>> (or someone else) fixes topology representation for guests.
>>
>> For this to work, we need one or more of:
>>
>> 1) to provide the guest a full mapping from APIC_ID to vcpu id at boot
>> time.
>
> So can we rely on ACPI data? Especially on MADT and processor ids there?
> I think we can always guarantee that processor ids there match vCPU
> ids. If yes I can try saving this data when we parse MADT.
>

To explain better what I'm trying to suggest here please take a look at
the attached patch. If we can guarantee long term that ACPI id always
equals to Xen's idea of vCPU id this is probably the easiest way.

-- 
  Vitaly


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-x86-acpi-store-APIC-ids-for-future-usage-use-it-in-x.patch --]
[-- Type: text/x-patch, Size: 5294 bytes --]

>From 7c9cd25bdcd3e6ee866aa49550c9a4160194c3ba Mon Sep 17 00:00:00 2001
From: Vitaly Kuznetsov <vkuznets@redhat.com>
Date: Wed, 29 Jun 2016 18:13:01 +0200
Subject: [PATCH RFC/WIP] x86/acpi: store APIC ids for future usage (+use it in
 xen_hvm_cpu_notify)

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 arch/x86/include/asm/smp.h     |  1 +
 arch/x86/kernel/acpi/boot.c    | 18 ++++++++++++++----
 arch/x86/kernel/setup_percpu.c |  3 +++
 arch/x86/xen/enlighten.c       |  8 ++++++--
 4 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index 66b0573..c68b56a 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -33,6 +33,7 @@ static inline struct cpumask *cpu_llc_shared_mask(int cpu)
 }
 
 DECLARE_EARLY_PER_CPU_READ_MOSTLY(u16, x86_cpu_to_apicid);
+DECLARE_EARLY_PER_CPU_READ_MOSTLY(int, x86_cpu_to_acpiid);
 DECLARE_EARLY_PER_CPU_READ_MOSTLY(u16, x86_bios_cpu_apicid);
 #if defined(CONFIG_X86_LOCAL_APIC) && defined(CONFIG_X86_32)
 DECLARE_EARLY_PER_CPU_READ_MOSTLY(int, x86_cpu_to_logical_apicid);
diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 9414f84..3bbf0ab 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -110,6 +110,9 @@ static u32 isa_irq_to_gsi[NR_IRQS_LEGACY] __read_mostly = {
 
 #define	ACPI_INVALID_GSI		INT_MIN
 
+DEFINE_EARLY_PER_CPU_READ_MOSTLY(int, x86_cpu_to_acpiid, -1);
+EXPORT_EARLY_PER_CPU_SYMBOL(x86_cpu_to_acpiid);
+
 /*
  * This is just a simple wrapper around early_ioremap(),
  * with sanity checks for phys == 0 and size == 0.
@@ -165,9 +168,10 @@ static int __init acpi_parse_madt(struct acpi_table_header *table)
  *
  * Returns the logic cpu number which maps to the local apic
  */
-static int acpi_register_lapic(int id, u8 enabled)
+static int acpi_register_lapic(int id, int acpiid, u8 enabled)
 {
 	unsigned int ver = 0;
+	int cpu;
 
 	if (id >= MAX_LOCAL_APIC) {
 		printk(KERN_INFO PREFIX "skipped apicid that is too big\n");
@@ -182,7 +186,11 @@ static int acpi_register_lapic(int id, u8 enabled)
 	if (boot_cpu_physical_apicid != -1U)
 		ver = apic_version[boot_cpu_physical_apicid];
 
-	return generic_processor_info(id, ver);
+	cpu = generic_processor_info(id, ver);
+	if (cpu >= 0)
+		early_per_cpu(x86_cpu_to_acpiid, cpu) = acpiid;
+
+	return cpu;
 }
 
 static int __init
@@ -212,7 +220,7 @@ acpi_parse_x2apic(struct acpi_subtable_header *header, const unsigned long end)
 	if (!apic->apic_id_valid(apic_id) && enabled)
 		printk(KERN_WARNING PREFIX "x2apic entry ignored\n");
 	else
-		acpi_register_lapic(apic_id, enabled);
+		acpi_register_lapic(apic_id, processor->uid, enabled);
 #else
 	printk(KERN_WARNING PREFIX "x2apic entry ignored\n");
 #endif
@@ -240,6 +248,7 @@ acpi_parse_lapic(struct acpi_subtable_header * header, const unsigned long end)
 	 * when we use CPU hotplug.
 	 */
 	acpi_register_lapic(processor->id,	/* APIC ID */
+			    processor->processor_id, /* ACPI ID */
 			    processor->lapic_flags & ACPI_MADT_ENABLED);
 
 	return 0;
@@ -258,6 +267,7 @@ acpi_parse_sapic(struct acpi_subtable_header *header, const unsigned long end)
 	acpi_table_print_madt_entry(header);
 
 	acpi_register_lapic((processor->id << 8) | processor->eid,/* APIC ID */
+			    processor->uid, /* ACPI ID */
 			    processor->lapic_flags & ACPI_MADT_ENABLED);
 
 	return 0;
@@ -714,7 +724,7 @@ int acpi_map_cpu(acpi_handle handle, phys_cpuid_t physid, int *pcpu)
 {
 	int cpu;
 
-	cpu = acpi_register_lapic(physid, ACPI_MADT_ENABLED);
+	cpu = acpi_register_lapic(physid, -1, ACPI_MADT_ENABLED);
 	if (cpu < 0) {
 		pr_info(PREFIX "Unable to map lapic to logical cpu number\n");
 		return cpu;
diff --git a/arch/x86/kernel/setup_percpu.c b/arch/x86/kernel/setup_percpu.c
index e4fcb87..7a40e06 100644
--- a/arch/x86/kernel/setup_percpu.c
+++ b/arch/x86/kernel/setup_percpu.c
@@ -236,6 +236,8 @@ void __init setup_per_cpu_areas(void)
 			early_per_cpu_map(x86_cpu_to_apicid, cpu);
 		per_cpu(x86_bios_cpu_apicid, cpu) =
 			early_per_cpu_map(x86_bios_cpu_apicid, cpu);
+		per_cpu(x86_cpu_to_acpiid, cpu) =
+			early_per_cpu_map(x86_cpu_to_acpiid, cpu);
 #endif
 #ifdef CONFIG_X86_32
 		per_cpu(x86_cpu_to_logical_apicid, cpu) =
@@ -271,6 +273,7 @@ void __init setup_per_cpu_areas(void)
 #ifdef CONFIG_X86_LOCAL_APIC
 	early_per_cpu_ptr(x86_cpu_to_apicid) = NULL;
 	early_per_cpu_ptr(x86_bios_cpu_apicid) = NULL;
+	early_per_cpu_ptr(x86_cpu_to_acpiid) = NULL;
 #endif
 #ifdef CONFIG_X86_32
 	early_per_cpu_ptr(x86_cpu_to_logical_apicid) = NULL;
diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index 0be29a0..8ae4871 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -1825,10 +1825,14 @@ static int xen_hvm_cpu_notify(struct notifier_block *self, unsigned long action,
 			      void *hcpu)
 {
 	int cpu = (long)hcpu;
+	int acpiid = per_cpu(x86_cpu_to_acpiid, cpu);
+
 	switch (action) {
 	case CPU_UP_PREPARE:
-		/* vLAPIC_ID == Xen's vCPU_ID * 2 for HVM guests */
-		per_cpu(xen_vcpu_id, cpu) = cpu_physical_id(cpu) / 2;
+		if (acpiid != -1)
+			per_cpu(xen_vcpu_id, cpu) = acpiid;
+		else
+			per_cpu(xen_vcpu_id, cpu) = cpu;
 		xen_vcpu_setup(cpu);
 		if (xen_have_vector_callback) {
 			if (xen_feature(XENFEAT_hvm_safe_pvclock))
-- 
2.5.5