| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20100603172126.GA5502@lenovo>
Date: Thu, 3 Jun 2010 21:21:26 +0400
From: Cyrill Gorcunov <gorcunov@...il.com>
To: Prarit Bhargava <prarit@...hat.com>
Cc: linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org,
mjg@...hat.com, x86@...nel.org,
Thomas Gleixner <tglx@...utronix.de>,
"H. Peter Anvin" <hpa@...or.com>, Yinghai Lu <yinghai@...nel.org>,
Suresh Siddha <suresh.b.siddha@...el.com>
Subject: Re: [PATCH]: x86: use acpi flags for apic mapping
On Wed, Jun 02, 2010 at 06:20:15PM -0400, Prarit Bhargava wrote:
>
>>>
>>> +#ifdef CONFIG_ACPI
>>> +enum apic_acpi_map_status apic_is_acpi_clustered_box(void)
>>> +{
>>
>> It's a bit strange that function is "is" prefixed and returns not true or false
>> but enum, perhaps we may name it apic_acpi_dst_model() or something like
>> that?
>>
>
> Sure, np -- new patch.
>
> P.
Hi Prarit,
just have reviewed it again and got some questions:
> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
> index 1fa03e0..6b63f95 100644
> --- a/arch/x86/include/asm/apic.h
> +++ b/arch/x86/include/asm/apic.h
> @@ -252,6 +252,14 @@ static inline int apic_is_clustered_box(void)
> }
> #endif
>
> +enum apic_acpi_map_status {
> + APIC_ACPI_BOTH,
> + APIC_ACPI_CLUSTER,
> + APIC_ACPI_PHYSICAL,
> + APIC_ACPI_NONE
> +};
> +extern enum apic_acpi_map_status apic_acpi_dst_model(void);
> +
> extern u8 setup_APIC_eilvt_mce(u8 vector, u8 msg_type, u8 mask);
> extern u8 setup_APIC_eilvt_ibs(u8 vector, u8 msg_type, u8 mask);
>
> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> index e5a4a1e..e94a189 100644
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -2189,6 +2189,30 @@ static const __cpuinitconst struct dmi_system_id multi_dmi_table[] = {
> {}
> };
>
> +#ifdef CONFIG_ACPI
> +enum apic_acpi_map_status apic_acpi_dst_model(void)
> +{
> + if (acpi_gbl_FADT.header.revision >= FADT2_REVISION_ID) {
> + if (acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL &&
> + acpi_gbl_FADT.flags & ACPI_FADT_APIC_CLUSTER) {
> + /*
> + * The rest of the code assumes physical flat
> + * in this case.
> + */
> + return APIC_ACPI_BOTH;
> + }
Havin both flags set in ACPI FADT make me worry -- I suspect this means
acpi is screwed (this is ok, who doubt :) but the problem is HOW should
we treat TSC instability in such case? The current code assumes (tsc.c)
that cluster mode has TSC unstable and if we had both bits set which
tsc mode we should choose? I suspect we have to assume that TSC unstable
then.
> +
> + if (acpi_gbl_FADT.flags & ACPI_FADT_APIC_CLUSTER)
> + return APIC_ACPI_CLUSTER;
> +
> + if (acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL)
> + return APIC_ACPI_PHYSICAL;
> + }
> +
> + return APIC_ACPI_NONE;
> +}
> +#endif
> +
> static void __cpuinit dmi_check_multi(void)
> {
> if (multi_checked)
> @@ -2208,6 +2232,20 @@ static void __cpuinit dmi_check_multi(void)
> */
> __cpuinit int apic_is_clustered_box(void)
> {
> +#ifdef CONFIG_ACPI
> + switch (apic_acpi_dst_model()) {
> + case APIC_ACPI_PHYSICAL:
> + case APIC_ACPI_BOTH: /* assume physical flat in this case */
> + return 0;
> + break;
> + case APIC_ACPI_CLUSTER:
> + return 1;
> + break;
> + default:
> + break;
> + }
> +#endif
> +
> dmi_check_multi();
> if (multi)
> return 1;
> diff --git a/arch/x86/kernel/apic/apic_flat_64.c b/arch/x86/kernel/apic/apic_flat_64.c
> index 09d3b17..c2318ac 100644
> --- a/arch/x86/kernel/apic/apic_flat_64.c
> +++ b/arch/x86/kernel/apic/apic_flat_64.c
> @@ -231,14 +231,32 @@ static int physflat_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
> {
> #ifdef CONFIG_ACPI
> /*
> - * Quirk: some x86_64 machines can only use physical APIC mode
> - * regardless of how many processors are present (x86_64 ES7000
> - * is an example).
> + * Some x86_64 machines can only use clustered or physical APIC
> + * mode regardless of how many processors are present.
> */
> - if (acpi_gbl_FADT.header.revision >= FADT2_REVISION_ID &&
> - (acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL)) {
> - printk(KERN_DEBUG "system APIC only can use physical flat");
> - return 1;
> + switch (apic_acpi_dst_model()) {
> + case APIC_ACPI_BOTH:
> + printk(KERN_WARNING FW_BUG "ACPI has set apic mode to "
> + "both clustered and physical flat. Please "
> + "contact your firmware vendor for an update.\n");
> + /*
> + * In this case assume physical flat as only a very
> + * limited number of systems use cluster
> + */
> + printk(KERN_DEBUG "system APIC using physical flat\n");
> + return 1;
> + break;
> + case APIC_ACPI_CLUSTER:
> + printk(KERN_DEBUG "system APIC can only use cluster\n");
> + return 0;
> + break;
> + case APIC_ACPI_PHYSICAL:
> + printk(KERN_DEBUG "system APIC can only use physical"
> + " flat\n");
> + return 1;
> + break;
> + default:
> + break;
> }
Not sure, but it seems this may broke IBM and EXA machines which should
use physical destination mode, hmm?
>
> if (!strncmp(oem_id, "IBM", 3) && !strncmp(oem_table_id, "EXA", 3)) {
Has this patch been tested on real hardware? Asking so since I don't
have neither IBM nor EXA machine.
I'm CC'ing experts I know were involved.
-- Cyrill
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists