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

PATCH Superseded Rahul Tanwar 3 participants 23 messages Nov 22, 2022 – Nov 24, 2022

Subsystems (3)

Device tree bindings x86 irqchip drivers

lore

Share & tools

lore

Rahul Tanwar

author

2022-11-22 07:40 UTC #0 lore raw

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

Hi All,

This patch series mainly fixes a boot time interrupt delivery mode configuration hardcoding bug for OF based x86 platforms. Presently, boot time interrupt delivery mode is hardcoded to legacy PIC mode with no option to configure it to virtual wire mode. This patch series aims to fix it by introducing a new optional boolean property for lapic devicetree node which can be used to configure it to virtual wire mode where applicable. Please find below detailed rationale behind it.

Rationale:

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. This causes kernel boot crash for platforms which does not support 8259 compatible external PIC.

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 (linux-v6.1.0-rc6):
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git

[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

v3:
- Address review concerns from Andy Shevchenko
* Reshuffle patch series changes to make it more logical.
* Patch 1 just converts existing intel,ce4100-ioapic.txt into YAML schema and separates out ioapic & lapic.
* Patch 2 adds new optional property for lapic.
* Patch 3 replaces older printk(KERN_LVL) to newer pr_lvl()
* Patch 4 adds code changes in devicetree.c to support newly added property.
- Fix 'make DT_CHECKER_FLAGS=-m dt_binding_check' errors reported by Rob Herring's bot.

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 (4):
x86/of: Convert Intel's APIC bindings to YAML schema x86/of: Introduce new optional bool property for lapic x86/of: Replace printk(KERN_LVL) with pr_lvl() x86/of: Add support for boot time interrupt delivery mode
configuration

 .../intel,ce4100-ioapic.txt                   | 26 --------
 .../intel,ce4100-ioapic.yaml                  | 62 ++++++++++++++++++
 .../intel,ce4100-lapic.yaml                   | 63 +++++++++++++++++++
 arch/x86/kernel/devicetree.c                  | 13 +++-
 4 files changed, 135 insertions(+), 29 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

Signature · 1 line

2.17.1

Rahul Tanwar

author

2022-11-22 07:40 UTC #1 lore raw

PATCH v3 1/4 x86/of: Convert Intel's APIC bindings to YAML schema

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. Addditionally, add description which was missing in text file and add few more required standard properties which were also missing in text file.

Signed-off-by Rahul Tanwar <[email protected]>

---
 .../intel,ce4100-ioapic.txt                   | 26 --------
 .../intel,ce4100-ioapic.yaml                  | 62 +++++++++++++++++++
 .../intel,ce4100-lapic.yaml                   | 49 +++++++++++++++
 3 files changed, 111 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

Documentation/devicetree/bindings/interrupt-controller/intel,ce4100-ioapic.txt deleted +0 ~~-26~~

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";

Documentation/devicetree/bindings/interrupt-controller/intel,ce4100-ioapic.yaml new file +62 -0

62 lines not on this page · raw message

Documentation/devicetree/bindings/interrupt-controller/intel,ce4100-lapic.yaml new file +49 -0

49 lines not on this page · raw message

Signature · 1 line

2.17.1

Andy Shevchenko

2022-11-22 09:10 UTC #2 lore raw

Re: [PATCH v3 1/4] x86/of: Convert Intel's APIC bindings to YAML schema

Quoted 6 lines · Rahul Tanwar

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. Addditionally, add description which was missing in text file and add few more required standard properties which were also missing in text file.

...

Documentation/devicetree/bindings/interrupt-controller/intel,ce4100-ioapic.yaml line 10

Quoted 10 lines

Documentation/devicetree/bindings/interrupt-controller/intel,ce4100-ioapic.yaml new file +9 -0

+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/interrupt-controller/intel,ce4100-ioapic.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Intel I/O Advanced Programmable Interrupt Controller (IO APIC)
 
+maintainers:
+  - Sebastian Andrzej Siewior <[email protected]>

I'm not sure, you need to have a confirmation before putting someone's name here. Yours is easier to add.

Documentation/devicetree/bindings/interrupt-controller/intel,ce4100-ioapic.yaml line 21

Quoted 10 lines

Documentation/devicetree/bindings/interrupt-controller/intel,ce4100-ioapic.yaml new file +9 -0

+description: |
+  Intel's Advanced Programmable Interrupt Controller (APIC) is a
+  family of interrupt controllers. The APIC is a split
+  architecture design, with a local component (LAPIC) integrated
+  into the processor itself and an external I/O APIC. Local APIC
+  (lapic) receives interrupts from the processor's interrupt pins,
+  from internal sources and from an external I/O APIC (ioapic).
+  And it sends these to the processor core for handling.
 
+  See https://pdos.csail.mit.edu/6.828/2008/readings/ia32/IA32-3A.pdf

Dunno if schema has special format for data sheet links...

Documentation/devicetree/bindings/interrupt-controller/intel,ce4100-ioapic.yaml line 26

Quoted 6 lines

Documentation/devicetree/bindings/interrupt-controller/intel,ce4100-ioapic.yaml new file +5 -0

+  Chapter 8 for more details.
+
+  Many of the Intel's generic devices like hpet, ioapic, lapic have
+  the ce4100 name in their compatible property names because they
 
+  first appeared in CE4100 SoC. See bindings/x86/ce4100.txt for more

Shouldn't you change this?

Quoted 3 lines

Documentation/devicetree/bindings/interrupt-controller/intel,ce4100-ioapic.yaml new file +3 -0
+  details on it.
+
+  This schema defines bindings for I/O APIC interrupt controller.

...

Quoted 11 lines

Documentation/devicetree/bindings/interrupt-controller/intel,ce4100-lapic.yaml new file +11 -0

+maintainers:
+  - Sebastian Andrzej Siewior <[email protected]>
+
+
+description: |
+  Intel's Advanced Programmable Interrupt Controller (APIC) is a
+  family of interrupt controllers. The APIC is a split
+  architecture design, with a local component (LAPIC) integrated
+  into the processor itself and an external I/O APIC. Local APIC
+  (lapic) receives interrupts from the processor's interrupt pins,
+  from internal sources and from an external I/O APIC (ioapic).

Documentation/devicetree/bindings/interrupt-controller/intel,ce4100-lapic.yaml line 29

Quoted 10 lines

Documentation/devicetree/bindings/interrupt-controller/intel,ce4100-lapic.yaml new file +10 -0

+  And it sends these to the processor core for handling.
+  See https://pdos.csail.mit.edu/6.828/2008/readings/ia32/IA32-3A.pdf
+  Chapter 8 for more details.
+
+  Many of the Intel's generic devices like hpet, ioapic, lapic have
+  the ce4100 name in their compatible property names because they
+  first appeared in CE4100 SoC. See bindings/x86/ce4100.txt for more
+  details on it.
+
+  This schema defines bindings for local APIC interrupt controller.

Same two comments as per above.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Rahul Tanwar

author

2022-11-22 09:44 UTC #3 lore raw

Re: [PATCH v3 1/4] x86/of: Convert Intel's APIC bindings to YAML schema

Quoted 22 lines · Andy Shevchenko

This email was sent from outside of MaxLinear.
Quoted 6 lines · Rahul Tanwar
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. Addditionally, add description which was missing in text file and add few more required standard properties which were also missing in text file.
...
Quoted 4 lines
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2
+---
+$id:
"http://devicetree.org/schemas/interrupt-controller/intel,ce4100-ioapic.yaml# <http://devicetree.org/schemas/interrupt-controller/intel,ce4100-ioapic.yaml#>"
Quoted 1 line
+$schema: "http://devicetree.org/meta-schemas/core.yaml#
<http://devicetree.org/meta-schemas/core.yaml#>"
Quoted 2 lines
Documentation/devicetree/bindings/interrupt-controller/intel,ce4100-ioapic.yaml new file +2 -0
+
+title: Intel I/O Advanced Programmable Interrupt Controller (IO APIC)

Quoted 7 lines

Quoted 2 lines
+maintainers:
+ - Sebastian Andrzej Siewior <[email protected]>
I'm not sure, you need to have a confirmation before putting someone's name here.
Yours is easier to add.

Well noted, will update.

Quoted 9 lines

Quoted 8 lines
+description: |
+ Intel's Advanced Programmable Interrupt Controller (APIC) is a
+ family of interrupt controllers. The APIC is a split
+ architecture design, with a local component (LAPIC) integrated
+ into the processor itself and an external I/O APIC. Local APIC
+ (lapic) receives interrupts from the processor's interrupt pins,
+ from internal sources and from an external I/O APIC (ioapic).
+ And it sends these to the processor core for handling.

Quoted 5 lines

Quoted 1 line
+ See https://pdos.csail.mit.edu/6.828/2008/readings/ia32/IA32-3A.pdf
<https://pdos.csail.mit.edu/6.828/2008/readings/ia32/IA32-3A.pdf>
Dunno if schema has special format for data sheet links...

Example-schema says this is the place to put URL's..

Quoted 9 lines

Quoted 6 lines
+ Chapter 8 for more details.
+
+ Many of the Intel's generic devices like hpet, ioapic, lapic have
+ the ce4100 name in their compatible property names because they
+ first appeared in CE4100 SoC. See bindings/x86/ce4100.txt for more
Shouldn't you change this?

Do you mean change compatibility property prefix from "intel,ce4100-ioapic" to "intel,ioapic"? If yes, then i totally agree and i will change it (including new file names & all other references to ce4100). If not, please clarify more..

Quoted 20 lines

Quoted 3 lines
+ details on it.
+
+ This schema defines bindings for I/O APIC interrupt controller.
...
Documentation/devicetree/bindings/interrupt-controller/intel,ce4100-ioapic.yaml line 21
Quoted 13 lines
Documentation/devicetree/bindings/interrupt-controller/intel,ce4100-ioapic.yaml new file +13 -0
+maintainers:
+ - Sebastian Andrzej Siewior <[email protected]>
+
+
+description: |
+ Intel's Advanced Programmable Interrupt Controller (APIC) is a
+ family of interrupt controllers. The APIC is a split
+ architecture design, with a local component (LAPIC) integrated
+ into the processor itself and an external I/O APIC. Local APIC
+ (lapic) receives interrupts from the processor's interrupt pins,
+ from internal sources and from an external I/O APIC (ioapic).
+ And it sends these to the processor core for handling.
+ See https://pdos.csail.mit.edu/6.828/2008/readings/ia32/IA32-3A.pdf 
<https://pdos.csail.mit.edu/6.828/2008/readings/ia32/IA32-3A.pdf>

Quoted 11 lines

Documentation/devicetree/bindings/interrupt-controller/intel,ce4100-lapic.yaml line 29
Quoted 8 lines
+ Chapter 8 for more details.
Documentation/devicetree/bindings/interrupt-controller/intel,ce4100-lapic.yaml new file +7 -0
+
+ Many of the Intel's generic devices like hpet, ioapic, lapic have
+ the ce4100 name in their compatible property names because they
+ first appeared in CE4100 SoC. See bindings/x86/ce4100.txt for more
+ details on it.
+
+ This schema defines bindings for local APIC interrupt controller.
Same two comments as per above.

Well noted.

Quoted 4 lines

Signature · 2 lines
With Best Regards,
Andy Shevchenko

Andy Shevchenko

2022-11-22 10:09 UTC #4 lore raw

Re: [PATCH v3 1/4] x86/of: Convert Intel's APIC bindings to YAML schema

Quoted 2 lines · Rahul Tanwar

Quoted 1 line · Andy Shevchenko
On Tue, Nov 22, 2022 at 03:39:07PM +0800, Rahul Tanwar wrote:

...

Quoted 8 lines

Quoted 3 lines
Quoted 1 line
+ first appeared in CE4100 SoC. See bindings/x86/ce4100.txt for more
Shouldn't you change this?
Do you mean change compatibility property prefix from "intel,ce4100-ioapic" to "intel,ioapic"? If yes, then i totally agree and i will change it (including new file names & all other references to ce4100). If not, please clarify more..

I specifically emphasized a single line (by putting blank lines around). For your convenience I removed the unneeded parts of the context, so you can see better what I meant.

...

Quoted 1 line

Quoted 1 line
Quoted 1 line
+ first appeared in CE4100 SoC. See bindings/x86/ce4100.txt for more

Ditto.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Rahul Tanwar

author

2022-11-22 10:44 UTC #5 lore raw

Re: [PATCH v3 1/4] x86/of: Convert Intel's APIC bindings to YAML schema

Quoted 13 lines · Andy Shevchenko

This email was sent from outside of MaxLinear.
Quoted 2 lines · Rahul Tanwar
Quoted 1 line · Andy Shevchenko
On Tue, Nov 22, 2022 at 03:39:07PM +0800, Rahul Tanwar wrote:
...
Quoted 4 lines
Quoted 3 lines
Quoted 1 line
+ first appeared in CE4100 SoC. See bindings/x86/ce4100.txt for more
Shouldn't you change this?

Quoted 9 lines

Quoted 4 lines
Do you mean change compatibility property prefix from "intel,ce4100-ioapic" to "intel,ioapic"? If yes, then i totally agree and i will change it (including new file names & all other references to ce4100). If not, please clarify more..
I specifically emphasized a single line (by putting blank lines around). For your convenience I removed the unneeded parts of the context, so you can see better what I meant.

Got it. I will remove the mention of "See bindings/x86/ce4100.txt" from here.

Quoted 13 lines

...
Quoted 1 line
Quoted 1 line
Quoted 1 line
+ first appeared in CE4100 SoC. See bindings/x86/ce4100.txt for more
Ditto.
--
With Best Regards,
Andy Shevchenko

Krzysztof Kozlowski

2022-11-23 16:03 UTC #6 lore raw

Re: [PATCH v3 1/4] x86/of: Convert Intel's APIC bindings to YAML schema

In reply to Rahul Tanwar

Quoted 9 lines · Rahul Tanwar

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. Addditionally, add description which was missing in text file and add few more required standard properties which were also missing in text file.
Suggested-by Andy Shevchenko <[email protected]>
Signed-off-by Rahul Tanwar <[email protected]>

Quoted 9 lines

---
 .../intel,ce4100-ioapic.txt                   | 26 --------
 .../intel,ce4100-ioapic.yaml                  | 62 +++++++++++++++++++
 .../intel,ce4100-lapic.yaml                   | 49 +++++++++++++++
 3 files changed, 111 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

Please use scripts/get_maintainers.pl to get a list of necessary people and lists to CC. It might happen, that command when run on an older kernel, gives you outdated entries. Therefore please be sure you base your patches on recent Linux kernel.

You miss not only people but also lists, meaning this will not be automatically tested.

So: No.

Best regards,
Krzysztof

Andy Shevchenko

2022-11-23 17:24 UTC #7 lore raw

Re: [PATCH v3 1/4] x86/of: Convert Intel's APIC bindings to YAML schema

Quoted 1 line · Krzysztof Kozlowski

On 22/11/2022 08:39, Rahul Tanwar wrote:

...

Quoted 7 lines

Please use scripts/get_maintainers.pl to get a list of necessary people and lists to CC. It might happen, that command when run on an older kernel, gives you outdated entries. Therefore please be sure you base your patches on recent Linux kernel.
You miss not only people but also lists, meaning this will not be automatically tested.

It seems that v4 manages to get the testing (and it's a good thing since it found some issues).

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Rahul Tanwar

author

2022-11-24 08:34 UTC #8 lore raw

Re: [PATCH v3 1/4] x86/of: Convert Intel's APIC bindings to YAML schema

In reply to Krzysztof Kozlowski

Quoted 28 lines · Krzysztof Kozlowski

This email was sent from outside of MaxLinear.
Quoted 9 lines · Rahul Tanwar
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. Addditionally, add description which was missing in text file and add few more required standard properties which were also missing in text file.
Suggested-by Andy Shevchenko <[email protected]>
Signed-off-by Rahul Tanwar <[email protected]>
Quoted 9 lines
---
  .../intel,ce4100-ioapic.txt                   | 26 --------
  .../intel,ce4100-ioapic.yaml                  | 62 +++++++++++++++++++
  .../intel,ce4100-lapic.yaml                   | 49 +++++++++++++++
  3 files changed, 111 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
Please use scripts/get_maintainers.pl to get a list of necessary people and lists to CC. It might happen, that command when run on an older kernel, gives you outdated entries. Therefore please be sure you base your patches on recent Linux kernel.

Quoted 5 lines

You miss not only people but also lists, meaning this will not be automatically tested.
So: No.

Agree that i made mistakes in email list earlier. But i fixed that problem from v4 onwards thanks to Andy. From v4 onwards, To & Cc should be correct. Thanks.

Regards,
Rahul

Quoted 4 lines

Best regards,
Krzysztof

Rahul Tanwar

author

2022-11-22 07:40 UTC #9 lore raw

PATCH v3 2/4 x86/of: Introduce new optional bool property for lapic

In reply to Rahul Tanwar

Intel defines a few possible interrupt delivery modes. With respect to boot/init time, mainly two interrupt delivery modes are possible. PIC Mode - Legacy external 8259 compliant PIC interrupt controller. Virtual Wire Mode - use lapic as virtual wire interrupt delivery mode.

For ACPI or MPS spec compliant systems, it is figured out by some read only bit field/s available in their respective defined data structures. But for OF based systems, it is by default set to PIC mode. Presently, it is hardcoded to legacy PIC mode for OF based x86 systems with no option to choose the configuration between PIC mode & virtual wire mode.

For this purpose, introduce a new boolean property for interrupt controller node of lapic which can allow it to be configured to virtual wire mode as well.

Property name: 'intel,virtual-wire-mode'
Type: Boolean

If not present/not defined, interrupt delivery mode defaults to legacy PIC mode. If present/defined, interrupt delivery mode is set to virtual wire mode.

Signed-off-by Rahul Tanwar <[email protected]>

---
 .../interrupt-controller/intel,ce4100-lapic.yaml   | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Documentation/devicetree/bindings/interrupt-controller/intel,ce4100-lapic.yaml +14 -0

index d4b99bf7bf6e..087f849e31ef 100644
@@ -35,6 +35,19 @@ properties:
   reg:
     maxItems: 1
 
+  intel,virtual-wire-mode:
+    description: Intel defines a few possible interrupt delivery
+      modes. With respect to boot/init time, mainly two interrupt
+      delivery modes are possible.
+      PIC Mode - Legacy external 8259 compliant PIC interrupt controller.
+      Virtual Wire Mode - use lapic as virtual wire interrupt delivery mode.
+      For ACPI or MPS spec compliant systems, it is figured out by some read
+      only bit field/s available in their respective defined data structures.
+      For OF based systems, it is by default set to PIC mode.
+      But if this optional boolean property is set, then the interrupt delivery
+      mode is configured to virtual wire compatibility mode.
+    type: boolean
+
 required:
   - compatible
   - reg

@@ -46,4 +59,5 @@ examples:
     lapic0: interrupt-controller@fee00000 {
         compatible = "intel,ce4100-lapic";
         reg = <0xfee00000 0x1000>;
+        intel,virtual-wire-mode;
     };

Signature · 1 line

2.17.1

Rahul Tanwar

author

2022-11-22 07:40 UTC #10 lore raw

PATCH v3 3/4 x86/of: Replace printk(KERN_LVL) with pr_lvl()

In reply to Rahul Tanwar

Use latest available pr_lvl() instead of older printk(KERN_LVL) Just a upgrade of print utilities usage no functional changes.

Signed-off-by Rahul Tanwar <[email protected]>

---
 arch/x86/kernel/devicetree.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/x86/kernel/devicetree.c +2 -2

index 5cd51f25f446..fcc6f1b7818f 100644
@@ -248,7 +248,7 @@ static void __init dtb_add_ioapic(struct device_node *dn)
 
 	ret = of_address_to_resource(dn, 0, &r);
 	if (ret) {
-		printk(KERN_ERR "Can't obtain address from device node %pOF.\n", dn);
+		pr_err("Can't obtain address from device node %pOF.\n", dn);
 		return;
 	}
 	mp_register_ioapic(++ioapic_id, r.start, gsi_top, &cfg);

@@ -265,7 +265,7 @@ static void __init dtb_ioapic_setup(void)
 		of_ioapic = 1;
 		return;
 	}
-	printk(KERN_ERR "Error: No information about IO-APIC in OF.\n");
+	pr_err("Error: No information about IO-APIC in OF.\n");
 }
 #else
 static void __init dtb_ioapic_setup(void) {}

Signature · 1 line

2.17.1

Andy Shevchenko

Reviewed-by on v3 3/4

2022-11-22 09:15 UTC #11 lore raw

Re: [PATCH v3 3/4] x86/of: Replace printk(KERN_LVL) with pr_lvl()

Quoted 2 lines · Rahul Tanwar

Use latest available pr_lvl() instead of older printk(KERN_LVL) Just a upgrade of print utilities usage no functional changes.

Reviewed-by Andy Shevchenko <[email protected]>

Quoted 31 lines

Signed-off-by Rahul Tanwar <[email protected]>

---
 arch/x86/kernel/devicetree.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/x86/kernel/devicetree.c +2 -2

index 5cd51f25f446..fcc6f1b7818f 100644
@@ -248,7 +248,7 @@ static void __init dtb_add_ioapic(struct device_node *dn)
 
 	ret = of_address_to_resource(dn, 0, &r);
 	if (ret) {
-		printk(KERN_ERR "Can't obtain address from device node %pOF.\n", dn);
+		pr_err("Can't obtain address from device node %pOF.\n", dn);
 		return;
 	}
 	mp_register_ioapic(++ioapic_id, r.start, gsi_top, &cfg);

@@ -265,7 +265,7 @@ static void __init dtb_ioapic_setup(void)
 		of_ioapic = 1;
 		return;
 	}
-	printk(KERN_ERR "Error: No information about IO-APIC in OF.\n");
+	pr_err("Error: No information about IO-APIC in OF.\n");
 }
 #else
 static void __init dtb_ioapic_setup(void) {}

Signature · 1 line

2.17.1

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Rahul Tanwar

author

2022-11-22 07:40 UTC #12 lore raw

PATCH v3 4/4 x86/of: Add support for boot time interrupt delivery mode configuration

In reply to Rahul Tanwar

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.

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

Fixes 3879a6f32948 ("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(-)

arch/x86/kernel/devicetree.c +8 -1

index fcc6f1b7818f..458e43490414 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")) {
+		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);
 }

Signature · 1 line

2.17.1

Andy Shevchenko

2022-11-22 09:14 UTC #13 lore raw

Re: [PATCH v3 4/4] x86/of: Add support for boot time interrupt delivery mode configuration

Quoted 15 lines · Rahul Tanwar

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.

Quoted 4 lines

Defaults to legacy PIC mode if absent. Configures it to virtual wire compatibility mode if present.
Fixes 3879a6f32948 ("x86: dtb: Add early parsing of IO_APIC")

If it was never working, there is nothing to fix. OTOH, without Cc: stable@ this is up to stable maintainers to backport.

Quoted 1 line

Suggested-by Andy Shevchenko <[email protected]>

I definitely haven't suggested this fix.

...

The code looks good to me.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Rahul Tanwar

author

2022-11-22 09:46 UTC #14 lore raw

Re: [PATCH v3 4/4] x86/of: Add support for boot time interrupt delivery mode configuration

Quoted 19 lines · Andy Shevchenko

This email was sent from outside of MaxLinear.
Quoted 15 lines · Rahul Tanwar
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.

Quoted 9 lines

Quoted 4 lines
Defaults to legacy PIC mode if absent. Configures it to virtual wire compatibility mode if present.
Fixes 3879a6f32948 ("x86: dtb: Add early parsing of IO_APIC")
If it was never working, there is nothing to fix. OTOH, without Cc: stable@ this is up to stable maintainers to backport.

Agree, will remove fixes tag..

Quoted 5 lines

Quoted 2 lines
Suggested-by Andy Shevchenko <[email protected]>
I definitely haven't suggested this fix.

Yes, i copy pasted to entire series. I will remove it from this patch.

Quoted 4 lines

...
The code looks good to me.

Thanks.

Regards,
Rahul

Quoted 6 lines

--
With Best Regards,
Andy Shevchenko

Andy Shevchenko

2022-11-22 10:10 UTC #15 lore raw

Re: [PATCH v3 4/4] x86/of: Add support for boot time interrupt delivery mode configuration

Quoted 2 lines · Rahul Tanwar

Quoted 1 line · Andy Shevchenko
On Tue, Nov 22, 2022 at 03:39:10PM +0800, Rahul Tanwar wrote:

...

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
Fixes 3879a6f32948 ("x86: dtb: Add early parsing of IO_APIC")
If it was never working, there is nothing to fix. OTOH, without Cc: stable@ this is up to stable maintainers to backport.
Agree, will remove fixes tag..

Don't forget to update cover letter and messages accordingly.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

2022-11-22 09:18 UTC #16 lore raw

In reply to Rahul Tanwar

Quoted 3 lines · Rahul Tanwar

Rahul Tanwar (4):
x86/of: Convert Intel's APIC bindings to YAML schema x86/of: Introduce new optional bool property for lapic

You need properly prefix the first two patches. I guess it's something like "dt-bindings: x86: ioapic:".

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Rahul Tanwar

author

2022-11-22 09:50 UTC #17 lore raw

Quoted 12 lines · Andy Shevchenko

This email was sent from outside of MaxLinear.
Quoted 3 lines · Rahul Tanwar
Rahul Tanwar (4):
x86/of: Convert Intel's APIC bindings to YAML schema x86/of: Introduce new optional bool property for lapic
You need properly prefix the first two patches. I guess it's something like "dt-bindings: x86: ioapic:".

Yes, i just checked the git log of devicetree.c and used same prefixes here. Thanks for correcting it. I will update it.

Regards,
Rahul

Quoted 6 lines

--
With Best Regards,
Andy Shevchenko

Andy Shevchenko

2022-11-22 10:15 UTC #18 lore raw

Quoted 7 lines · Rahul Tanwar

Quoted 6 lines · Andy Shevchenko
Quoted 3 lines · Rahul Tanwar
Rahul Tanwar (4):
x86/of: Convert Intel's APIC bindings to YAML schema x86/of: Introduce new optional bool property for lapic

Quoted 6 lines

Quoted 3 lines
You need properly prefix the first two patches. I guess it's something like "dt-bindings: x86: ioapic:".
Yes, i just checked the git log of devicetree.c and used same prefixes here. Thanks for correcting it. I will update it.

Moreover the Cc list in those patches is quite wrong.

I recommend to utilize my "smart" script [1] for sending series. (You may take an idea from it how to prepare the Cc list close to proper one)

[1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Rahul Tanwar

author

2022-11-22 10:49 UTC #19 lore raw

Quoted 20 lines · Andy Shevchenko

This email was sent from outside of MaxLinear.
Quoted 8 lines · Rahul Tanwar
Quoted 7 lines · Andy Shevchenko
Quoted 3 lines · Rahul Tanwar
Rahul Tanwar (4):
x86/of: Convert Intel's APIC bindings to YAML schema x86/of: Introduce new optional bool property for lapic
You need properly prefix the first two patches. I guess it's
something like
Quoted 5 lines
Quoted 2 lines
"dt-bindings: x86: ioapic:".
Yes, i just checked the git log of devicetree.c and used same prefixes here. Thanks for correcting it. I will update it.
Moreover the Cc list in those patches is quite wrong.

Quoted 8 lines

I recommend to utilize my "smart" script [1] for sending series. (You may take an idea from it how to prepare the Cc list close to proper one)
[1]:
https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh
<https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh>

Agree on the Cc list being wrong. Thanks for the script.

Quite useful. Could not understand how the script works for multiple commits when you take count & version as inputs. Also, where does it create patches (format-patch)? It seems to get suitable to & cc list & send emails to them. And the input is COMMIT_HASH. How do i use it for a series with multiple commits & do i have to create patches on my own? Thanks again.

Regards,
Rahul

Quoted 4 lines

Signature · 2 lines
With Best Regards,
Andy Shevchenko

Andy Shevchenko

2022-11-22 12:42 UTC #20 lore raw

Quoted 2 lines · Rahul Tanwar

Quoted 1 line · Andy Shevchenko
On Tue, Nov 22, 2022 at 09:49:04AM +0000, Rahul Tanwar wrote:

...

Quoted 11 lines

Quoted 6 lines
I recommend to utilize my "smart" script [1] for sending series. (You may take an idea from it how to prepare the Cc list close to proper one)
[1]:
https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh
Agree on the Cc list being wrong. Thanks for the script.
Quite useful. Could not understand how the script works for multiple commits when you take count & version as inputs.

It starts from the commit <COMMIT_HASH>~$count and goes up. So it's basically depth from the given commit (<COMMIT_HASH>).

Quoted 4 lines

Also, where does it
create patches (format-patch)? It seems to get suitable to & cc list & send emails to them. And the input is COMMIT_HASH. How do i use it for a series with multiple commits & do i have to create patches on my own?

It creates patches automatically in the same was as it's done by git format-patch. That's why it accepts a lot of the parameters which you usually add there.

Typical use for the series is

  ge2maintainer.sh -v 1 -c 4 HEAD~0 --annotate --cover-letter

Note, that your editor should be able to handle several files to edit (e.g. vim supports that mode).

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Rahul Tanwar

author

2022-11-23 09:59 UTC #21 lore raw

Quoted 36 lines · Andy Shevchenko

This email was sent from outside of MaxLinear.
Quoted 2 lines · Rahul Tanwar
Quoted 1 line · Andy Shevchenko
On Tue, Nov 22, 2022 at 09:49:04AM +0000, Rahul Tanwar wrote:
...
Quoted 2 lines
Quoted 2 lines
I recommend to utilize my "smart" script [1] for sending series. (You may take an idea from it how to prepare the Cc list close to
proper
Quoted 4 lines
Quoted 4 lines
one)
[1]:
https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh
<https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh>
Quoted 5 lines
Agree on the Cc list being wrong. Thanks for the script.
Quite useful. Could not understand how the script works for multiple commits when you take count & version as inputs.
It starts from the commit <COMMIT_HASH>~$count and goes up. So it's basically
depth from the given commit (<COMMIT_HASH>).
Quoted 4 lines
Also, where does it
create patches (format-patch)? It seems to get suitable to & cc list & send emails to them. And the input is COMMIT_HASH. How do i use it for a series with multiple commits & do i have to create patches on my own?
It creates patches automatically in the same was as it's done by git format-patch. That's why it accepts a lot of the parameters which you usually add there.

Quoted 7 lines

Typical use for the series is
ge2maintainer.sh -v 1 -c 4 HEAD~0 --annotate --cover-letter
Note, that your editor should be able to handle several files to edit (e.g. vim supports that mode).

The script worked like a charm. Very useful. Thanks. But i missed to CC you while using the script. I have addressed all your concerns and already sent v4. Request to please review the series starting from below link:
https://lore.kernel.org/all/[email protected]/

Regards,
Rahul

Quoted 4 lines

Signature · 2 lines
With Best Regards,
Andy Shevchenko

Andy Shevchenko

2022-11-23 13:53 UTC #22 lore raw

Quoted 4 lines · Rahul Tanwar

Quoted 3 lines · Andy Shevchenko
Quoted 2 lines · Rahul Tanwar
Quoted 1 line · Andy Shevchenko
On Tue, Nov 22, 2022 at 09:49:04AM +0000, Rahul Tanwar wrote:

...

Quoted 4 lines

Quoted 4 lines
It creates patches automatically in the same was as it's done by git format-patch. That's why it accepts a lot of the parameters which you usually add there.

Quoted 8 lines

Quoted 6 lines
Typical use for the series is
ge2maintainer.sh -v 1 -c 4 HEAD~0 --annotate --cover-letter
Note, that your editor should be able to handle several files to edit (e.g. vim supports that mode).
The script worked like a charm. Very useful. Thanks.

Thank you for trying it. I'm using it on a daily basis.

Quoted 5 lines

But i missed to CC
you while using the script. I have addressed all your concerns and already sent v4. Request to please review the series starting from below link:
https://lore.kernel.org/all/[email protected]/

Since we have lore.kernel.org and very powerful tool, called b4 (you may install it, if you have one of the most spread Linux distro, with your package manager) no need to resend. The Cc'ing on cover letter is enough, I can retrieve the whole thread from lore.

Signature · 2 lines

With Best Regards,
Andy Shevchenko