| Message ID | 1433617803-18892-1-git-send-email-pcacjr@zytor.com |
|---|---|
| State | New |
| Headers | show |
On 2015-06-06 12:10:03, 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 <pcacjr@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 > + > #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 > + > ## 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 Do we need a PCD here? How about just defining ICH9_ROOT_COMPLEX_BASE? -Jordan > + ); > break; > default: > DEBUG ((EFI_D_ERROR, "%a: Unknown Host Bridge Device ID: 0x%04x\n", > 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 > -- > 2.1.0 > > > ------------------------------------------------------------------------------ > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/edk2-devel
On Sat, 06 Jun 2015 22:13:21 -0700 Jordan Justen <jordan.l.justen@intel.com> wrote: > On 2015-06-06 12:10:03, 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 <pcacjr@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 > > + > > #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 > > + ## 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 > > Do we need a PCD here? How about just defining ICH9_ROOT_COMPLEX_BASE? In fact, we don't. Since the MMIO address is being hard-corded in QEMU's ACPI DSDT table and it is unlikely to be changed, so defining ICH9_ROOT_COMPLEX_BASE for it is OK. Thanks, Paulo > > -Jordan > > > + ); > > break; > > default: > > DEBUG ((EFI_D_ERROR, "%a: Unknown Host Bridge Device ID: > > 0x%04x\n", 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 > > -- > > 2.1.0 > > > > > > ------------------------------------------------------------------------------ > > _______________________________________________ > > edk2-devel mailing list > > edk2-devel@lists.sourceforge.net > > https://lists.sourceforge.net/lists/listinfo/edk2-devel > > ------------------------------------------------------------------------------ > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/edk2-devel
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 <pcacjr@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.) > #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. > ## 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. > 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. Thanks Laszlo
On 2015-06-08 02:06:40, Laszlo Ersek wrote: > On 06/06/15 21:10, Paulo Alcantara wrote: > > 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'm not sure there is a good reason for the PMBA PCD at this point. Do you remember why we decided to add a PCD? It doesn't actually change values. I wonder if we were only half committed to the 0x400 => 0xb000 value change at that point? :) I could also see adding a PCD if it looks better for some 'common' code to key off of the PCD, rather than including a chipset specific include file. -Jordan
On 06/08/15 21:07, Jordan Justen wrote: > On 2015-06-08 02:06:40, Laszlo Ersek wrote: >> On 06/06/15 21:10, Paulo Alcantara wrote: >>> 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'm not sure there is a good reason for the PMBA PCD at this point. > > Do you remember why we decided to add a PCD? It doesn't actually > change values. I wonder if we were only half committed to the 0x400 => > 0xb000 value change at that point? :) It's a fixed PCD. I think the argument was simply "hey it's a build time constant". However, at this point PcdAcpiPmBaseAddress is used in quite a few places, so I think it should remain a PCD. > I could also see adding a PCD if it looks better for some 'common' > code to key off of the PCD, rather than including a chipset specific > include file. Options: (1) both PMBA and RCBA are PCDs, (2) PMBA is a PCD, RCBA is a macro, (3) both PMBA and RCBA are macros. I think I prefer (1), but I don't really insist on it: I'd be okay with (2). Whereas, (3) would be really bad. Thanks Laszlo
On Mon, 08 Jun 2015 11:06:40 +0200 Laszlo Ersek <lersek@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 <pcacjr@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
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 + #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 + ## 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", 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
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 <pcacjr@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(+)