Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753378AbcJFIHO (ORCPT ); Thu, 6 Oct 2016 04:07:14 -0400 Received: from cn.fujitsu.com ([59.151.112.132]:22451 "EHLO heian.cn.fujitsu.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1752332AbcJFIHL (ORCPT ); Thu, 6 Oct 2016 04:07:11 -0400 X-IronPort-AV: E=Sophos;i="5.22,518,1449504000"; d="scan'208";a="11676842" Subject: Re: [tip:x86/apic] x86/acpi: Introduce persistent storage for cpuid <-> apicid mapping To: Yinghai Lu , Thomas Gleixner References: <1472114120-3281-4-git-send-email-douly.fnst@cn.fujitsu.com> CC: Linux Kernel Mailing List , Ingo Molnar , "H. Peter Anvin" , "linux-tip-commits@vger.kernel.org" , Tony Luck From: Dou Liyang Message-ID: <4608f474-c49e-550b-90e2-c5f4c25e00f5@cn.fujitsu.com> Date: Thu, 6 Oct 2016 16:06:42 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.167.226.106] X-yoursite-MailScanner-ID: 113E5466F727.ABD66 X-yoursite-MailScanner: Found to be clean X-yoursite-MailScanner-From: douly.fnst@cn.fujitsu.com Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2937 Lines: 108 Hi Yinghai, At 10/06/2016 12:53 PM, Yinghai Lu wrote: > On Wed, Oct 5, 2016 at 7:04 AM, Thomas Gleixner wrote: >>> @@ -176,6 +177,11 @@ static int acpi_register_lapic(int id, u >>> return -EINVAL; >>> } >>> >>> + if (!enabled && (id == disabled_id)) { >>> + ++disabled_cpus; >>> + return -EINVAL; >>> + } >> >> Why would you need that disabled_id thing at all? The proper fix is to let >> the apic driver detect the issue and this boils down to a 5 lines >> change. Does the patch below fix the issue for you? >> 8<---------------- >> --- a/arch/x86/kernel/apic/apic.c >> +++ b/arch/x86/kernel/apic/apic.c >> @@ -2076,6 +2076,11 @@ int __generic_processor_info(int apicid, >> bool boot_cpu_detected = physid_isset(boot_cpu_physical_apicid, >> phys_cpu_present_map); >> >> + if (!apic->apic_id_valid(apicid)) { >> + disabled_cpus++; >> + return -EINVAL; >> + } >> + >> /* >> * boot_cpu_physical_apicid is designed to have the apicid >> * returned by read_apic_id(), i.e, the apicid of the >> >> > > No, That does not fix the issue. > > the system have x2apic pre_enabled from BIOS, so at the time > apic is set to &apic_x2apic_cluster. > > early_acpi_boot_init ==> early_acpi_process_madt ==> acpi_parse_madt > ==> default_acpi_madt_oem_check > > default_acpi_madt_oem_check > ==> apic_x2apic_cluster/x2apic_acpi_madt_oem_check ==> x2apic_enabled > ==> apic = &apic_x2apic_cluster > > and > static int x2apic_apic_id_valid(int apicid) > { > return 1; > } > > To make your change work, may need to update x2apic_apic_id_valid to > > static int x2apic_apic_id_valid(int apicid) > { > if (apicid == 0xff || apicid == -1) > return 0; > I seem to remember that in x2APIC Spec the x2APIC ID may be at 255 or greater. If we do that judgment, it may be affect x2APIC's work in some other places. I saw the MADT, the main reason may be that we define 0xff to acpi_id in LAPIC mode. As you said, it was like: [ 42.107902] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled) [ 42.120125] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled) [ 42.132361] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled) ... How about doing the acpi_id check when we parse it in acpi_parse_lapic(). 8<---------------- --- a/arch/x86/kernel/acpi/boot.c +++ b/arch/x86/kernel/acpi/boot.c @@ -233,6 +233,11 @@ acpi_parse_lapic(struct acpi_subtable_header * header, const unsigned long end) acpi_table_print_madt_entry(header); + if (processor->id >= 255) { + ++disabled_cpus; + return -EINVAL; + } + /* * We need to register disabled CPU as well to permit * counting disabled CPUs. This allows us to size Thanks Dou > return 1; > } > > > Thanks > > Yinghai > >