linux-kernel - Re: [PATCH] x86/apic: Remove the extra judgement of skipped IO APIC setup

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]

Date:   Wed, 1 Mar 2017 10:04:55 +0100
From:   Ingo Molnar <mingo@...nel.org>
To:     Dou Liyang <douly.fnst@...fujitsu.com>
Cc:     tglx@...utronix.de, hpa@...or.com, bp@...en8.de,
        wanpeng.li@...mail.com, nicstange@...il.com, x86@...nel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] x86/apic: Remove the extra judgement of skipped IO APIC
 setup


* Dou Liyang <douly.fnst@...fujitsu.com> wrote:

> As the commit 2e63ad4bd5dd ("x86/apic: Do not init irq remapping
> if ioapic is disabled") added the judgement of skipped IO APIC
> setup at the beginning of enable_IR_x2apic(). It may be redundant
> that we check it again when we try to enable the interrupt mapping.
> 
> So, remove the one in try_to_enable_IR() and refine them for
> better readability.
> 
> Signed-off-by: Dou Liyang <douly.fnst@...fujitsu.com>
> ---
>  arch/x86/kernel/apic/apic.c | 17 ++++-------------
>  1 file changed, 4 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> index 8567c85..86e7bd8 100644
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -1610,24 +1610,15 @@ static inline void try_to_enable_x2apic(int remap_mode) { }
>  static inline void __x2apic_enable(void) { }
>  #endif /* !CONFIG_X86_X2APIC */
>  
> -static int __init try_to_enable_IR(void)
> -{
> -#ifdef CONFIG_X86_IO_APIC
> -	if (!x2apic_enabled() && skip_ioapic_setup) {
> -		pr_info("Not enabling interrupt remapping due to skipped IO-APIC setup\n");
> -		return -1;
> -	}
> -#endif
> -	return irq_remapping_enable();
> -}
> -
>  void __init enable_IR_x2apic(void)
>  {
>  	unsigned long flags;
>  	int ret, ir_stat;
>  
> -	if (skip_ioapic_setup)
> +	if (skip_ioapic_setup) {
> +		pr_info("Not init interrupt remapping due to skipped IO-APIC setup\n");

So you replaced a perfectly readable kernel message:

 -		pr_info("Not enabling interrupt remapping due to skipped IO-APIC setup\n");

... with an unreadable one:

 +		pr_info("Not init interrupt remapping due to skipped IO-APIC setup\n");

Why?

Also, the changelog is pretty much unreadable as well:

> As the commit 2e63ad4bd5dd ("x86/apic: Do not init irq remapping
> if ioapic is disabled") added the judgement of skipped IO APIC
> setup at the beginning of enable_IR_x2apic(). It may be redundant
> that we check it again when we try to enable the interrupt mapping.
> 
> So, remove the one in try_to_enable_IR() and refine them for
> better readability.

I edited it to:

   The following commit:

     2e63ad4bd5dd ("x86/apic: Do not init irq remapping if ioapic is disabled") 

   ... added a check for skipped IO-APIC setup to enable_IR_x2apic(), but this 
   check is also duplicated in try_to_enable_IR() - and it will never succeed in 
   calling irq_remapping_enable().

   Remove the whole irq_remapping_enable() complication: if the IO-APIC is 
   disabled we cannot enable IRQ remapping.

And I restored the original pr_info() message as well.

Thanks,

	Ingo