All of lore.kernel.org
 help / color / mirror / Atom feed
From: HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com>
To: hpa@linux.intel.com
Cc: jingbai.ma@hp.com, kexec@lists.infradead.org,
	linux-kernel@vger.kernel.org, bp@alien8.de,
	ebiederm@xmission.com, akpm@linux-foundation.org,
	fengguang.wu@intel.com, vgoyal@redhat.com
Subject: Re: [PATCH v5 1/3] x86, apic: add bsp_physical_apicid
Date: Thu, 14 Nov 2013 20:16:44 +0900	[thread overview]
Message-ID: <5284B11C.3020103@jp.fujitsu.com> (raw)
In-Reply-To: <20131112095153.4902.51290.stgit@localhost6.localdomain6>

Hello HPA,

I have another question relevant to http://lkml.org/lkml/2013/11/12/311.

(2013/11/12 18:51), HATAYAMA Daisuke wrote:
<cut>
> @@ -2589,3 +2593,14 @@ static int __init lapic_insert_resource(void)
>    * that is using request_resource
>    */
>   late_initcall(lapic_insert_resource);
> +
> +void __init bsp_physical_apicid_init(void)
> +{
> +	u32 l, h;
> +
> +	if (boot_cpu_data.x86 >= 6 && cpu_has_apic) {
> +		rdmsr(MSR_IA32_APICBASE, l, h);
> +		if (!(l & MSR_IA32_APICBASE_BSP))
> +			bsp_physical_apicid = boot_cpu_data.initial_apicid;
> +	}
> +}

So, rigrously, we should not use rdmsr for MSR_IA32_APICBASE here since this can
return wrong value on some clustered system?

At least all the mpparse.c, amdtopology.c and visws_quirks.c has referred to
MP table only, so there's no issue by simply dropping this code.

> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 2793d1f..28bea2c 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -681,7 +681,7 @@ static void identify_cpu_without_cpuid(struct cpuinfo_x86 *c)
>  /*
>   * Do minimum CPU detection early.
>   * Fields really needed: vendor, cpuid_level, family, model, mask,
> - * cache alignment.
> + * cache alignment, initial_apicid.
>   * The others are not touched to avoid unwanted side effects.
>   *
>   * WARNING: this function is only called on the BP.  Don't add code here
> @@ -725,6 +725,9 @@ static void __init early_identify_cpu(struct cpuinfo_x86 *c)
>  		this_cpu->c_bsp_init(c);
>
>  	setup_force_cpu_cap(X86_FEATURE_ALWAYS);
> +
> +	if (c->cpuid_level >= 0x00000001)
> +		c->initial_apicid = (cpuid_ebx(1) >> 24) & 0xFF;
>  }

Can the initial APID IDs obtained in this way using cpuid duplicate on some
clustered system?

Can we consider each apic->get_apic_id() method return *initial* APIC ID?
According to 8.4 Multiple-Processor (MP) Initialization, Intel System Programming
Guide, the APID IDs written into MADT and MP table are the initial ones.
So, APID IDs to be compared with these must be the initial ones.

-- 
Thanks.
HATAYAMA, Daisuke


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

WARNING: multiple messages have this Message-ID (diff)
From: HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com>
To: hpa@linux.intel.com
Cc: ebiederm@xmission.com, vgoyal@redhat.com,
	kexec@lists.infradead.org, linux-kernel@vger.kernel.org,
	bp@alien8.de, akpm@linux-foundation.org, fengguang.wu@intel.com,
	jingbai.ma@hp.com
Subject: Re: [PATCH v5 1/3] x86, apic: add bsp_physical_apicid
Date: Thu, 14 Nov 2013 20:16:44 +0900	[thread overview]
Message-ID: <5284B11C.3020103@jp.fujitsu.com> (raw)
In-Reply-To: <20131112095153.4902.51290.stgit@localhost6.localdomain6>

Hello HPA,

I have another question relevant to http://lkml.org/lkml/2013/11/12/311.

(2013/11/12 18:51), HATAYAMA Daisuke wrote:
<cut>
> @@ -2589,3 +2593,14 @@ static int __init lapic_insert_resource(void)
>    * that is using request_resource
>    */
>   late_initcall(lapic_insert_resource);
> +
> +void __init bsp_physical_apicid_init(void)
> +{
> +	u32 l, h;
> +
> +	if (boot_cpu_data.x86 >= 6 && cpu_has_apic) {
> +		rdmsr(MSR_IA32_APICBASE, l, h);
> +		if (!(l & MSR_IA32_APICBASE_BSP))
> +			bsp_physical_apicid = boot_cpu_data.initial_apicid;
> +	}
> +}

So, rigrously, we should not use rdmsr for MSR_IA32_APICBASE here since this can
return wrong value on some clustered system?

At least all the mpparse.c, amdtopology.c and visws_quirks.c has referred to
MP table only, so there's no issue by simply dropping this code.

> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 2793d1f..28bea2c 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -681,7 +681,7 @@ static void identify_cpu_without_cpuid(struct cpuinfo_x86 *c)
>  /*
>   * Do minimum CPU detection early.
>   * Fields really needed: vendor, cpuid_level, family, model, mask,
> - * cache alignment.
> + * cache alignment, initial_apicid.
>   * The others are not touched to avoid unwanted side effects.
>   *
>   * WARNING: this function is only called on the BP.  Don't add code here
> @@ -725,6 +725,9 @@ static void __init early_identify_cpu(struct cpuinfo_x86 *c)
>  		this_cpu->c_bsp_init(c);
>
>  	setup_force_cpu_cap(X86_FEATURE_ALWAYS);
> +
> +	if (c->cpuid_level >= 0x00000001)
> +		c->initial_apicid = (cpuid_ebx(1) >> 24) & 0xFF;
>  }

Can the initial APID IDs obtained in this way using cpuid duplicate on some
clustered system?

Can we consider each apic->get_apic_id() method return *initial* APIC ID?
According to 8.4 Multiple-Processor (MP) Initialization, Intel System Programming
Guide, the APID IDs written into MADT and MP table are the initial ones.
So, APID IDs to be compared with these must be the initial ones.

-- 
Thanks.
HATAYAMA, Daisuke


  reply	other threads:[~2013-11-14 11:17 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-11-12  9:51 [PATCH v5 0/3] x86, apic, kexec: Add disable_cpu_apic kernel parameter HATAYAMA Daisuke
2013-11-12  9:51 ` HATAYAMA Daisuke
2013-11-12  9:51 ` [PATCH v5 1/3] x86, apic: add bsp_physical_apicid HATAYAMA Daisuke
2013-11-12  9:51   ` HATAYAMA Daisuke
2013-11-14 11:16   ` HATAYAMA Daisuke [this message]
2013-11-14 11:16     ` HATAYAMA Daisuke
2013-11-12  9:51 ` [PATCH v5 2/3] x86, apic: Add disable_cpu_apicid kernel parameter HATAYAMA Daisuke
2013-11-12  9:51   ` HATAYAMA Daisuke
2013-11-12 10:44   ` Borislav Petkov
2013-11-12 10:44     ` Borislav Petkov
2013-11-14 10:42     ` HATAYAMA Daisuke
2013-11-14 10:42       ` HATAYAMA Daisuke
2013-11-12  9:52 ` [PATCH v5 3/3] Documentation, x86, apic, kexec: " HATAYAMA Daisuke
2013-11-12  9:52   ` HATAYAMA Daisuke

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5284B11C.3020103@jp.fujitsu.com \
    --to=d.hatayama@jp.fujitsu.com \
    --cc=akpm@linux-foundation.org \
    --cc=bp@alien8.de \
    --cc=ebiederm@xmission.com \
    --cc=fengguang.wu@intel.com \
    --cc=hpa@linux.intel.com \
    --cc=jingbai.ma@hp.com \
    --cc=kexec@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=vgoyal@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
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.