Messages in this thread | | | Date | Thu, 7 Sep 2017 13:22:59 +0800 | From | Baoquan He <> | Subject | Re: [PATCH v8 01/13] x86/apic: Construct a selector for the interrupt delivery mode |
| |
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")
Hmm, sounds reasonable. Just a sentence to describe it could be better.
> > At 09/06/2017 06:17 PM, Baoquan He wrote: > > Hi Dou, > > > > On 08/28/17 at 11:20am, 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; > > > + } > > > + > > > > I am not very familiar with cpu registers, not sure if we can adjust > > below code flow as: > > > > /* If APIC is integrated, check local APIC only */ > > if (lapic_is_integrated() && !boot_cpu_has(X86_FEATURE_APIC)) { > > disable_apic = 1; > > pr_info("APIC disabled by BIOS\n"); > > return APIC_PIC; > > } > > > > /* If APIC is on a separate chip, check if smp_found_config is found*/ > > if (!lapic_is_integrated() && !smp_found_config) { > > disable_apic = 1; > > return APIC_PIC; > > } > > Yes, Awesome, we first consider it from APIC register space, then > the BOIS and software configration. let me do more investigation. > > I rewrite it based on you, any comments will welcome. > > /* 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 > * > */ > 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; > } > }
Yeah, it's fine to me. At least the logic looks more understandable.
> > BTW, As the macro APIC_INTEGRATED(x) has already wrapped by > CONFIG_X86_32, I will cleanup the lapic_is_integrated() for readablity > like that:
Yes, looks good. There's duplicate judgement of X86_64 in lapic_is_integrated.
> > diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c > index 7834f73..63b3ae9 100644 > --- a/arch/x86/kernel/apic/apic.c > +++ b/arch/x86/kernel/apic/apic.c > @@ -211,11 +211,7 @@ static inline int lapic_get_version(void) > */ > static inline int lapic_is_integrated(void) > { > -#ifdef CONFIG_X86_64 > - return 1; > -#else > return APIC_INTEGRATED(lapic_get_version()); > -#endif > } > > > Do you think so. ;-) > > > Thanks, > dou. > > > > ~~~~ Now, I haven't think of why smp_found_config has to be > > checked here. > > > > In this way, we don't need the CONFIG_X86_64 checking since it's > > contained in lapic_is_integrated() already. And the checking is obvious > > for understanding. Just not very sure if the checking is adequate. > > > > Just my personal opinion. > > > > > + /* 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; > > > + } > > > + > > > + /* 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 > > > > > > > > > > > > > > > > >
| |