lkml.org 
[lkml]   [2022]   [Nov]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Subject[PATCH v3 0/4] x86/of: Fix a bug in x86 arch OF support
    Date
    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 devicetree@vger.kernel.org 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

    --
    2.17.1

    \
     
     \ /
      Last update: 2022-11-22 08:42    [W:2.327 / U:0.080 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site