[edk2-devel] [PATCH] Ovmf: Set matching PCI routing values in PIIX4
Borghorst, Hendrik via groups.io
hborghor=amazon.de at groups.io
Thu Dec 17 14:02:22 UTC 2020
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 at amazon.de>
> > > Reviewed-by: David Woodhouse <dwmw at 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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#69131): https://edk2.groups.io/g/devel/message/69131
Mute This Topic: https://groups.io/mt/78850811/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list