[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: [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