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

[Baoquan He]

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
> +	 * APIC
> +	 */
> +
> +	/* Has a separate chip ? */
> +	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?

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.

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
make it more specific and precise? Then people can get the exact
information from the comment and code.

Thanks
Baoquan

> +	/* Has a local APIC ? */
> +	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
> 
> 
>