x86/of: Fix a bug in x86 arch OF support

+ [email protected]

Hi Sebastian,

All touched part in this patch series was added by you. Hoping for concerns/comments or best case ACK, reviewed-by tag from you. I have CCed all others who were involved with you when you added this changes.

Hi [email protected], Rob,

Since this appears to be entrirely in devicetree domain. Hoping for feedback, concerns, mistakes or improvement feedbacks from you.

Hi Andy,

Hoping for a reviewed-by tag from you once you think it is ready to go. Thanks.

Hi Thomas, Ingo, Boris, Andy, Dave & all [email protected] maintainers,

Resending below details hoping to figure out if the changes are logically aligned with your understanding & that it will not break any x86/arch concerns that you might have. Shooting on any mistakes welcomed :-).

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

v2:
- Address review concern from Andy - rename property name to make it a bit more positive & self explanatory.
- Found that the bindings document for these HW's (APIC) are a bit off/obsolete and still in text format. Created new YAML schemas one for each - lapic & ioapic. Updated these schemas with latest info and add in new optional property details in the updated schema for lapic. Delete/let go of the text binding doc.
- CC [email protected] as these changes appear to be mainly targeted for devicetree maintainers review & approval.
- Increase CCed list to include all possible people who touched and were involved this part of code/feature addition.

v1:
- Initial draft

Rahul Tanwar (2):
x86/of: Add support for boot time interrupt delivery mode
configuration
x86/of: Convert & update Intel's APIC related binding schemas

 .../intel,ce4100-ioapic.txt                   | 26 --------
 .../intel,ce4100-ioapic.yaml                  | 62 ++++++++++++++++++
 .../intel,ce4100-lapic.yaml                   | 63 +++++++++++++++++++
 arch/x86/kernel/devicetree.c                  |  9 ++-
 4 files changed, 133 insertions(+), 27 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/interrupt-controller/intel,ce4100-ioapic.txt
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/intel,ce4100-ioapic.yaml
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/intel,ce4100-lapic.yaml

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 & hardcoded to legacy PIC mode. This is a bug for platforms which are OF based but do not use 8259 compliant legacy PIC interrupt controller. Such platforms can not even boot because of this bug/hardcoding.

Fix this bug by adding support for configuration of init time interrupt delivery mode for x86 OF based systems by introducing a new optional boolean property 'intel,virtual-wire-mode' for interrupt-controller node of local APIC. This property emulates IMCRP Bit 7 of MP feature info byte 2 of MP floating pointer structure [1].

Defaults to legacy PIC mode if absent. Configures it to virtual wire compatibility mode if present.

[1] https://www.manualslib.com/manual/77733/Intel-Multiprocessor.html?page=40#manual

---
 arch/x86/kernel/devicetree.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

index 5cd51f25f446..2a8833f0f6ae 100644
@@ -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,virtual-wire-mode")) {
+		printk(KERN_NOTICE "Virtual Wire compatibility mode.\n");
+		pic_mode = 0;
+	} else {
+		printk(KERN_NOTICE "IMCR and PIC compatibility mode.\n");
+		pic_mode = 1;
+	}
+
 	register_lapic_address(lapic_addr);
 }
 

2.17.1

...

With Best Regards,
Andy Shevchenko

Rechecked. Link URL works for me. Am i missing any standard practice to add URL links in commit log ?

Reset of the file uses printk(KERN_xxx ""). In v1, i used pr_notice() but on reviewing again found it to be odd one out in the file. So switched to printk(KERN_xxx ""). I can revert back to using pr_notice() if you think that's a better fit. Thanks.

Regards,
Rahul

...

I don;t know why we should use antique style of printing APIs in new patches. Even if the old code uses that, you can create a followup that can do two things:
- uses pr_lvl() instead of printk(KERN_LVL)
- keeps string literals unbroken between the lines (if any
of such breakage exists)

With Best Regards,
Andy Shevchenko

Well noted about it. Will update. Thanks.

Regards,
Rahul

Intel's APIC family of interrupt controllers support local APIC (lapic) & I/O APIC (ioapic). Convert existing bindings for lapic & ioapic from text to YAML schema. Separate lapic & ioapic schemas.

Also, update more info and newly introduced optional property for lapic to choose legacy PIC or virtual wire compatibility interrupt delivery mode.

---
 .../intel,ce4100-ioapic.txt                   | 26 --------
 .../intel,ce4100-ioapic.yaml                  | 62 ++++++++++++++++++
 .../intel,ce4100-lapic.yaml                   | 63 +++++++++++++++++++
 3 files changed, 125 insertions(+), 26 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/interrupt-controller/intel,ce4100-ioapic.txt
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/intel,ce4100-ioapic.yaml
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/intel,ce4100-lapic.yaml

deleted file mode 100644
index 7d19f494f19a..000000000000
@@ -1,26 +0,0 @@
-Interrupt chips
----------------
-
-* Intel I/O Advanced Programmable Interrupt Controller (IO APIC)
-
-  Required properties:
-  --------------------
-     compatible = "intel,ce4100-ioapic";
-     #interrupt-cells = <2>;
-
-  Device's interrupt property:
-
-     interrupts = <P S>;
-
-  The first number (P) represents the interrupt pin which is wired to the
-  IO APIC. The second number (S) represents the sense of interrupt which
-  should be configured and can be one of:
-    0 - Edge Rising
-    1 - Level Low
-    2 - Level High
-    3 - Edge Falling
-
-* Local APIC
-  Required property:
-
-     compatible = "intel,ce4100-lapic";

2.17.1

Conversion should be split from a new property addition.

With Best Regards,
Andy Shevchenko

Do you mean, i first update older text file with new property addition and then later convert it into YAML i.e. for now i just update existing text file with new addition and later convert them to YAML schema ? Thanks.

Regards,
Rahul

Patch 1: Convert to YAML (no content changes except its format) Patch 2: Introducing a new property
Patch 3: Updating code in x86

First two must be send to the DT people and have their Acks/Rb after all.

With Best Regards,
Andy Shevchenko

Now they will not see the ongoing discussion. Better was to just Cc them and give a link to lore.kernel.org for the thread.

With Best Regards,
Andy Shevchenko

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/interrupt-controller/intel,ce4100-lapic.yaml: properties:compatible: [{'const': 'intel,ce4100-lapic'}] is not of type 'object', 'boolean'
from schema $id: http://json-schema.org/draft-07/schema#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/interrupt-controller/intel,ce4100-ioapic.yaml: properties:compatible: [{'const': 'intel,ce4100-ioapic'}] is not of type 'object', 'boolean'
from schema $id: http://json-schema.org/draft-07/schema#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/interrupt-controller/intel,ce4100-lapic.yaml: ignoring, error in schema: properties: compatible /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/interrupt-controller/intel,ce4100-ioapic.yaml: ignoring, error in schema: properties: compatible Documentation/devicetree/bindings/interrupt-controller/intel,ce4100-lapic.example.dtb:0:0: /example-0/interrupt-controller@fee00000: failed to match any schema with compatible: ['intel,ce4100-lapic'] Documentation/devicetree/bindings/interrupt-controller/intel,ce4100-ioapic.example.dtb:0:0: /example-0/interrupt-controller@fec00000: failed to match any schema with compatible: ['intel,ce4100-ioapic']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date:

pip3 install dtschema --upgrade

Please check and re-submit.