linux-kernel - Re: [PATCH] Use x2apic_supported() in the default_apic_id

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4F627401.1010907@numascale.com>
Date:	Thu, 15 Mar 2012 23:58:09 +0100
From:	Steffen Persvold <sp@...ascale.com>
To:	Suresh Siddha <suresh.b.siddha@...el.com>
CC:	Yinghai Lu <yinghai@...nel.org>, Ingo Molnar <mingo@...hat.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	"H. Peter Anvin" <hpa@...or.com>,
	Daniel J Blueman <daniel@...ascale-asia.com>,
	linux-kernel@...r.kernel.org, x86@...nel.org
Subject: Re: [PATCH] Use x2apic_supported() in the default_apic_id_valid()
 function.

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@...ascale.com>
>>> 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@...ascale.com>
>>>> Reviewed-by: Daniel J Blueman<daniel@...ascale-asia.com>
>>>
>>> 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@...nel.org>
>>
>
> 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/