DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com C561A8553C
Date: Wed, 13 Sep 2017 10:30:43 +0800
From: Baoquan He <bhe@redhat.com>
To: Dou Liyang <douly.fnst@cn.fujitsu.com>
Cc: x86@kernel.org, linux-kernel@vger.kernel.org, tglx@linutronix.de,
        mingo@kernel.org, hpa@zytor.com, rjw@rjwysocki.net, bp@alien8.de,
        indou.takao@jp.fujitsu.com, izumi.taku@jp.fujitsu.com
Subject: Re: [PATCH v8 01/13] x86/apic: Construct a selector for the
 interrupt delivery mode
Message-ID: <20170913023043.GG12824@x1>
References: <1503890438-27840-1-git-send-email-douly.fnst@cn.fujitsu.com>
 <1503890438-27840-2-git-send-email-douly.fnst@cn.fujitsu.com>
 <20170906101728.GM30906@x1>
 <9f9f7477-01e9-cd16-47a2-e7ce13789e50@cn.fujitsu.com>
 <20170907052259.GP30906@x1>
 <172fc486-f894-0daa-7fb8-29a384cb6ae0@cn.fujitsu.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <172fc486-f894-0daa-7fb8-29a384cb6ae0@cn.fujitsu.com>
User-Agent: Mutt/1.7.0 (2016-08-17)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1755
Lines: 57

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.
> 
>