linux-kernel - Re: [PATCH v5 01/12] x86/apic: Construct a selector for the interrupt delivery mode

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.20.1707021922000.2296@nanos>
Date:   Sun, 2 Jul 2017 19:37:10 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Dou Liyang <douly.fnst@...fujitsu.com>
cc:     x86@...nel.org, linux-kernel@...r.kernel.org,
        xen-devel@...ts.xenproject.org, mingo@...nel.org, hpa@...or.com,
        ebiederm@...ssion.com, bhe@...hat.com, boris.ostrovsky@...cle.com,
        peterz@...radead.org, izumi.taku@...fujitsu.com
Subject: Re: [PATCH v5 01/12] x86/apic: Construct a selector for the interrupt
 delivery mode

On Fri, 30 Jun 2017, Dou Liyang wrote:
> +static int __init apic_intr_mode_select(void)
> +{
> +	/* Check kernel option */
> +	if (disable_apic) {
> +		pr_info("APIC disabled via kernel command line\n");
> +		return APIC_PIC;
> +	}
> +
> +	/* Check BIOS */
> +#ifdef CONFIG_X86_64
> +	/* On 64-bit, the APIC must be integrated, Check local APIC only */
> +	if (!boot_cpu_has(X86_FEATURE_APIC)) {
> +		disable_apic = 1;
> +		pr_info("APIC disabled by BIOS\n");
> +		return APIC_PIC;
> +	}
> +#else
> +	/*
> +	 * On 32-bit, check whether there is a separate chip or integrated
> +	 * APIC
> +	 */
> +
> +	/* Has a local APIC ? */
> +	if (!boot_cpu_has(X86_FEATURE_APIC) &&
> +		APIC_INTEGRATED(boot_cpu_apic_version)) {

This looks wrong. The existing logic is:

	if (!boot_cpu_has(X86_FEATURE_APIC) && !smp_found_config)
		return -1;

	if (!boot_cpu_has(X86_FEATURE_APIC) &&
	                APIC_INTEGRATED(boot_cpu_apic_version)) {
		pr_err(....);

I know that this is magically the same because boot_cpu_apic_version is 0
in the !boot_cpu_has(X86_FEATURE_APIC) && !smp_found_config case, so you
don't fall into that conditional, but it's completely non obvious and does
not really make the code more understandable. Quite the contrary.

> +		disable_apic = 1;
> +		pr_err(FW_BUG "Local APIC %d not detected, force emulation\n",
> +				       boot_cpu_physical_apicid);
> +		return APIC_PIC;
> +	}
> +
> +	/* Has a separate chip ? */
> +	if (!boot_cpu_has(X86_FEATURE_APIC) && !smp_found_config) {
> +		disable_apic = 1;
> +
> +		return APIC_PIC;
> +	}

So if you move exactly that check above the other then it's clear what's
going on.

> +#endif
> +
> +	/* Check MP table or ACPI MADT configuration */
> +	if (!smp_found_config) {
> +		disable_ioapic_support();
> +
> +		if (!acpi_lapic)
> +			pr_info("APIC: ACPI MADT or MP tables are not detected\n");
> +
> +		return APIC_VIRTUAL_WIRE;
> +	}
> +
> +	/* Other checks of APIC options will be done in each setup function */
> +

Please remove the extra new line. It's not helping readability.

> +	return APIC_SYMMETRIC_IO;
> +}

Thanks,

	tglx