[lkml]   [2017]   [Sep]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [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
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.


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

 \ /
  Last update: 2017-09-13 04:31    [W:0.051 / U:1.584 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site