From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56293) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z24Z8-0006oN-M3 for qemu-devel@nongnu.org; Mon, 08 Jun 2015 17:26:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z24Z5-00026S-9L for qemu-devel@nongnu.org; Mon, 08 Jun 2015 17:26:42 -0400 Received: from terminus.zytor.com ([2001:1868:205::10]:35136 helo=mail.zytor.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z24Z5-0001wq-08 for qemu-devel@nongnu.org; Mon, 08 Jun 2015 17:26:39 -0400 Date: Mon, 8 Jun 2015 18:26:01 -0300 From: Paulo Alcantara Message-ID: <20150608182601.6c24a4e0@zytor.com> In-Reply-To: <55755B20.9090104@redhat.com> References: <1433617803-18892-1-git-send-email-pcacjr@zytor.com> <55755B20.9090104@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] OvmfPkg/PlatformPei: Initialise RCBA (B0:D31:F0 0xf0) register List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laszlo Ersek Cc: pbonzini@redhat.com, edk2-devel@lists.sourceforge.net, Paulo Alcantara , qemu-devel@nongnu.org, mst@redhat.com On Mon, 08 Jun 2015 11:06:40 +0200 Laszlo Ersek wrote: > On 06/06/15 21:10, Paulo Alcantara wrote: > > This patch initialises root complex register block BAR in order to > > support TCO watchdog emulation features (e.g. reboot upon NO_REBOOT > > bit not set) on QEMU. > > > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Paulo Alcantara > > --- > > OvmfPkg/Include/IndustryStandard/Q35MchIch9.h | 6 ++++++ > > OvmfPkg/OvmfPkg.dec | 4 ++++ > > OvmfPkg/PlatformPei/Platform.c | 7 +++++++ > > OvmfPkg/PlatformPei/PlatformPei.inf | 1 + > > 4 files changed, 18 insertions(+) > > > > diff --git a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h > > b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h index > > 4f59a7c..4d42dfa 100644 --- > > a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h +++ > > b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h @@ -90,4 +90,10 @@ > > #define ICH9_SMI_EN_APMC_EN BIT5 > > #define ICH9_SMI_EN_GBL_SMI_EN BIT0 > > > > +// > > +// Root Complex Base Address register > > +// > > +#define ICH9_RCBA 0xf0 > > +#define ICH9_RCBA_EN BIT0 > > + > > The RCBA belongs to the same register block as PMBASE (I just checked > in the ich9 spec). Therefore, please move up this source code hunk, > to the end of the source code section that starts with: > > // > // B/D/F/Type: 0/0x1f/0/PCI > // > > As-is, the patch would add it to "IO ports relative to PMBASE", and > that's not a good place for it. > > In addition, the 0xf0 replacement text should line up with 0x40, 0x44, > 0xA0 visible in that part of the code. Then, the BIT0 replacement text > in the other macro should be indented two more positions relative to > that. (So that it lines up with BIT7, BIT4 etc. visible in that part > of the code.) OK. > > > #endif > > diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec > > index 4cb70dc..a6586f3 100644 > > --- a/OvmfPkg/OvmfPkg.dec > > +++ b/OvmfPkg/OvmfPkg.dec > > @@ -78,6 +78,10 @@ > > # to PIIX4 function 3 offset 0x40-0x43 bits [15:6]. > > gUefiOvmfPkgTokenSpaceGuid.PcdAcpiPmBaseAddress|0xB000|UINT16|5 > > > > + ## This flag determines the Root Complex Register Block BAR, > > written to Q35 > > + # function 31 offset 0xf0-0xf3 bits [31:14] > > + > > gUefiOvmfPkgTokenSpaceGuid.PcdRootComplexBaseAddress|0xfed1c000|UINT32|0x1e > > + > > I understand Jordan doesn't like the new PCD here, and proposes a > fixed macro for the same purpose, but I don't understand why we > should follow a different avenue for this base address when we opted > for a PCD with the PMBA. I will keep it as a macro since it won't be used in other places like PcdAcpiPmBaseAddress. > > > ## When VirtioScsiDxe is instantiated for a HBA, the numbers of > > targets and # LUNs are retrieved from the host during virtio-scsi > > setup. # MdeModulePkg/Bus/Scsi/ScsiBusDxe then scans all MaxTarget > > * MaxLun diff --git a/OvmfPkg/PlatformPei/Platform.c > > b/OvmfPkg/PlatformPei/Platform.c index 1126c65..09c9a2c 100644 > > --- a/OvmfPkg/PlatformPei/Platform.c > > +++ b/OvmfPkg/PlatformPei/Platform.c > > @@ -261,6 +261,13 @@ MiscInitialization ( > > Pmba = POWER_MGMT_REGISTER_Q35 (ICH9_PMBASE); > > AcpiCtlReg = POWER_MGMT_REGISTER_Q35 (ICH9_ACPI_CNTL); > > AcpiEnBit = ICH9_ACPI_CNTL_ACPI_EN; > > + > > + // > > + // Set Root Complex Register Block BAR > > + // > > + PciWrite32 (POWER_MGMT_REGISTER_Q35 (ICH9_RCBA), > > + PcdGet32 (PcdRootComplexBaseAddress) | > > (UINT32)ICH9_RCBA_EN > > + ); > > break; > > default: > > DEBUG ((EFI_D_ERROR, "%a: Unknown Host Bridge Device ID: > > 0x%04x\n", > > This is not the right place for this code, I think. > > If there's going to be a separate DXE driver for the feature, then the > enablement belongs there. > > If not (and I guess there won't be?), then chipset register setting > indeed belongs here -- but not this particular block of code. The > "case" statement in question bears the comment "Query Host Bridge DID > to determine platform type and save to PCD". Let's not mix > responsibilities. > > If you'd like to add the RCBA setting to this function (and I do agree > MiscInitialization is a good place for it), then please add an > explicit "if" and the dependent code to the end of the function. There won't be a separate DXE driver for it, so I agree with you by moving it to the end of the function and setting it conditionally. > > > diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf > > b/OvmfPkg/PlatformPei/PlatformPei.inf index 0307bca..4aa47cc 100644 > > --- a/OvmfPkg/PlatformPei/PlatformPei.inf > > +++ b/OvmfPkg/PlatformPei/PlatformPei.inf > > @@ -74,6 +74,7 @@ > > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize > > gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize > > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId > > + gUefiOvmfPkgTokenSpaceGuid.PcdRootComplexBaseAddress > > gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdS3AcpiReservedMemorySize > > gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress > > gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize > > > > Finally, from your other email it looks like the RCBA will be set to > the fixed address 0xFED1C000. In order to keep the code & comments > up-to-date, please modify MemMapInitialization() as well: the MMIO > address 0xFED1C000 splits the "gap" that starts at 0xFED00400. Please > update the comment accordingly, and add another > AddIoMemoryBaseSizeHob() call below it. OK. Thanks, Paulo -- Paulo Alcantara, C.E.S.A.R Speaking for myself only.