On Mon, 08 Jun 2015 11:06:40 +0200 Laszlo Ersek <ler...@redhat.com> 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 <pca...@zytor.com> > > --- > > 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.
