Re: [PATCH v8 01/13] x86/apic: Construct a selector for the interrupt delivery mode

[Dou Liyang]

Hi Baoquan,

Thanks for your reply!

My answer is in below.

At 09/06/2017 08:55 AM, Baoquan He wrote:
> Hi Liyang,
>
> On 08/28/17 at 11:20am, Dou Liyang wrote:
> > Now, there are many switches in kernel which are used to determine
> > the final interrupt delivery mode, as shown below:
> >
> > 1) kconfig:
> >    CONFIG_X86_64; CONFIG_X86_LOCAL_APIC; CONFIG_x86_IO_APIC
> > 2) kernel option: disable_apic; skip_ioapic_setup
> > 3) CPU Capability: boot_cpu_has(X86_FEATURE_APIC)
> > 4) MP table: smp_found_config
> > 5) ACPI: acpi_lapic; acpi_ioapic; nr_ioapic
> >
> > These switches are disordered and scattered and there are also some
> > dependencies with each other. These make the code difficult to
> > maintain and read.
> >
> > Construct a selector to unify them into a single function, then,
> > Use this selector to get an interrupt delivery mode directly.
> >
> > Signed-off-by: Dou Liyang <douly.fnst@xxxxxxxxxxxxxx>
> > ---
> >  arch/x86/kernel/apic/apic.c | 59 +++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 59 insertions(+)
> >
> > diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> > index 98b3dd8..01bde03 100644
> > --- a/arch/x86/kernel/apic/apic.c
> > +++ b/arch/x86/kernel/apic/apic.c
> > @@ -1235,6 +1235,65 @@ void __init sync_Arb_IDs(void)
> >  			APIC_INT_LEVELTRIG | APIC_DM_INIT);
> >  }
> >
> > +enum apic_intr_mode {
> > +	APIC_PIC,
> > +	APIC_VIRTUAL_WIRE,
> > +	APIC_SYMMETRIC_IO,
> > +};
> > +
> > +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) {
> > +		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