| Message ID | 87k2h8ufw0.fsf@vitty.brq.redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 29/06/16 17:19, Vitaly Kuznetsov wrote: > 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 The code in hvmloader which sets up the MADT does: for ( i = 0; i < hvm_info->nr_vcpus; i++ ) { memset(lapic, 0, sizeof(*lapic)); lapic->type = ACPI_PROCESSOR_LOCAL_APIC; lapic->length = sizeof(*lapic); /* Processor ID must match processor-object IDs in the DSDT. */ lapic->acpi_processor_id = i; lapic->apic_id = LAPIC_ID(i); lapic->flags = (test_bit(i, hvm_info->vcpu_online) ? ACPI_LOCAL_APIC_ENABLED : 0); lapic++; } So relying on the acpi_processor_id does look to be reliable. That code hasn't changed since 2007, and that was only a bugfix. I would go so far as to say it is reasonable for us to guarantee this in the guest ABI. Jan - thoughts? ~Andrew
>>> On 29.06.16 at 18:27, <andrew.cooper3@citrix.com> wrote: > On 29/06/16 17:19, Vitaly Kuznetsov wrote: >> 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 > > The code in hvmloader which sets up the MADT does: > > for ( i = 0; i < hvm_info->nr_vcpus; i++ ) > { > memset(lapic, 0, sizeof(*lapic)); > lapic->type = ACPI_PROCESSOR_LOCAL_APIC; > lapic->length = sizeof(*lapic); > /* Processor ID must match processor-object IDs in the DSDT. */ > lapic->acpi_processor_id = i; > lapic->apic_id = LAPIC_ID(i); > lapic->flags = (test_bit(i, hvm_info->vcpu_online) > ? ACPI_LOCAL_APIC_ENABLED : 0); > lapic++; > } > > So relying on the acpi_processor_id does look to be reliable. That code > hasn't changed since 2007, and that was only a bugfix. I would go so > far as to say it is reasonable for us to guarantee this in the guest ABI. In fact - is there any other way a guest could learn the vCPU IDs of its CPUs in a reliable way? I don't think so, and hence this de facto already is part of the ABI; we should of course spell it out somewhere. Jan
"Jan Beulich" <JBeulich@suse.com> writes: >>>> On 29.06.16 at 18:27, <andrew.cooper3@citrix.com> wrote: >> On 29/06/16 17:19, Vitaly Kuznetsov wrote: >>> 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 >> >> The code in hvmloader which sets up the MADT does: >> >> for ( i = 0; i < hvm_info->nr_vcpus; i++ ) >> { >> memset(lapic, 0, sizeof(*lapic)); >> lapic->type = ACPI_PROCESSOR_LOCAL_APIC; >> lapic->length = sizeof(*lapic); >> /* Processor ID must match processor-object IDs in the DSDT. */ >> lapic->acpi_processor_id = i; >> lapic->apic_id = LAPIC_ID(i); >> lapic->flags = (test_bit(i, hvm_info->vcpu_online) >> ? ACPI_LOCAL_APIC_ENABLED : 0); >> lapic++; >> } >> >> So relying on the acpi_processor_id does look to be reliable. That code >> hasn't changed since 2007, and that was only a bugfix. I would go so >> far as to say it is reasonable for us to guarantee this in the guest ABI. > > In fact - is there any other way a guest could learn the vCPU IDs > of its CPUs in a reliable way? I don't think so, and hence this de > facto already is part of the ABI; we should of course spell it out > somewhere. I'm unsure about the right place in the hypervisor tree to put this information to. At the very least users of VCPUOP_* and EVTCHNOP_* hypervcalls need to know this so xen/include/public/{event_channel.h, vcpu.h} are the candidates. Or is there a better place?
>>> On 01.07.16 at 14:06, <vkuznets@redhat.com> wrote: > "Jan Beulich" <JBeulich@suse.com> writes: > >>>>> On 29.06.16 at 18:27, <andrew.cooper3@citrix.com> wrote: >>> On 29/06/16 17:19, Vitaly Kuznetsov wrote: >>>> 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 >>> >>> The code in hvmloader which sets up the MADT does: >>> >>> for ( i = 0; i < hvm_info->nr_vcpus; i++ ) >>> { >>> memset(lapic, 0, sizeof(*lapic)); >>> lapic->type = ACPI_PROCESSOR_LOCAL_APIC; >>> lapic->length = sizeof(*lapic); >>> /* Processor ID must match processor-object IDs in the DSDT. */ >>> lapic->acpi_processor_id = i; >>> lapic->apic_id = LAPIC_ID(i); >>> lapic->flags = (test_bit(i, hvm_info->vcpu_online) >>> ? ACPI_LOCAL_APIC_ENABLED : 0); >>> lapic++; >>> } >>> >>> So relying on the acpi_processor_id does look to be reliable. That code >>> hasn't changed since 2007, and that was only a bugfix. I would go so >>> far as to say it is reasonable for us to guarantee this in the guest ABI. >> >> In fact - is there any other way a guest could learn the vCPU IDs >> of its CPUs in a reliable way? I don't think so, and hence this de >> facto already is part of the ABI; we should of course spell it out >> somewhere. > > I'm unsure about the right place in the hypervisor tree to put this > information to. At the very least users of VCPUOP_* and EVTCHNOP_* > hypervcalls need to know this so xen/include/public/{event_channel.h, > vcpu.h} are the candidates. Or is there a better place? I'd probably put this in public/hvm/hvm_info_table.h or public/hvm/hvm_vcpu.h; the two headers you name shouldn't be cluttered with information like this not pertaining to all guests. Jan
On Thu, Jun 30, 2016 at 03:10:11AM -0600, Jan Beulich wrote: > >>> On 29.06.16 at 18:27, <andrew.cooper3@citrix.com> wrote: > > On 29/06/16 17:19, Vitaly Kuznetsov wrote: > >> 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 > > > > The code in hvmloader which sets up the MADT does: > > > > for ( i = 0; i < hvm_info->nr_vcpus; i++ ) > > { > > memset(lapic, 0, sizeof(*lapic)); > > lapic->type = ACPI_PROCESSOR_LOCAL_APIC; > > lapic->length = sizeof(*lapic); > > /* Processor ID must match processor-object IDs in the DSDT. */ > > lapic->acpi_processor_id = i; > > lapic->apic_id = LAPIC_ID(i); > > lapic->flags = (test_bit(i, hvm_info->vcpu_online) > > ? ACPI_LOCAL_APIC_ENABLED : 0); > > lapic++; > > } > > > > So relying on the acpi_processor_id does look to be reliable. That code > > hasn't changed since 2007, and that was only a bugfix. I would go so > > far as to say it is reasonable for us to guarantee this in the guest ABI. > > In fact - is there any other way a guest could learn the vCPU IDs > of its CPUs in a reliable way? I don't think so, and hence this de > facto already is part of the ABI; we should of course spell it out > somewhere. CCing Joao. Joao worked (and I think he posted an RFC patchset?) where this is changed so that the true hardware topology (core, thread, etc) is exposed. This is obviously for cases where you want pinning. I would hesistate to spell this out as an ABI.. P.S. Which reminds me, Joao, you OK posting the patchset? > > Jan > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
>>> On 05.07.16 at 17:34, <konrad.wilk@oracle.com> wrote: > On Thu, Jun 30, 2016 at 03:10:11AM -0600, Jan Beulich wrote: >> >>> On 29.06.16 at 18:27, <andrew.cooper3@citrix.com> wrote: >> > On 29/06/16 17:19, Vitaly Kuznetsov wrote: >> >> 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 >> > >> > The code in hvmloader which sets up the MADT does: >> > >> > for ( i = 0; i < hvm_info->nr_vcpus; i++ ) >> > { >> > memset(lapic, 0, sizeof(*lapic)); >> > lapic->type = ACPI_PROCESSOR_LOCAL_APIC; >> > lapic->length = sizeof(*lapic); >> > /* Processor ID must match processor-object IDs in the DSDT. */ >> > lapic->acpi_processor_id = i; >> > lapic->apic_id = LAPIC_ID(i); >> > lapic->flags = (test_bit(i, hvm_info->vcpu_online) >> > ? ACPI_LOCAL_APIC_ENABLED : 0); >> > lapic++; >> > } >> > >> > So relying on the acpi_processor_id does look to be reliable. That code >> > hasn't changed since 2007, and that was only a bugfix. I would go so >> > far as to say it is reasonable for us to guarantee this in the guest ABI. >> >> In fact - is there any other way a guest could learn the vCPU IDs >> of its CPUs in a reliable way? I don't think so, and hence this de >> facto already is part of the ABI; we should of course spell it out >> somewhere. > > CCing Joao. > > Joao worked (and I think he posted an RFC patchset?) where this is changed so > that the true hardware topology (core, thread, etc) is exposed. This is obviously > for cases where you want pinning. > > I would hesistate to spell this out as an ABI.. Are you perhaps mixing up ACPI and APIC IDs? Here talk is of the former, while Joao's patch set was about the latter iirc. Jan
On 07/05/2016 04:44 PM, Jan Beulich wrote: >>>> On 05.07.16 at 17:34, <konrad.wilk@oracle.com> wrote: >> On Thu, Jun 30, 2016 at 03:10:11AM -0600, Jan Beulich wrote: >>>>>> On 29.06.16 at 18:27, <andrew.cooper3@citrix.com> wrote: >>>> On 29/06/16 17:19, Vitaly Kuznetsov wrote: >>>>> 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 >>>> >>>> The code in hvmloader which sets up the MADT does: >>>> >>>> for ( i = 0; i < hvm_info->nr_vcpus; i++ ) >>>> { >>>> memset(lapic, 0, sizeof(*lapic)); >>>> lapic->type = ACPI_PROCESSOR_LOCAL_APIC; >>>> lapic->length = sizeof(*lapic); >>>> /* Processor ID must match processor-object IDs in the DSDT. */ >>>> lapic->acpi_processor_id = i; >>>> lapic->apic_id = LAPIC_ID(i); >>>> lapic->flags = (test_bit(i, hvm_info->vcpu_online) >>>> ? ACPI_LOCAL_APIC_ENABLED : 0); >>>> lapic++; >>>> } >>>> >>>> So relying on the acpi_processor_id does look to be reliable. That code >>>> hasn't changed since 2007, and that was only a bugfix. I would go so >>>> far as to say it is reasonable for us to guarantee this in the guest ABI. >>> >>> In fact - is there any other way a guest could learn the vCPU IDs >>> of its CPUs in a reliable way? I don't think so, and hence this de >>> facto already is part of the ABI; we should of course spell it out >>> somewhere. >> >> CCing Joao. >> >> Joao worked (and I think he posted an RFC patchset?) where this is changed so >> that the true hardware topology (core, thread, etc) is exposed. This is obviously >> for cases where you want pinning. >> >> I would hesistate to spell this out as an ABI.. > > Are you perhaps mixing up ACPI and APIC IDs? Here talk is of the > former, while Joao's patch set was about the latter iirc. > Correct, my patch series indeed changed APIC IDs. I guess making acpi_processor_id to get vcpu_id reliably stated as guest ABI, is safe wrt to future arrangements to apic_ids. Joao
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