public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH v2] OvmfPkg/PlatformBootManagerLib: fix PCI interrupt link (LNKx)
@ 2020-12-17 14:12 Borghorst, Hendrik
  2020-12-17 17:33 ` [edk2-devel] " Laszlo Ersek
  2020-12-18 12:53 ` Laszlo Ersek
  0 siblings, 2 replies; 3+ messages in thread
From: Borghorst, Hendrik @ 2020-12-17 14:12 UTC (permalink / raw)
  To: devel@edk2.groups.io; +Cc: Woodhouse, David

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 ()) {
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [edk2-devel] [PATCH v2] OvmfPkg/PlatformBootManagerLib: fix PCI interrupt link (LNKx)
  2020-12-17 14:12 [PATCH v2] OvmfPkg/PlatformBootManagerLib: fix PCI interrupt link (LNKx) Borghorst, Hendrik
@ 2020-12-17 17:33 ` Laszlo Ersek
  2020-12-18 12:53 ` Laszlo Ersek
  1 sibling, 0 replies; 3+ messages in thread
From: Laszlo Ersek @ 2020-12-17 17:33 UTC (permalink / raw)
  To: devel, hborghor; +Cc: Woodhouse, David

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>

Yes, David's R-b is now on the list:

https://edk2.groups.io/g/devel/message/68797

> ---
>  .../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)
>  //

You missed the comment update I requested for this part, namely:

//
// Table of (interrupt link) --> (actual interrupt number) mappings; for (a)
// programming the
//
//   (slot, function) --> (interrupt link) --> (actual interrupt number)
//
// values into PCI Interrupt Line registers, and for (b) programming the
//
//   (interrupt link) --> (actual interrupt number)
//
// mappings themselves into the routing (= link configuration) registers.
//

at:

https://edk2.groups.io/g/devel/message/68805


Also the subject isn't exactly what I asked for; but I guess I don't want to obsess about it.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>


Thanks
Laszlo


>  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 ()) {
> 


^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [edk2-devel] [PATCH v2] OvmfPkg/PlatformBootManagerLib: fix PCI interrupt link (LNKx)
  2020-12-17 14:12 [PATCH v2] OvmfPkg/PlatformBootManagerLib: fix PCI interrupt link (LNKx) Borghorst, Hendrik
  2020-12-17 17:33 ` [edk2-devel] " Laszlo Ersek
@ 2020-12-18 12:53 ` Laszlo Ersek
  1 sibling, 0 replies; 3+ messages in thread
From: Laszlo Ersek @ 2020-12-18 12:53 UTC (permalink / raw)
  To: devel, hborghor; +Cc: Woodhouse, David

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


^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-12-18 12:53 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-12-17 14:12 [PATCH v2] OvmfPkg/PlatformBootManagerLib: fix PCI interrupt link (LNKx) Borghorst, Hendrik
2020-12-17 17:33 ` [edk2-devel] " Laszlo Ersek
2020-12-18 12:53 ` Laszlo Ersek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox