DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com F2BAD4E4C8
Date: Tue, 12 Sep 2017 16:04:16 +0800
From: Baoquan He <bhe@redhat.com>
To: Dou Liyang <douly.fnst@cn.fujitsu.com>
Cc: x86@kernel.org, linux-kernel@vger.kernel.org, tglx@linutronix.de,
        mingo@kernel.org, hpa@zytor.com, rjw@rjwysocki.net, bp@alien8.de,
        indou.takao@jp.fujitsu.com, izumi.taku@jp.fujitsu.com
Subject: Re: [PATCH v8 01/13] x86/apic: Construct a selector for the
 interrupt delivery mode
Message-ID: <20170912080416.GE12824@x1>
References: <1503890438-27840-1-git-send-email-douly.fnst@cn.fujitsu.com>
 <1503890438-27840-2-git-send-email-douly.fnst@cn.fujitsu.com>
 <20170906101728.GM30906@x1>
 <9f9f7477-01e9-cd16-47a2-e7ce13789e50@cn.fujitsu.com>
 <20170907052259.GP30906@x1>
 <172fc486-f894-0daa-7fb8-29a384cb6ae0@cn.fujitsu.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <172fc486-f894-0daa-7fb8-29a384cb6ae0@cn.fujitsu.com>
User-Agent: Mutt/1.7.0 (2016-08-17)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4043
Lines: 116

Hi dou,

I tested your patchset, the result is positive. Kdump kernel functions
well.

About the sanity check in patch 1/13, I still have concerns.
On 09/12/17 at 09:20am, Dou Liyang wrote:
> Hi Baoquan,
> 
> At 09/07/2017 01:22 PM, Baoquan He wrote:
> > On 09/07/17 at 12:19pm, Dou Liyang wrote:
> > > Hi Baoquan
> > > 
> > > I am wordy one ah:
> > > our target is checking if BIOS supports APIC, no matter what
> > > type(separated/integrated) it is. if not, go to PIC mode.
> > > 
> > > Let‘s discuss the original logic and the smp_found_config,
> > > then take about your code.
> > > 
> > > The existing logic is:
> > > 
> > > 	if (!boot_cpu_has(X86_FEATURE_APIC) && !smp_found_config) ...(1)
> > > 		return -1;
> > > 
> > > 	if (!boot_cpu_has(X86_FEATURE_APIC) &&
> > > 	                APIC_INTEGRATED(boot_cpu_apic_version)) { ...(2)
> > > 		pr_err(....);
> > > 
> > > why smp_found_config has to be checked in (1)?
> > > 
> > > Because, In case of discrete (pretty old) apics we may not set
> > > X86_FEATURE_APIC bit in cpuid, with 82489DX we can't rely on apic
> > > feature bit retrieved via cpuid(boot_cpu_has(X86_FEATURE_APIC)).[1]
> > > So we assume that if SMP configuration is found from MP table
> > > (smp_found_config = 1) in above case, there maybe a separated
> > > chip in our pc.
> > > 
> > > After passing the check of (1), we in (2), check whether local APIC
> > > is detected or not, If we have a BIOS bug.
> > > 
> > > [1] Commit 8312136fa8b0("x86, apic: Fix missed handling of discrete apics")

> 
> I thought again and again, I would not change this check logic.

I remeber you said you have been working on this issue for more than
half year, and you must have read the code flow agin and again, and
again. I read it too again and again recently.

You have read the related code so many times, while when we talk about
it, you still need think about it again and again, so for other code
reviewers or people who just read code for learning knowledge in this
area in short time, do you think they will get what the
apic_intr_mode_select() is doing immediately? or need read and think
again and again, and again ....?

I think it makes sense to make the logic clearer. In fact the below
logic looks better to me.

        /* If APIC is not integrated, check if SMP configuration is
         * found from MP table. If not too, no 82489DX. switch to
         * PIC mode
         *
         * Else APIC is integrated, check if the BIOS allows local APIC
         *
         */
apic is not integrated into cpu, it includes two cases: apic is on
82489DX or no apic at all. whatever it is, it's not on cpu. In this two
cases, PIC mode is right choice.
	if (!lapic_is_integrated()) {
                if (!smp_found_config) {
                        disable_apic = 1;
                        return APIC_PIC;
                }
        } else if(!boot_cpu_has(X86_FEATURE_APIC)) {
                        disable_apic = 1;
                        pr_info("APIC disabled by BIOS\n");
                        return APIC_PIC;
                }
        }

>From this logic the code can explain what it is doing, even without the
need of your code comment. But with the old logic you stick to, I am not
optimistic it can be made clear by code comments.

Surely, this is decided by maintainer.
> 

> Because actually, we have three possibilities:
> 
>   1. ACPI on chip
>   2. 82489DX
>   3. no APIC
> 
> lapic_is_integrated() is used to check the APIC's type which is
> APIC on chip or 82489DX. It has a prerequisite, we should avoid
> the third possibility(no APIC) first, which is decided by
> boot_cpu_has(X86_FEATURE_APIC) and smp_found_config. So, the original
> logic:
> 
> if (!boot_cpu_has(X86_FEATURE_APIC) && !smp_found_config)
> 
> ...is not just for 82489DX, but also for no APIC.
> 
> It looks more correct and understandable than us.
> 
> I am sorry my comments were wrong, and misled us. I will modify it
> in my next version.
> 
> BTW, How about your test result, is this series OK?
> 
> Thanks,
> 	dou.
> 
>