Re: [edk2-devel] [PATCH v2] OvmfPkg/PlatformBootManagerLib: fix PCI interrupt link (LNKx)

["Laszlo Ersek" <lersek@redhat.com>]

On 12/17/20 15:12, Borghorst, Hendrik via groups.io wrote:
> This patch fixes an issue with the current programming of the i440fx
> PCI Interrupt routing assignment.
> 
> Explanation by Laszlo Ersek:
> 
> (1) The rotating pattern is a map:
> 
>   (slot, function) --> (interrupt link) [LNKA..LNKD]
> 
> (more precisely, it is a pattern from (slot, pin) to (interrupt link),
> but function<->pin is an identity mapping in the QEMU hardware, so we
> can just use (slot, function) rather than (slot, pin) on the left hand
> side. But I digress.)
> 
> The ACPI _PRT object is generated by QEMU; it describes this map.
> 
> (2) Another map is
> 
>   (interrupt link) --> { set of possible interrupt numbers,
>                          for this link }
> 
> This map is given by the LNK[A..D] ACPI objects, also given by QEMU.
> 
> (3) What the firmware is expected to do is:
> 
> (3a) for each interrupt link, select an *actual* interrupt from the set
> that's possible for that link, yielding a deterministic map
> 
>   (interrupt link) --> (actual interrupt number)
> 
> and
> 
> (3b) for each PCI device/function with an interrupt pin, resolve the
> 
>   (slot, function) --> (interrupt link) --> (actual interrupt number)
> 
> functional composition, and program the result into the Interrupt Line
> register of the device.
> 
> In OVMF, we do not parse the rotating map described under (1) from
> QEMU's _PRT object. Instead, we duplicate the code. This is not a
> problem.
> 
> In OVMF, we also do not parse the map described under (2) from QEMU's
> ACPI content. Instead, we pick a specific selection (3a) that we
> "apriori" know satisfies (2). This is also not a problem. OVMF's
> particular selection is the PciHostIrqs table.
> 
> (
> 
> Table (2) from QEMU is
> 
>   LNKA -> { 5, 10, 11 }
>   LNKB -> { 5, 10, 11 }
>   LNKC -> { 5, 10, 11 }
>   LNKD -> { 5, 10, 11 }
> 
> and our specific pick in OVMF, in the PciHostIrqs table, is
> 
>   LNKA -> 10
>   LNKB -> 10
>   LNKC -> 11
>   LNKD -> 11
> 
> )
> 
> In OVMF, we also cover step (3b), in the SetPciIntLine() function.
> 
> What's missing in OVMF -- and what this patch corrects -- is that we
> currently fail to program our selection for table (3) into the hardware.
> We pick a specific LNKx->IRQ# mapping for each interrupt link, and we
> correctly program the PCI Interrupt Line registers through those
> link-to-IRQ mappings -- but we don't tell the hardware about the
> link-to-IRQ mappings. More precisely, we program such a link-to-IRQ
> mapping table into the hardware that is then not matched by the mapping
> we use for programming the PCI device/function interrupt lines. As a
> result, some PCI Interrupt Line registers will have impossible values --
> a given (slot, function) may use a particular link, but also report an
> interrupt number that was never picked for that link.
> 
> Output of Linux PCI Interrupt Links for i440fx before the patch:
> 
> [    0.327305] ACPI: PCI Interrupt Link [LNKA] (IRQs 5 10 *11)
> [    0.327944] ACPI: PCI Interrupt Link [LNKB] (IRQs 5 10 *11)
> [    0.328582] ACPI: PCI Interrupt Link [LNKC] (IRQs 5 *10 11)
> [    0.329208] ACPI: PCI Interrupt Link [LNKD] (IRQs 5 *10 11)
> [    0.329807] ACPI: PCI Interrupt Link [LNKS] (IRQs *9)
> 
> after the patch:
> 
> [    0.327292] ACPI: PCI Interrupt Link [LNKA] (IRQs 5 *10 11)
> [    0.327934] ACPI: PCI Interrupt Link [LNKB] (IRQs 5 *10 11)
> [    0.328564] ACPI: PCI Interrupt Link [LNKC] (IRQs 5 10 *11)
> [    0.329195] ACPI: PCI Interrupt Link [LNKD] (IRQs 5 10 *11)
> [    0.329785] ACPI: PCI Interrupt Link [LNKS] (IRQs *9)
> 
> Output of Linux PCI Interrupt Links for q35 before the patch:
> 
> [    0.307474] ACPI: PCI Interrupt Link [LNKA] (IRQs 5 *10 11)
> [    0.308027] ACPI: PCI Interrupt Link [LNKB] (IRQs 5 *10 11)
> [    0.308764] ACPI: PCI Interrupt Link [LNKC] (IRQs 5 10 *11)
> [    0.309310] ACPI: PCI Interrupt Link [LNKD] (IRQs 5 10 *11)
> [    0.309853] ACPI: PCI Interrupt Link [LNKE] (IRQs 5 *10 11)
> [    0.310508] ACPI: PCI Interrupt Link [LNKF] (IRQs 5 *10 11)
> [    0.311051] ACPI: PCI Interrupt Link [LNKG] (IRQs 5 10 *11)
> [    0.311589] ACPI: PCI Interrupt Link [LNKH] (IRQs 5 10 *11)
> 
> after the patch:
> 
> [    0.301991] ACPI: PCI Interrupt Link [LNKA] (IRQs 5 *10 11)
> [    0.302833] ACPI: PCI Interrupt Link [LNKB] (IRQs 5 *10 11)
> [    0.303354] ACPI: PCI Interrupt Link [LNKC] (IRQs 5 10 *11)
> [    0.303873] ACPI: PCI Interrupt Link [LNKD] (IRQs 5 10 *11)
> [    0.304399] ACPI: PCI Interrupt Link [LNKE] (IRQs 5 *10 11)
> [    0.304918] ACPI: PCI Interrupt Link [LNKF] (IRQs 5 *10 11)
> [    0.305436] ACPI: PCI Interrupt Link [LNKG] (IRQs 5 10 *11)
> [    0.305954] ACPI: PCI Interrupt Link [LNKH] (IRQs 5 10 *11)
> 
> Signed-off-by: Hendrik Borghorst <hborghor@amazon.de>
> Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>  .../PlatformBootManagerLib/BdsPlatform.c      | 29 ++++++++++---------
>  1 file changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> index 3c55ec9bd9..b0e9742937 100644
> --- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> +++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> @@ -29,7 +29,10 @@ UINT16        mHostBridgeDevId;
>  // (for configuring PCI Interrupt Line register)
>  //
>  CONST UINT8 PciHostIrqs[] = {
> -  0x0a, 0x0a, 0x0b, 0x0b
> +  0x0a, // LNKA, LNKE
> +  0x0a, // LNKB, LNKF
> +  0x0b, // LNKC, LNKG
> +  0x0b  // LNKD, LNKH
>  };
>  
>  //
> @@ -1211,24 +1214,24 @@ PciAcpiInitialization (
>        //
>        // 00:01.0 ISA Bridge (PIIX4) LNK routing targets
>        //
> -      PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x60), 0x0b); // A
> -      PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x61), 0x0b); // B
> -      PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x62), 0x0a); // C
> -      PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x63), 0x0a); // D
> +      PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x60), PciHostIrqs[0]); // A
> +      PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x61), PciHostIrqs[1]); // B
> +      PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x62), PciHostIrqs[2]); // C
> +      PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x63), PciHostIrqs[3]); // D
>        break;
>      case INTEL_Q35_MCH_DEVICE_ID:
>        Pmba = POWER_MGMT_REGISTER_Q35 (ICH9_PMBASE);
>        //
>        // 00:1f.0 LPC Bridge (Q35) LNK routing targets
>        //
> -      PciWrite8 (PCI_LIB_ADDRESS (0, 0x1f, 0, 0x60), 0x0a); // A
> -      PciWrite8 (PCI_LIB_ADDRESS (0, 0x1f, 0, 0x61), 0x0a); // B
> -      PciWrite8 (PCI_LIB_ADDRESS (0, 0x1f, 0, 0x62), 0x0b); // C
> -      PciWrite8 (PCI_LIB_ADDRESS (0, 0x1f, 0, 0x63), 0x0b); // D
> -      PciWrite8 (PCI_LIB_ADDRESS (0, 0x1f, 0, 0x68), 0x0a); // E
> -      PciWrite8 (PCI_LIB_ADDRESS (0, 0x1f, 0, 0x69), 0x0a); // F
> -      PciWrite8 (PCI_LIB_ADDRESS (0, 0x1f, 0, 0x6a), 0x0b); // G
> -      PciWrite8 (PCI_LIB_ADDRESS (0, 0x1f, 0, 0x6b), 0x0b); // H
> +      PciWrite8 (PCI_LIB_ADDRESS (0, 0x1f, 0, 0x60), PciHostIrqs[0]); // A
> +      PciWrite8 (PCI_LIB_ADDRESS (0, 0x1f, 0, 0x61), PciHostIrqs[1]); // B
> +      PciWrite8 (PCI_LIB_ADDRESS (0, 0x1f, 0, 0x62), PciHostIrqs[2]); // C
> +      PciWrite8 (PCI_LIB_ADDRESS (0, 0x1f, 0, 0x63), PciHostIrqs[3]); // D
> +      PciWrite8 (PCI_LIB_ADDRESS (0, 0x1f, 0, 0x68), PciHostIrqs[0]); // E
> +      PciWrite8 (PCI_LIB_ADDRESS (0, 0x1f, 0, 0x69), PciHostIrqs[1]); // F
> +      PciWrite8 (PCI_LIB_ADDRESS (0, 0x1f, 0, 0x6a), PciHostIrqs[2]); // G
> +      PciWrite8 (PCI_LIB_ADDRESS (0, 0x1f, 0, 0x6b), PciHostIrqs[3]); // H
>        break;
>      default:
>        if (XenDetected ()) {
> 

Merged as commit 6573ae8c8575, via
<https://github.com/tianocore/edk2/pull/1242>.

Thanks
Laszlo