[PATCH] Ovmf: Set matching PCI routing values in PIIX4

[PATCH] Ovmf: Set matching PCI routing values in PIIX4

[hborghor]

From 1d887726b2ff42ed2c90fcdaa8549c3ed8be4b7d Mon Sep 17 00:00:00 2001
From: Hendrik Borghorst <hborghor@amazon.de>
Date: Tue, 8 Dec 2020 14:36:48 +0100
Subject: [PATCH] Ovmf: Set matching PCI routing values in PIIX4

The OVMF package tries to mimic the PCI initialization of SeaBIOS.
Both set the PCI_INTERRUPT_LINE register according to the same
logic with rotation based on IRQs (10, 10, 11, 11). However, while
SeaBIOS applies these IRQs to the PCI interrupt routing register (0x60)
of the PIIX4, OVMF wrongly applies (11, 11, 10, 10) which breaks legacy
INTx routing.

Signed-off-by: Hendrik Borghorst <hborghor@amazon.de>
Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
---
 .../PlatformBootManagerLib/BdsPlatform.c      | 24 +++++++++----------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
index 3c55ec9bd9..b8c3f54be6 100644
--- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
+++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
@@ -1211,24 +1211,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

Re: [edk2-devel] [PATCH] Ovmf: Set matching PCI routing values in PIIX4

[Laszlo Ersek]

Hi Hendrik,

On 12/09/20 11:55, Borghorst, Hendrik via groups.io wrote:
> From 1d887726b2ff42ed2c90fcdaa8549c3ed8be4b7d Mon Sep 17 00:00:00 2001
> From: Hendrik Borghorst <hborghor@amazon.de>
> Date: Tue, 8 Dec 2020 14:36:48 +0100
> Subject: [PATCH] Ovmf: Set matching PCI routing values in PIIX4
> 
> The OVMF package tries to mimic the PCI initialization of SeaBIOS.
> Both set the PCI_INTERRUPT_LINE register according to the same
> logic with rotation based on IRQs (10, 10, 11, 11). However, while
> SeaBIOS applies these IRQs to the PCI interrupt routing register (0x60)
> of the PIIX4, OVMF wrongly applies (11, 11, 10, 10) which breaks legacy
> INTx routing.
> 
> Signed-off-by: Hendrik Borghorst <hborghor@amazon.de>
> Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>  .../PlatformBootManagerLib/BdsPlatform.c      | 24 +++++++++----------
>  1 file changed, 12 insertions(+), 12 deletions(-)

this patch seems malformed; I'll comment on the other posting of it instead.

Meanwhile, David, can you please send your R-b to the list (under that
thread)?

Thanks,
Laszlo

> 
> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> index 3c55ec9bd9..b8c3f54be6 100644
> --- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> +++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> @@ -1211,24 +1211,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 ()) {
> 

Re: [EXTERNAL] [edk2-devel] [PATCH] Ovmf: Set matching PCI routing values in PIIX4

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 920 bytes --]

On Mon, 2020-12-14 at 16:19 +0000, David Woodhouse wrote:
> On Mon, 2020-12-14 at 15:51 +0100, Laszlo Ersek wrote:
> > this patch seems malformed; I'll comment on the other posting of it instead.
> > 
> > Meanwhile, David, can you please send your R-b to the list (under that
> > thread)?
> 
> Hm, I appear not to have received any list messages since February.
> 
> If someone can bounce me the relevant message (ISTR there's no archive
> that gets the threading right) I'll happily reply to it but otherwise 
> 
> Reviewed-by: David Woodhouse <dwmw2@infradead.org>

And here it is *again* to the list, because the list rejected it last
time. Because my email was allegedly bouncing... so I click on the link
to 'unbounce' it and at no point does it actually show me a time/date
or an error message that I could actually track down to fix anything,
but at least it claims to have 'unbounced' it now.

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

Re: [edk2-devel] [PATCH] Ovmf: Set matching PCI routing values in PIIX4

[Laszlo Ersek]

On 12/08/20 15:51, Borghorst, Hendrik via groups.io wrote:
> The OVMF package tries to mimic the PCI initialization of SeaBIOS.
> Both set the PCI_INTERRUPT_LINE register according to the same
> logic with rotation based on IRQs (10, 10, 11, 11). However, while
> SeaBIOS applies these IRQs to the PCI interrupt routing register (0x60)
> of the PIIX4, OVMF wrongly applies (11, 11, 10, 10) which breaks legacy
> INTx routing.
>
> Signed-off-by: Hendrik Borghorst <hborghor@amazon.de>
> Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>  .../PlatformBootManagerLib/BdsPlatform.c      | 24 +++++++++----------
>  1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> index 3c55ec9bd9..b8c3f54be6 100644
> --- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> +++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> @@ -1211,24 +1211,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 ()) {
>

The patch is correct, in my opinion; the commit message is lacking.

(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.


Please submit v2 of this patch, with the following updates:

- please extend the commit message -- feel free to include as much of
  the above discussion as you prefer (assuming you agree with it),

- please include the following snippet in the commit log,  taken before
  and after your patch from the Linux guest dmesg, for *both* i440fx
  *and* q35:

> [    0.247378] ACPI: PCI Interrupt Link [LNKA] (IRQs 5 10 *11)
> [    0.247505] ACPI: PCI Interrupt Link [LNKB] (IRQs 5 10 *11)
> [    0.247629] ACPI: PCI Interrupt Link [LNKC] (IRQs 5 *10 11)
> [    0.247747] ACPI: PCI Interrupt Link [LNKD] (IRQs 5 *10 11)
> [    0.247823] ACPI: PCI Interrupt Link [LNKS] (IRQs *9)
> [    0.292546] PCI Interrupt Link [LNKC] enabled at IRQ 10
> [    0.653097] PCI Interrupt Link [LNKD] enabled at IRQ 11
> [    0.667452] PCI Interrupt Link [LNKA] enabled at IRQ 11
> [    0.682064] PCI Interrupt Link [LNKB] enabled at IRQ 10

  (four snippets in total).

- please also reformat the PciHostIrqs array, near the top of the file,
  in the same patch. Currently it is:

> //
> // Table of host IRQs matching PCI IRQs A-D
> // (for configuring PCI Interrupt Line register)
> //
> CONST UINT8 PciHostIrqs[] = {
>   0x0a, 0x0a, 0x0b, 0x0b
> };

It should go like:

> //
> // 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.
> /
> CONST UINT8 PciHostIrqs[] = {
>   0x0a, // LNKA, LNKE
>   0x0a, // LNKB, LNKF
>   0x0b, // LNKC, LNKG
>   0x0b  // LNKD, LNKH
> };

Thanks,
Laszlo

Re: [edk2-devel] [PATCH] Ovmf: Set matching PCI routing values in PIIX4

[Laszlo Ersek]

On 12/14/20 19:37, Laszlo Ersek wrote:
> On 12/08/20 15:51, Borghorst, Hendrik via groups.io wrote:
>> The OVMF package tries to mimic the PCI initialization of SeaBIOS.
>> Both set the PCI_INTERRUPT_LINE register according to the same
>> logic with rotation based on IRQs (10, 10, 11, 11). However, while
>> SeaBIOS applies these IRQs to the PCI interrupt routing register (0x60)
>> of the PIIX4, OVMF wrongly applies (11, 11, 10, 10) which breaks legacy
>> INTx routing.
>>
>> Signed-off-by: Hendrik Borghorst <hborghor@amazon.de>
>> Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
>> ---
>>  .../PlatformBootManagerLib/BdsPlatform.c      | 24 +++++++++----------
>>  1 file changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
>> index 3c55ec9bd9..b8c3f54be6 100644
>> --- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
>> +++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
>> @@ -1211,24 +1211,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 ()) {
>>
> 
> The patch is correct, in my opinion; the commit message is lacking.
> 
> (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.
> 
> 
> Please submit v2 of this patch, with the following updates:
> 
> - please extend the commit message -- feel free to include as much of
>   the above discussion as you prefer (assuming you agree with it),
> 
> - please include the following snippet in the commit log,  taken before
>   and after your patch from the Linux guest dmesg, for *both* i440fx
>   *and* q35:
> 
>> [    0.247378] ACPI: PCI Interrupt Link [LNKA] (IRQs 5 10 *11)
>> [    0.247505] ACPI: PCI Interrupt Link [LNKB] (IRQs 5 10 *11)
>> [    0.247629] ACPI: PCI Interrupt Link [LNKC] (IRQs 5 *10 11)
>> [    0.247747] ACPI: PCI Interrupt Link [LNKD] (IRQs 5 *10 11)
>> [    0.247823] ACPI: PCI Interrupt Link [LNKS] (IRQs *9)
>> [    0.292546] PCI Interrupt Link [LNKC] enabled at IRQ 10
>> [    0.653097] PCI Interrupt Link [LNKD] enabled at IRQ 11
>> [    0.667452] PCI Interrupt Link [LNKA] enabled at IRQ 11
>> [    0.682064] PCI Interrupt Link [LNKB] enabled at IRQ 10
> 
>   (four snippets in total).
> 
> - please also reformat the PciHostIrqs array, near the top of the file,
>   in the same patch. Currently it is:
> 
>> //
>> // Table of host IRQs matching PCI IRQs A-D
>> // (for configuring PCI Interrupt Line register)
>> //
>> CONST UINT8 PciHostIrqs[] = {
>>   0x0a, 0x0a, 0x0b, 0x0b
>> };
> 
> It should go like:
> 
>> //
>> // 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.
>> /
>> CONST UINT8 PciHostIrqs[] = {
>>   0x0a, // LNKA, LNKE
>>   0x0a, // LNKB, LNKF
>>   0x0b, // LNKC, LNKG
>>   0x0b  // LNKD, LNKH
>> };

... please also refresh the subject line of the patch:

OvmfPkg/PlatformBootManagerLib: fix PCI interrupt link (LNKx) programming

(The board specs call these "Routing Control Register"s, but "routing"
is an obscure term, with the multiple levels of mappings that we have,
in my opinion.)

Thanks
Laszlo

Re: [EXTERNAL] [edk2-devel] [PATCH] Ovmf: Set matching PCI routing values in PIIX4

[Laszlo Ersek]

On 12/14/20 17:19, David Woodhouse wrote:
> On Mon, 2020-12-14 at 15:51 +0100, Laszlo Ersek wrote:
>> this patch seems malformed; I'll comment on the other posting of it instead.
>>
>> Meanwhile, David, can you please send your R-b to the list (under that
>> thread)?
> 
> Hm, I appear not to have received any list messages since February.
> 
> If someone can bounce me the relevant message (ISTR there's no archive
> that gets the threading right) I'll happily reply to it but otherwise 
> 
> Reviewed-by: David Woodhouse <dwmw2@infradead.org>
> 

Mail-archive.com used to be a good secondary archive, with threading and
message-id-based search; but it stopped getting a feed for some reason,
and I failed to rectify it (I reached out to multiple people and nobody
responded).

I also run a secondary archive at

https://www.redhat.com/archives/edk2-devel-archive/

but it doesn't go back beyond "2019-April", plus it doesn't have
msgid-based search. But, at least, it's mailman2 :)

Regarding the R-b, thanks for that; I've just asked for a v2 -- the code
is correct IMO, but I'd like to receive a more extensive commit message,
plus some extra comments in the code.

Thanks!
Laszlo

Re: [EXTERNAL] [edk2-devel] [PATCH] Ovmf: Set matching PCI routing values in PIIX4

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1161 bytes --]

On Mon, 2020-12-14 at 19:58 +0100, Laszlo Ersek wrote:
> Mail-archive.com used to be a good secondary archive, with threading and
> message-id-based search; but it stopped getting a feed for some reason,
> and I failed to rectify it (I reached out to multiple people and nobody
> responded).
> 
> I also run a secondary archive at
> 
> https://www.redhat.com/archives/edk2-devel-archive/
> 
> but it doesn't go back beyond "2019-April", plus it doesn't have
> msgid-based search. But, at least, it's mailman2 :)

Some years ago I fixed Mailman's web archives to contain mailto: links
with a proper &in-reply-to= in the URI, such that using it to reply got
the threading right.

Your archive doesn't seem to have the links at all though.

I seem to recall this is only necessary because groups.io does
something weird and *mangles* the Message-ID so that the one we receive
directly is different and all the threading is screwed anyway?

> Regarding the R-b, thanks for that; I've just asked for a v2 -- the code
> is correct IMO, but I'd like to receive a more extensive commit message,
> plus some extra comments in the code.

Ack.

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

Re: [EXTERNAL] [edk2-devel] [PATCH] Ovmf: Set matching PCI routing values in PIIX4

[Laszlo Ersek]

On 12/15/20 00:48, David Woodhouse wrote:
> On Mon, 2020-12-14 at 19:58 +0100, Laszlo Ersek wrote:
>> Mail-archive.com used to be a good secondary archive, with threading and
>> message-id-based search; but it stopped getting a feed for some reason,
>> and I failed to rectify it (I reached out to multiple people and nobody
>> responded).
>>
>> I also run a secondary archive at
>>
>> https://www.redhat.com/archives/edk2-devel-archive/
>>
>> but it doesn't go back beyond "2019-April", plus it doesn't have
>> msgid-based search. But, at least, it's mailman2 :)
> 
> Some years ago I fixed Mailman's web archives to contain mailto: links
> with a proper &in-reply-to= in the URI, such that using it to reply got
> the threading right.
> 
> Your archive doesn't seem to have the links at all though.

TBH I'm happy this facility still exists at RH at all (i.e. that we can
request and run our own mailing lists) :)

> I seem to recall this is only necessary because groups.io does
> something weird and *mangles* the Message-ID so that the one we receive
> directly is different and all the threading is screwed anyway?

No, the threading is generally OK.

Groups.io has a profile-level option that basically means

  I have a gmail.com account, which forcefully de-duplicates messages,
  but I really want two separate copies of what *I* send to the list, so
  please falsify the message-id's on my messages delivered back to me.

The option is not called exactly that, of course. So yes, it can break
threading for the affected person, but it's not the general case.

So anyway, I run this secondary archive because the 01.org admins had
killed the original list archive that used to live their, despite their
express promise. The archive was deleted in two stages actually; first
they just replaced Mailman2 with Mailman3, which broke all the links of
course, but then they removed the edk2-devel archive altogether.

> 
>> Regarding the R-b, thanks for that; I've just asked for a v2 -- the code
>> is correct IMO, but I'd like to receive a more extensive commit message,
>> plus some extra comments in the code.
> 
> Ack.
> 

Thanks!
Laszlo

Re: [edk2-devel] [PATCH] Ovmf: Set matching PCI routing values in PIIX4

[Borghorst, Hendrik]

On Mon, 2020-12-14 at 19:41 +0100, Laszlo Ersek wrote:
> CAUTION: This email originated from outside of the organization. Do
> not click links or open attachments unless you can confirm the sender
> and know the content is safe.
> 
> 
> 
> On 12/14/20 19:37, Laszlo Ersek wrote:
> > On 12/08/20 15:51, Borghorst, Hendrik via groups.io wrote:
> > > The OVMF package tries to mimic the PCI initialization of
> > > SeaBIOS.
> > > Both set the PCI_INTERRUPT_LINE register according to the same
> > > logic with rotation based on IRQs (10, 10, 11, 11). However,
> > > while
> > > SeaBIOS applies these IRQs to the PCI interrupt routing register
> > > (0x60)
> > > of the PIIX4, OVMF wrongly applies (11, 11, 10, 10) which breaks
> > > legacy
> > > INTx routing.
> > > 
> > > Signed-off-by: Hendrik Borghorst <hborghor@amazon.de>
> > > Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
> > > ---
> > >  .../PlatformBootManagerLib/BdsPlatform.c      | 24 +++++++++--
> > > --------
> > >  1 file changed, 12 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> > > b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> > > index 3c55ec9bd9..b8c3f54be6 100644
> > > --- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> > > +++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> > > @@ -1211,24 +1211,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 ()) {
> > > 
> > 
> > The patch is correct, in my opinion; the commit message is lacking.
> > 
> > (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.
> > 
> > 
> > Please submit v2 of this patch, with the following updates:
> > 
> > - please extend the commit message -- feel free to include as much
> > of
> >   the above discussion as you prefer (assuming you agree with it),
> > 
> > - please include the following snippet in the commit log,  taken
> > before
> >   and after your patch from the Linux guest dmesg, for *both*
> > i440fx
> >   *and* q35:
> > 
> > > [    0.247378] ACPI: PCI Interrupt Link [LNKA] (IRQs 5 10 *11)
> > > [    0.247505] ACPI: PCI Interrupt Link [LNKB] (IRQs 5 10 *11)
> > > [    0.247629] ACPI: PCI Interrupt Link [LNKC] (IRQs 5 *10 11)
> > > [    0.247747] ACPI: PCI Interrupt Link [LNKD] (IRQs 5 *10 11)
> > > [    0.247823] ACPI: PCI Interrupt Link [LNKS] (IRQs *9)
> > > [    0.292546] PCI Interrupt Link [LNKC] enabled at IRQ 10
> > > [    0.653097] PCI Interrupt Link [LNKD] enabled at IRQ 11
> > > [    0.667452] PCI Interrupt Link [LNKA] enabled at IRQ 11
> > > [    0.682064] PCI Interrupt Link [LNKB] enabled at IRQ 10
> > 
> >   (four snippets in total).
> > 
> > - please also reformat the PciHostIrqs array, near the top of the
> > file,
> >   in the same patch. Currently it is:
> > 
> > > //
> > > // Table of host IRQs matching PCI IRQs A-D
> > > // (for configuring PCI Interrupt Line register)
> > > //
> > > CONST UINT8 PciHostIrqs[] = {
> > >   0x0a, 0x0a, 0x0b, 0x0b
> > > };
> > 
> > It should go like:
> > 
> > > //
> > > // 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.
> > > /
> > > CONST UINT8 PciHostIrqs[] = {
> > >   0x0a, // LNKA, LNKE
> > >   0x0a, // LNKB, LNKF
> > >   0x0b, // LNKC, LNKG
> > >   0x0b  // LNKD, LNKH
> > > };
> 
> ... please also refresh the subject line of the patch:
> 
> OvmfPkg/PlatformBootManagerLib: fix PCI interrupt link (LNKx)
> programming
> 
> (The board specs call these "Routing Control Register"s, but
> "routing"
> is an obscure term, with the multiple levels of mappings that we
> have,
> in my opinion.)
> 
> Thanks
> Laszlo
> 

thanks for this really nice write up. I will include it in the next
commit message when adressing the requested changes.

Thanks

Hendrik