To: Jeremy Fitzhardinge <jeremy@goop.org>
Cc: Ingo Molnar <mingo@redhat.com>, Thomas Gleixner <tglx@linutronix.de>,
       "H. Peter Anvin" <hpa@zytor.com>,
       the arch/x86 maintainers <x86@kernel.org>,
       Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
       Xen-devel <xen-devel@lists.xensource.com>
References: <4A329CF8.4050502@goop.org>
From: ebiederm@xmission.com (Eric W. Biederman)
Date: Mon, 15 Jun 2009 03:51:17 -0700
In-Reply-To: <4A329CF8.4050502@goop.org> (Jeremy Fitzhardinge's message of "Fri\, 12 Jun 2009 11\:22\:48 -0700")
Message-ID: <m18wjtbwii.fsf@fess.ebiederm.org>
User-Agent: Gnus/5.11 (Gnus v5.11) Emacs/22.2 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Subject: Re: [PATCH RFC] x86/acpi: don't ignore I/O APICs just because there's no local APIC
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3531
Lines: 109

Jeremy Fitzhardinge <jeremy@goop.org> writes:

> Parse the ACPI MADT for I/O APIC information, even if the cpu has no
> (apparent) local APIC (ie, the CPU's APIC feature flag is clear).
>
> In principle, the local APIC and the I/O APIC are distinct (but related)
> components, which can be independently present.
>
> In practice this can happen in a Xen system, where the hypervisor has
> full control over the local APICs, and delivers interrupts initiated by
> the I/O APICs via Xen's event channel mechanism.
>
> (This eliminates the need for any explicit if (xen...) tests in
> acpi/boot.c)
>
> Signed-off-by: Jeremy Fitzhardinge<jeremy.fitzhardinge@citrix.com>

To be very clear.  We have mechanism and policy mixed in the mptable
and related code today.  While we continue to have that mixed I think
even attempting to reuse it for Xen dom0 is a horrifically bad move.

Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>

> diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
> index 2410469..19d13e5 100644
> --- a/arch/x86/kernel/acpi/boot.c
> +++ b/arch/x86/kernel/acpi/boot.c
> @@ -193,9 +193,6 @@ static int __init acpi_parse_madt(struct acpi_table_header *table)
>  {
>  	struct acpi_table_madt *madt = NULL;
>
> -	if (!cpu_has_apic)
> -		return -EINVAL;
> -
>  	madt = (struct acpi_table_madt *)table;
>  	if (!madt) {
>  		printk(KERN_WARNING PREFIX "Unable to map MADT\n");
> @@ -1252,9 +1249,6 @@ static int __init acpi_parse_madt_ioapic_entries(void)
>  	if (acpi_disabled || acpi_noirq)
>  		return -ENODEV;
>
> -	if (!cpu_has_apic)
> -		return -ENODEV;
> -
>  	/*
>  	 * if "noapic" boot option, don't look for IO-APICs
>  	 */
> @@ -1357,6 +1351,16 @@ static void __init acpi_process_madt(void)
>  #ifdef CONFIG_X86_BIGSMP
>  			generic_bigsmp_probe();
>  #endif
> +		}
> +		if (error == -EINVAL) {
> +			/*
> +			 * The ACPI tables themselves were malformed.
> +			 * Dell Precision Workstation 410, 610 come here.
> +			 */
> +			printk(KERN_ERR PREFIX
> +			       "Invalid BIOS MADT, disabling ACPI\n");
> +			disable_acpi();
> +		} else {
>  			/*
>  			 * Parse MADT IO-APIC entries
>  			 */
> @@ -1370,14 +1374,6 @@ static void __init acpi_process_madt(void)
>  					apic->setup_apic_routing();
>  			}
>  		}
> -		if (error == -EINVAL) {
> -			/*
> -			 * Dell Precision Workstation 410, 610 come here.
> -			 */
> -			printk(KERN_ERR PREFIX
> -			       "Invalid BIOS MADT, disabling ACPI\n");
> -			disable_acpi();
> -		}
>  	} else {
>  		/*
>   		 * ACPI found no MADT, and so ACPI wants UP PIC mode.
> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
> index c6acce2..d5e3f03 100644
> --- a/arch/x86/kernel/apic/io_apic.c
> +++ b/arch/x86/kernel/apic/io_apic.c
> @@ -1807,6 +1807,10 @@ __apicdebuginit(void) print_all_local_APICs(void)
>  {
>  	int cpu;
>
> +	/* don't print out if apic is not there */
> +	if (!cpu_has_apic)
> +		return;
> +
>  	preempt_disable();
>  	for_each_online_cpu(cpu)
>  		smp_call_function_single(cpu, print_local_APIC, NULL, 1);
> @@ -1849,8 +1853,7 @@ __apicdebuginit(int) print_all_ICs(void)
>  {
>  	print_PIC();
>
> -	/* don't print out if apic is not there */
> -	if (!cpu_has_apic || disable_apic)
> +	if (disable_apic)
>  		return 0;
>
>  	print_all_local_APICs();
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/