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

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: <20170913023043.GG12824@x1>
Date:   Wed, 13 Sep 2017 10:30:43 +0800
From:   Baoquan He <bhe@...hat.com>
To:     Dou Liyang <douly.fnst@...fujitsu.com>
Cc:     x86@...nel.org, linux-kernel@...r.kernel.org, tglx@...utronix.de,
        mingo@...nel.org, hpa@...or.com, rjw@...ysocki.net, bp@...en8.de,
        indou.takao@...fujitsu.com, izumi.taku@...fujitsu.com
Subject: Re: [PATCH v8 01/13] x86/apic: Construct a selector for the
 interrupt delivery mode

Hi dou,

On 09/12/17 at 09:20am, Dou Liyang wrote:
> I thought again and again, I would not change this check logic.
> 
> Because actually, we have three possibilities:
> 
>   1. ACPI on chip
>   2. 82489DX
>   3. no APIC
> 
> lapic_is_integrated() is used to check the APIC's type which is
> APIC on chip or 82489DX. It has a prerequisite, we should avoid
> the third possibility(no APIC) first, which is decided by
> boot_cpu_has(X86_FEATURE_APIC) and smp_found_config. So, the original
> logic:
> 
> if (!boot_cpu_has(X86_FEATURE_APIC) && !smp_found_config)

I won't insist that the logic need be changed. From the test result, the
patchset works very well with notsc specified. And the whole patchset
looks not risky. Maybe the patch putting acpi_early_init() earlier can
be posted independently and involve other ARCHes maintainer to review.

About the code logic, I think the confusion comes from the unclear
condition check. E.g the above case, you said it's used to check
discrete apic, in fact !boot_cpu_has(X86_FEATURE_APIC) could means 3
cases:
1) discrete apic
2) no apic
3) integrated apic but disabled by bios.

See, that's why it's confusing, the condition of judgement is not
adequate. I don't know why the code contributer wanted to check discrete
apic case with it.

Anyway, after discussion, it's clear to me now. And the code works well.
So it's up to you to change it or not. Except of this place, the whole
patchset looks good.

Thanks
Baoquan

> 
> ...is not just for 82489DX, but also for no APIC.
> 
> It looks more correct and understandable than us.
> 
> I am sorry my comments were wrong, and misled us. I will modify it
> in my next version.
> 
> BTW, How about your test result, is this series OK?
> 
> Thanks,
> 	dou.
> 
>