Re: [PATCH] Use x2apic_supported() in the default_apic_id_valid()function.

[Steffen Persvold]

On 3/15/12 23:34 , Steffen Persvold wrote:
> On 3/15/12 22:21 , Suresh Siddha wrote:
> > On Thu, 2012-03-15 at 13:23 -0700, Yinghai Lu wrote:
> > > On Thu, Mar 15, 2012 at 11:03 AM, Steffen Persvold<sp@xxxxxxxxxxxxx>
> > > wrote:
> > > > Use x2apic_supported() in the default_apic_id_valid() function. If
> > > > x2apic mode is disabled (via nox2apic for example),
> > > > x2apic_supported() will return false.
> > > >
> > > > This allows us to substitute the check in
> > > > arch/x86/kernel/acpi/boot.c::acpi_parse_x2apic and avoid feigning
> > > > the x2apic cpu feature in the NumaChip apic code.
> > > >
> > > > Signed-off-by: Steffen Persvold<sp@xxxxxxxxxxxxx>
> > > > Reviewed-by: Daniel J Blueman<daniel@xxxxxxxxxxxxxxxxxx>
> > >
> > > I double checked on system with x2apic preenabled,
> > > nox2apic in boot command line still works well and it
> > > skips starting APs with apic id> 255.
> > >
> > > Acked-by: Yinghai Lu<yinghai@xxxxxxxxxx>
>
> Suresh,
>
> > This breaks the smpboot check if enabling interrupt-remapping/x2apic
> > fails on a platform. We will be in xapic mode and we don't clear the
> > x2apic cpufeature bit in this case and as such smpboot check will fail.
> >
> > So this change breaks the commit
> > c284b42abadbb22083bfde24d308899c08d44ffa.
>
> I was afraid of that.
>
> > I think the right thing is to have two different apid_id_valid checks
> > one for xapic driver (apic_flat_64.c) and another for x2apic driver
> > (x2apic_phys/cluster.c) and that way, x2apic MADT entries will be parsed
> > only if bios has handed over the OS in x2apic mode or if we have
> > selected the numachip model.
>
> Is my understanding of your suggestion correct that in
> x2apic_phys/cluster.c we add the following apic_id_valid() function :
>
> static int x2apic_apic_id_valid(int apicid)
> {
> return x2apic_mode || (apicid < 255);
> }
>
> ?
>
> Considering that this function (apic->apic_id_valid()) is called already
> in the acpi/boot.c::acpi_parse_x2apic() function is it sufficient enough
> to test for x2apic_mode ? Yinghai indicated that x2apic_mode was not set
> at this point, thus it was testing cpu_has_x2apic instead ?
>
> I must admit that I am not familiar enough with the APIC/ACPI code base
> to determine the sequence of events here (i.e MADT parsing, enabling of
> x2apic mode etc. etc.).

After reading the code a bit more it seems that the sequence is as follows :

kernel/setup.c::setup_arch() calls check_x2apic(). check_x2apic() first 
checks the cpu feature flag, then checks the MSR_IA32_APICBASE msr to 
see if bios has enabled x2apic mode. If this is the case, 
x2apic_preenabled and x2apic_mode is set to 1.

Later on in setup_arch(), the ACPI parsing starts.

My assumption is that the approach suggested in my previous email (based 
on Suresh' comment) with separate apic_id_valid() functions would be 
sufficient even for the MADT parsing ?

Kind regards,
Steffen
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/