DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 87E034E4C6
Date: Wed, 6 Sep 2017 17:02:23 +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: <20170906090223.GL30906@x1>
References: <1503890438-27840-1-git-send-email-douly.fnst@cn.fujitsu.com>
 <1503890438-27840-2-git-send-email-douly.fnst@cn.fujitsu.com>
 <20170906005500.GE30906@x1>
 <411f2b15-9a56-0b2e-af1c-cc41e5767208@cn.fujitsu.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <411f2b15-9a56-0b2e-af1c-cc41e5767208@cn.fujitsu.com>
User-Agent: Mutt/1.7.0 (2016-08-17)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3971
Lines: 136

On 09/06/17 at 12:18pm, 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
> 
> Change the comment to:
> 
> On 32-bit, the APIC may be a separated chip(82489DX) or integrated chip.
> if BSP doesn't has APIC feature, we can sure there is no integrated
> chip, but can not be sure there is no independent chip. So check two
> situation when BSP doesn't has APIC feature.
> 
> > > +	 * APIC
> > > +	 */
> > > +
> > > +	/* Has a separate chip ? */
> 
> If there is also no SMP configuration, we can be sure there is no
> separated chip. Switch the interrupt delivery node to APIC_PIC directly.
> 
> > > +	if (!boot_cpu_has(X86_FEATURE_APIC) && !smp_found_config) {

Here, the most confusing thing to me is the '!smp_found_config'. Why
does 'smp_found_config' has anything with APIC being separate or
integrated?

>From code, 'smp_found_config = 1' when process ACPI MADT, or in
smp_scan_config(). Do you have any finding about the thing that if no
smp config apic must not exist?

Just for curiosity, I know this is copied from APIC_init_uniprocessor().
But I don't understand the logic clearly.

> > > +		disable_apic = 1;
> > > +
> > > +		return APIC_PIC;
> > > +	}
> > > +
> > Here you said several times you are checking if APIC is integrated, but
> > you always check boot_cpu_has(X86_FEATURE_APIC), and you also check
> > smp_found_config in above case. Can you make the comment match the code?
> > 
> 
> Yes.
> 
> > E.g if (!boot_cpu_has(X86_FEATURE_APIC)), cpu doesn't support lapic,
> > just return, you can remove the CONFIG_X86_64 check to make it a common
> > check. And we have lapic_is_integrated() to check if lapic is integrated.
> > 
> I am sorry my comment may confuse you. our target is checking if BIOS
> supports APIC, no matter what type(separated/integrated) it is.
> 
> The new logic 1) as you said may like :
> 
> 	if (!boot_cpu_has(X86_FEATURE_APIC))
> 		return ...
> 	if (lapic_is_integrated())
> 		return ...
> here we miss (!boot_cpu_has(X86_FEATURE_APIC) && smp_found_config) for
> a separated chip.
> 
> > Besides, we are saying lapic is integrated with ioapic in a single chip,
> > right? I found MP-Spec mention it. If yes, could you add more words to
> 
> Yes, 82489DX – it was a discrete chip that functioned both as local and
> I/O APIC
> 
> > make it more specific and precise? Then people can get the exact
> 
> Indeed, I will. Please see the modification of comments
> 
> > information from the comment and code.
> > 
> > Thanks
> > Baoquan
> > 
> > > +	/* Has a local APIC ? */
> 
> Sanity check if the BIOS pretends there is one local APIC.
> 
> 
> Thanks,
> 	dou.
> 
> > > +	if (!boot_cpu_has(X86_FEATURE_APIC) &&
> > > +		APIC_INTEGRATED(boot_cpu_apic_version)) {
> > > +		disable_apic = 1;
> > > +		pr_err(FW_BUG "Local APIC %d not detected, force emulation\n",
> > > +				       boot_cpu_physical_apicid);
> > > +
> > > +		return APIC_PIC;
> > > +	}
> > > +#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;
> > > +	}
> > > +
> > > +	return APIC_SYMMETRIC_IO;
> > > +}
> > > +
> > >  /*
> > >   * An initial setup of the virtual wire mode.
> > >   */
> > > --
> > > 2.5.5
> > > 
> > > 
> > > 
> > 
> > 
> > 
> 
>