Hi Sebastian Andrzej Siewior, Rob, Thomas, Ingo, Boris, Andy, Dave
& all [email protected] maintainers,
I sent this patch 3 times but for some reasons, never got any response or
attention from anybody so far. Hence, resending with a cover letter to
explain the rationale behind it in detail & to justify the need to add
this fix. Also, hoping to get some feedback in-case i am mistaken in my
understanding which might be the reason why i never got any response.
Background (baseline understanding - pls correct if mistaken anywhere):
References [1], [2] & [6]
For SMP systems, Intel defines three (logically four) interrupt modes
during boot/init time while BIOS/bootloader boots & switches to linux
kernel.
1. PIC mode - Legacy 8259 PIC interrupt controller.
2. Virtual wire mode via Local APIC - uses local APIC as virtual wire
3. Virtual wire mode via I/O APIC - uses I/O APIC as virtual wire
4. Symmetric I/O mode - final one used by linux for SMP systems.
BIOS/bootloaders are supposed to boot in either #1 or #2 or #3 and then
switch to #4 in linux for SMP systems.
For our platform, we use #2.
Detection of which interrupt mode the system is booting in is made by using
below global variable in apic.c
int pic_mode __ro_after_init;
Here pic_mode = 1 means #1 (PIC mode) above.
And pic_mode = 0 means #2 or #3 (basically virtual wire mode via apic).
And apic.c while doing setup_local_APIC() uses below code [3]:
value = apic_read(APIC_LVT0) & APIC_LVT_MASKED;
if (!cpu && (pic_mode || !value || skip_ioapic_setup)) {
value = APIC_DM_EXTINT;
apic_printk(APIC_VERBOSE, "enabled ExtINT on CPU#%d\n", cpu);
} else {
value = APIC_DM_EXTINT | APIC_LVT_MASKED;
apic_printk(APIC_VERBOSE, "masked ExtINT on CPU#%d\n", cpu);
}
apic_write(APIC_LVT0, value);
What i understand from above is that if at this point of time, as long as
it is cpu0 & pic_mode=1, it will set delivery mode to ExtINT (causes the
processor to respond to the interrupt as if the interrupt originated in an
externally connected (8259A-compatible) interrupt controller) and enables/
unmask the interrupts.
pic_mode is presently set/populated/initialized at only two places:
1. In mpparse.c [4]
2. In devicetree.c [7]
For #1 MPPARSE Kconfig definition is as below:
config X86_MPPARSE
bool "Enable MPS table" if ACPI
default y
depends on X86_LOCAL_APIC
help
For old smp systems that do not have proper acpi support. Newer systems
(esp with 64bit cpus) with acpi support, MADT and DSDT will override it
As seen above, if ACPI is not enabled, then mpparse by default is always
enabled. Presently, there is no way to disable MPPARSE (if ACPI is not
enabled). This to me appears to be another bug which needs fixing. As per
theory, MPPARSE was to support MPS spec [1] as a temporary solution to
support SMP systems until a final ACPI standard was added. But now if ACPI
is not enabled, it will rely on MPPARSE driver to read MP floating pointer
structure's IMCRP Bit 7 of MP feature info byte 2 [5] to figure out if it
supports PIC mode or virtual wire mode and initialize pic_mode variable
accordingly. If ACPI is enabled, the ACPI code overrides it by using the
MADT table spec'ed in ACPI spec [2].
For #2 devicetree.c presently hardcodes pic_mode = 1 (PIC Mode). There is
no support to configure virtual wire mode via devicetree path for OF based
systems.
Now we have a platform which is OF based & does not use legacy 8259 PIC
interrupt controller. Non ACPI compliant as well as non MPPARSE compliant.
For such platforms, it appears to me that hardcoding pic_mode = 1 (PIC Mode)
and giving no other choice to choose virtual wire mode is a bug for OF
based x86 platforms.
Just like mpparse relies on IMCRP bit 7 of MP feature info byte2 [5] to
select pic_mode to PIC mode or virtual wire mode. arch/x86/kernel/devicetree.c
should also provide some similar configurability to choose interrupt
delivery mode & not hardcode it to PIC mode.
This patch is to fix above explained bug in x86/of support for interrupt
delivery mode configuration. Please let me know if you find any mistake
in above understanding or if you have a alternative better suggestion to
solve it or if you find anything odd here in our platform/system. TIA.
The patch is baselined on below git tree:
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/core
[1] https://pdos.csail.mit.edu/6.828/2008/readings/ia32/MPspec.pdf
[2] https://uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf
[3] https://elixir.bootlin.com/linux/v6.1-rc5/source/arch/x86/kernel/apic/apic.c#L1691
[4] https://elixir.bootlin.com/linux/v6.1-rc5/source/arch/x86/kernel/mpparse.c#L517
[5] https://www.manualslib.com/manual/77733/Intel-Multiprocessor.html?page=40#manual
[6] https://www.intel.com/content/www/us/en/developer/articles/technical/intel-sdm.html
[7] https://elixir.bootlin.com/linux/v6.1-rc5/source/arch/x86/kernel/devicetree.c#L170
Rahul Tanwar (1):
x86/of: Add support for boot time interrupt mode config
arch/x86/kernel/devicetree.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
--
2.17.1
Presently, init/boot time interrupt delivery mode is enumerated only
for ACPI enabled systems by parsing MADT table or for older systems
by parsing MP table. But for OF based x86 systems, it is assumed &
fixed to legacy PIC mode.
Add support for configuration of init time interrupt delivery mode for
x86 OF based systems by introducing a new optional boolean property
'intel,no-imcr' for interrupt-controller node of local APIC. This
property emulates IMCRP Bit 7 of MP feature info byte 2 of MP
floating pointer structure.
Defaults to legacy PIC mode if absent. Configures it to virtual wire
compatibility mode if present.
Fixes: 3879a6f329483 ("x86: dtb: Add early parsing of IO_APIC")
Signed-off-by: Rahul Tanwar <[email protected]>
---
arch/x86/kernel/devicetree.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kernel/devicetree.c b/arch/x86/kernel/devicetree.c
index 5cd51f25f446..1e4ed420478b 100644
--- a/arch/x86/kernel/devicetree.c
+++ b/arch/x86/kernel/devicetree.c
@@ -167,7 +167,14 @@ static void __init dtb_lapic_setup(void)
return;
}
smp_found_config = 1;
- pic_mode = 1;
+ if (of_property_read_bool(dn, "intel,no-imcr")) {
+ pr_info(" Virtual Wire compatibility mode.\n");
+ pic_mode = 0;
+ } else {
+ pr_info(" IMCR and PIC compatibility mode.\n");
+ pic_mode = 1;
+ }
+
register_lapic_address(lapic_addr);
}
--
2.17.1
On Mon, Nov 14, 2022 at 05:20:06PM +0800, Rahul Tanwar wrote:
> Presently, init/boot time interrupt delivery mode is enumerated only
> for ACPI enabled systems by parsing MADT table or for older systems
> by parsing MP table. But for OF based x86 systems, it is assumed &
> fixed to legacy PIC mode.
>
> Add support for configuration of init time interrupt delivery mode for
> x86 OF based systems by introducing a new optional boolean property
> 'intel,no-imcr' for interrupt-controller node of local APIC. This
> property emulates IMCRP Bit 7 of MP feature info byte 2 of MP
> floating pointer structure.
>
> Defaults to legacy PIC mode if absent. Configures it to virtual wire
> compatibility mode if present.
...
> + if (of_property_read_bool(dn, "intel,no-imcr")) {
I can't find this property in the Documentation/devicetree/bindings.
Moreover, I prefer to see positive one, something like:
intel,virtual-wire-bla-bla-bla
Please consult with DT people on how properly name it.
> + pr_info(" Virtual Wire compatibility mode.\n");
> + pic_mode = 0;
> + } else {
> + pr_info(" IMCR and PIC compatibility mode.\n");
> + pic_mode = 1;
> + }
--
With Best Regards,
Andy Shevchenko
Hi Andy,
Thanks for response.
On 14/11/2022 5:45 pm, Andy Shevchenko wrote:
> This email was sent from outside of MaxLinear.
>
>
> On Mon, Nov 14, 2022 at 05:20:06PM +0800, Rahul Tanwar wrote:
>> Presently, init/boot time interrupt delivery mode is enumerated only
>> for ACPI enabled systems by parsing MADT table or for older systems
>> by parsing MP table. But for OF based x86 systems, it is assumed &
>> fixed to legacy PIC mode.
>>
>> Add support for configuration of init time interrupt delivery mode for
>> x86 OF based systems by introducing a new optional boolean property
>> 'intel,no-imcr' for interrupt-controller node of local APIC. This
>> property emulates IMCRP Bit 7 of MP feature info byte 2 of MP
>> floating pointer structure.
>>
>> Defaults to legacy PIC mode if absent. Configures it to virtual wire
>> compatibility mode if present.
>
> ...
>
>> + if (of_property_read_bool(dn, "intel,no-imcr")) {
>
> I can't find this property in the Documentation/devicetree/bindings.
>
> Moreover, I prefer to see positive one, something like:
>
> intel,virtual-wire-bla-bla-bla
>
> Please consult with DT people on how properly name it.
Yes, agree. Need to add it in bindings doc after finalizing the property
name. I chose "intel,no-imcr" to have a direct correlation with the MPS
spec defined data field for the same purpose. It reads below bit in
mpparse code to detect PIC mode or virtual wire mode.
Bit 7: IMCRP. When the IMCR presence bit is set, the IMCR is present and
PIC Mode is implemented; otherwise, Virtual Wire Mode is implemented.
Please refer [1]
[1] https://www.manualslib.com/manual/77733/Intel
Multiprocessor.html?page=40#manual
Regards,
Rahul
>
>> + pr_info(" Virtual Wire compatibility mode.\n");
>> + pic_mode = 0;
>> + } else {
>> + pr_info(" IMCR and PIC compatibility mode.\n");
>> + pic_mode = 1;
>> + }
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
>
On Mon, Nov 14, 2022 at 10:00:02AM +0000, Rahul Tanwar wrote:
> On 14/11/2022 5:45 pm, Andy Shevchenko wrote:
> > On Mon, Nov 14, 2022 at 05:20:06PM +0800, Rahul Tanwar wrote:
...
> >> + if (of_property_read_bool(dn, "intel,no-imcr")) {
> >
> > I can't find this property in the Documentation/devicetree/bindings.
> >
> > Moreover, I prefer to see positive one, something like:
> >
> > intel,virtual-wire-bla-bla-bla
> >
> > Please consult with DT people on how properly name it.
>
>
> Yes, agree. Need to add it in bindings doc after finalizing the property
> name. I chose "intel,no-imcr" to have a direct correlation with the MPS
> spec defined data field for the same purpose.
The problems with it are:
- it's negative
- it's too cryptic to one who doesn't know area well enough
> It reads below bit in
> mpparse code to detect PIC mode or virtual wire mode.
>
> Bit 7: IMCRP. When the IMCR presence bit is set, the IMCR is present and
> PIC Mode is implemented; otherwise, Virtual Wire Mode is implemented.
>
> Please refer [1]
>
> [1] https://www.manualslib.com/manual/77733/Intel
> Multiprocessor.html?page=40#manual
This is good reference for DT people to suggest you a better name.
--
With Best Regards,
Andy Shevchenko