From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web12.12533.1608226439344897733 for ; Thu, 17 Dec 2020 09:33:59 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=BpdKC4Nj; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1608226438; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=aUm+GAeR6IfPs7GIYhlA8qsO4WtQBsiLMNJqaYnYP2A=; b=BpdKC4NjKQE9G8dY6y77ofpU3FNUvHzSoMM9sxpCln4/inUpwHweQ2SwK8uW96jRcakVLs 4amRrC2VHLqsWmx5mCcG8LyQ9LqlyEWcukDJy7uNSCEXSFwMrZhBQi/ktE3w3hxwojgu80 nVJ9oRfcOkqCJO9gmEYcl6B4L1ywTlc= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-313-CmUhBDJAOOSMkLzqrRHGIw-1; Thu, 17 Dec 2020 12:33:52 -0500 X-MC-Unique: CmUhBDJAOOSMkLzqrRHGIw-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 15B8C800D25; Thu, 17 Dec 2020 17:33:49 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-17.ams2.redhat.com [10.36.112.17]) by smtp.corp.redhat.com (Postfix) with ESMTP id EDE2169323; Thu, 17 Dec 2020 17:33:47 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2] OvmfPkg/PlatformBootManagerLib: fix PCI interrupt link (LNKx) To: devel@edk2.groups.io, hborghor@amazon.de Cc: "Woodhouse, David" References: <8dbedc4c7a1c3fd390aca915270814e3b35e13a5.camel@amazon.com> From: "Laszlo Ersek" Message-ID: <8245eb05-cd71-2c37-5ba4-09bd48e11197@redhat.com> Date: Thu, 17 Dec 2020 18:33:46 +0100 MIME-Version: 1.0 In-Reply-To: <8dbedc4c7a1c3fd390aca915270814e3b35e13a5.camel@amazon.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Reviewed-by: David Woodhouse 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 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 ()) { >