[Qemu-devel] [PATCH] OvmfPkg/PlatformPei: Initialise RCBA (B0:D31:F0 0xf0) register

[Qemu-devel] [PATCH] OvmfPkg/PlatformPei: Initialise RCBA (B0:D31:F0 0xf0) register

[Paulo Alcantara]

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
+                 );
       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

Re: [Qemu-devel] [edk2] [PATCH] OvmfPkg/PlatformPei: Initialise RCBA (B0:D31:F0 0xf0) register

[Jordan Justen]

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

Re: [Qemu-devel] [edk2] [PATCH] OvmfPkg/PlatformPei: Initialise RCBA (B0:D31:F0 0xf0) register

[Paulo Alcantara]

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



-- 
Paulo Alcantara, C.E.S.A.R
Speaking for myself only.

[Qemu-devel] [PATCH v2] OvmfPkg/PlatformPei: Initialise RCBA (B0:D31:F0 0xf0) register

[Paulo Alcantara]

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 | 7 +++++++
 OvmfPkg/PlatformPei/Platform.c                | 7 +++++++
 2 files changed, 14 insertions(+)

diff --git a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
index 4f59a7c..b02de1b 100644
--- a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
+++ b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
@@ -90,4 +90,11 @@
 #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_ROOT_COMPLEX_BASE  0xfed1c000
+#define ICH9_RCBA_EN            BIT0
+
 #endif
diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index 1126c65..d69be8b 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),
+                  ICH9_ROOT_COMPLEX_BASE | ICH9_RCBA_EN
+                 );
       break;
     default:
       DEBUG ((EFI_D_ERROR, "%a: Unknown Host Bridge Device ID: 0x%04x\n",
-- 
2.1.0

Re: [Qemu-devel] [PATCH] OvmfPkg/PlatformPei: Initialise RCBA (B0:D31:F0 0xf0) register

[Laszlo Ersek]

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

Re: [Qemu-devel] [edk2] [PATCH] OvmfPkg/PlatformPei: Initialise RCBA (B0:D31:F0 0xf0) register

[Jordan Justen]

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

Re: [Qemu-devel] [edk2] [PATCH] OvmfPkg/PlatformPei: Initialise RCBA (B0:D31:F0 0xf0) register

[Laszlo Ersek]

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

Re: [Qemu-devel] [PATCH] OvmfPkg/PlatformPei: Initialise RCBA (B0:D31:F0 0xf0) register

[Paulo Alcantara]

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

-- 
Paulo Alcantara, C.E.S.A.R
Speaking for myself only.

[Qemu-devel] [PATCH v3] OvmfPkg/PlatformPei: Initialise RCBA (B0:D31:F0 0xf0) register

[Paulo Alcantara]

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 |  5 +++++
 OvmfPkg/PlatformPei/Platform.c                | 14 +++++++++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
index 4f59a7c..18b34a3 100644
--- a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
+++ b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
@@ -77,6 +77,9 @@
 #define ICH9_GEN_PMCON_1          0xA0
 #define ICH9_GEN_PMCON_1_SMI_LOCK   BIT4
 
+#define ICH9_RCBA                 0xF0
+#define ICH9_RCBA_EN                BIT0
+
 //
 // IO ports
 //
@@ -90,4 +93,6 @@
 #define ICH9_SMI_EN_APMC_EN      BIT5
 #define ICH9_SMI_EN_GBL_SMI_EN   BIT0
 
+#define ICH9_ROOT_COMPLEX_BASE 0xFED1C000
+
 #endif
diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index 1126c65..3811162 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -212,13 +212,16 @@ MemMapInitialization (
     // 0xFEC00000    IO-APIC                        4 KB
     // 0xFEC01000    gap                         1020 KB
     // 0xFED00000    HPET                           1 KB
-    // 0xFED00400    gap                         1023 KB
+    // 0xFED00400    gap                          111 KB
+    // 0xFED1C000    RCRB                          16 KB
+    // 0xFED20000    gap                          896 KB
     // 0xFEE00000    LAPIC                          1 MB
     //
     AddIoMemoryRangeHob (TopOfLowRam < BASE_2GB ?
                          BASE_2GB : TopOfLowRam, 0xFC000000);
     AddIoMemoryBaseSizeHob (0xFEC00000, SIZE_4KB);
     AddIoMemoryBaseSizeHob (0xFED00000, SIZE_1KB);
+    AddIoMemoryBaseSizeHob (ICH9_ROOT_COMPLEX_BASE, SIZE_16KB);
     AddIoMemoryBaseSizeHob (PcdGet32(PcdCpuLocalApicBaseAddress), SIZE_1MB);
   }
 }
@@ -292,6 +295,15 @@ MiscInitialization (
     //
     PciOr8 (AcpiCtlReg, AcpiEnBit);
   }
+
+  if (HostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
+    //
+    // Set Root Complex Register Block BAR
+    //
+    PciWrite32 (POWER_MGMT_REGISTER_Q35 (ICH9_RCBA),
+                ICH9_ROOT_COMPLEX_BASE | ICH9_RCBA_EN
+               );
+  }
 }
 
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH v3] OvmfPkg/PlatformPei: Initialise RCBA (B0:D31:F0 0xf0) register

[Laszlo Ersek]

On 06/09/15 00:07, 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 |  5 +++++
>  OvmfPkg/PlatformPei/Platform.c                | 14 +++++++++++++-
>  2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
> index 4f59a7c..18b34a3 100644
> --- a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
> +++ b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
> @@ -77,6 +77,9 @@
>  #define ICH9_GEN_PMCON_1          0xA0
>  #define ICH9_GEN_PMCON_1_SMI_LOCK   BIT4
>  
> +#define ICH9_RCBA                 0xF0
> +#define ICH9_RCBA_EN                BIT0
> +
>  //
>  // IO ports
>  //
> @@ -90,4 +93,6 @@
>  #define ICH9_SMI_EN_APMC_EN      BIT5
>  #define ICH9_SMI_EN_GBL_SMI_EN   BIT0
>  
> +#define ICH9_ROOT_COMPLEX_BASE 0xFED1C000
> +
>  #endif
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index 1126c65..3811162 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -212,13 +212,16 @@ MemMapInitialization (
>      // 0xFEC00000    IO-APIC                        4 KB
>      // 0xFEC01000    gap                         1020 KB
>      // 0xFED00000    HPET                           1 KB
> -    // 0xFED00400    gap                         1023 KB
> +    // 0xFED00400    gap                          111 KB
> +    // 0xFED1C000    RCRB                          16 KB
> +    // 0xFED20000    gap                          896 KB
>      // 0xFEE00000    LAPIC                          1 MB
>      //
>      AddIoMemoryRangeHob (TopOfLowRam < BASE_2GB ?
>                           BASE_2GB : TopOfLowRam, 0xFC000000);
>      AddIoMemoryBaseSizeHob (0xFEC00000, SIZE_4KB);
>      AddIoMemoryBaseSizeHob (0xFED00000, SIZE_1KB);
> +    AddIoMemoryBaseSizeHob (ICH9_ROOT_COMPLEX_BASE, SIZE_16KB);
>      AddIoMemoryBaseSizeHob (PcdGet32(PcdCpuLocalApicBaseAddress), SIZE_1MB);
>    }
>  }
> @@ -292,6 +295,15 @@ MiscInitialization (
>      //
>      PciOr8 (AcpiCtlReg, AcpiEnBit);
>    }
> +
> +  if (HostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
> +    //
> +    // Set Root Complex Register Block BAR
> +    //
> +    PciWrite32 (POWER_MGMT_REGISTER_Q35 (ICH9_RCBA),
> +                ICH9_ROOT_COMPLEX_BASE | ICH9_RCBA_EN
> +               );
> +  }
>  }

The right formatting for the last function call would be

PciWrite32 (
  POWER_MGMT_REGISTER_Q35 (ICH9_RCBA),
  ICH9_ROOT_COMPLEX_BASE | ICH9_RCBA_EN
  );

or

PciWrite32 (POWER_MGMT_REGISTER_Q35 (ICH9_RCBA),
  ICH9_ROOT_COMPLEX_BASE | ICH9_RCBA_EN);

but I won't obsess about it.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Let's see what Jordan says.

Thanks
Laszlo

[Qemu-devel] [PATCH v4] OvmfPkg/PlatformPei: Initialise RCBA (B0:D31:F0 0xf0) register

[Paulo Alcantara]

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 |  5 +++++
 OvmfPkg/PlatformPei/Platform.c                | 15 ++++++++++++++-
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
index 4f59a7c..18b34a3 100644
--- a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
+++ b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
@@ -77,6 +77,9 @@
 #define ICH9_GEN_PMCON_1          0xA0
 #define ICH9_GEN_PMCON_1_SMI_LOCK   BIT4
 
+#define ICH9_RCBA                 0xF0
+#define ICH9_RCBA_EN                BIT0
+
 //
 // IO ports
 //
@@ -90,4 +93,6 @@
 #define ICH9_SMI_EN_APMC_EN      BIT5
 #define ICH9_SMI_EN_GBL_SMI_EN   BIT0
 
+#define ICH9_ROOT_COMPLEX_BASE 0xFED1C000
+
 #endif
diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index 1126c65..82252f9 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -212,13 +212,16 @@ MemMapInitialization (
     // 0xFEC00000    IO-APIC                        4 KB
     // 0xFEC01000    gap                         1020 KB
     // 0xFED00000    HPET                           1 KB
-    // 0xFED00400    gap                         1023 KB
+    // 0xFED00400    gap                          111 KB
+    // 0xFED1C000    RCRB                          16 KB
+    // 0xFED20000    gap                          896 KB
     // 0xFEE00000    LAPIC                          1 MB
     //
     AddIoMemoryRangeHob (TopOfLowRam < BASE_2GB ?
                          BASE_2GB : TopOfLowRam, 0xFC000000);
     AddIoMemoryBaseSizeHob (0xFEC00000, SIZE_4KB);
     AddIoMemoryBaseSizeHob (0xFED00000, SIZE_1KB);
+    AddIoMemoryBaseSizeHob (ICH9_ROOT_COMPLEX_BASE, SIZE_16KB);
     AddIoMemoryBaseSizeHob (PcdGet32(PcdCpuLocalApicBaseAddress), SIZE_1MB);
   }
 }
@@ -292,6 +295,16 @@ MiscInitialization (
     //
     PciOr8 (AcpiCtlReg, AcpiEnBit);
   }
+
+  if (HostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
+    //
+    // Set Root Complex Register Block BAR
+    //
+    PciWrite32 (
+      POWER_MGMT_REGISTER_Q35 (ICH9_RCBA),
+      ICH9_ROOT_COMPLEX_BASE | ICH9_RCBA_EN
+      );
+  }
 }
 
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH v4] OvmfPkg/PlatformPei: Initialise RCBA (B0:D31:F0 0xf0) register

[Laszlo Ersek]

On 06/09/15 00:42, 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 |  5 +++++
>  OvmfPkg/PlatformPei/Platform.c                | 15 ++++++++++++++-
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
> index 4f59a7c..18b34a3 100644
> --- a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
> +++ b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
> @@ -77,6 +77,9 @@
>  #define ICH9_GEN_PMCON_1          0xA0
>  #define ICH9_GEN_PMCON_1_SMI_LOCK   BIT4
>  
> +#define ICH9_RCBA                 0xF0
> +#define ICH9_RCBA_EN                BIT0
> +
>  //
>  // IO ports
>  //
> @@ -90,4 +93,6 @@
>  #define ICH9_SMI_EN_APMC_EN      BIT5
>  #define ICH9_SMI_EN_GBL_SMI_EN   BIT0
>  
> +#define ICH9_ROOT_COMPLEX_BASE 0xFED1C000
> +
>  #endif
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index 1126c65..82252f9 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -212,13 +212,16 @@ MemMapInitialization (
>      // 0xFEC00000    IO-APIC                        4 KB
>      // 0xFEC01000    gap                         1020 KB
>      // 0xFED00000    HPET                           1 KB
> -    // 0xFED00400    gap                         1023 KB
> +    // 0xFED00400    gap                          111 KB
> +    // 0xFED1C000    RCRB                          16 KB
> +    // 0xFED20000    gap                          896 KB
>      // 0xFEE00000    LAPIC                          1 MB
>      //
>      AddIoMemoryRangeHob (TopOfLowRam < BASE_2GB ?
>                           BASE_2GB : TopOfLowRam, 0xFC000000);
>      AddIoMemoryBaseSizeHob (0xFEC00000, SIZE_4KB);
>      AddIoMemoryBaseSizeHob (0xFED00000, SIZE_1KB);
> +    AddIoMemoryBaseSizeHob (ICH9_ROOT_COMPLEX_BASE, SIZE_16KB);
>      AddIoMemoryBaseSizeHob (PcdGet32(PcdCpuLocalApicBaseAddress), SIZE_1MB);
>    }
>  }
> @@ -292,6 +295,16 @@ MiscInitialization (
>      //
>      PciOr8 (AcpiCtlReg, AcpiEnBit);
>    }
> +
> +  if (HostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
> +    //
> +    // Set Root Complex Register Block BAR
> +    //
> +    PciWrite32 (
> +      POWER_MGMT_REGISTER_Q35 (ICH9_RCBA),
> +      ICH9_ROOT_COMPLEX_BASE | ICH9_RCBA_EN
> +      );
> +  }
>  }

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [Qemu-devel] [edk2] [PATCH v3] OvmfPkg/PlatformPei: Initialise RCBA (B0:D31:F0 0xf0) register

[Jordan Justen]

On 2015-06-08 15:07:13, 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 |  5 +++++
>  OvmfPkg/PlatformPei/Platform.c                | 14 +++++++++++++-
>  2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
> index 4f59a7c..18b34a3 100644
> --- a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
> +++ b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
> @@ -77,6 +77,9 @@
>  #define ICH9_GEN_PMCON_1          0xA0
>  #define ICH9_GEN_PMCON_1_SMI_LOCK   BIT4
>  
> +#define ICH9_RCBA                 0xF0
> +#define ICH9_RCBA_EN                BIT0
> +
>  //
>  // IO ports
>  //
> @@ -90,4 +93,6 @@
>  #define ICH9_SMI_EN_APMC_EN      BIT5
>  #define ICH9_SMI_EN_GBL_SMI_EN   BIT0
>  
> +#define ICH9_ROOT_COMPLEX_BASE 0xFED1C000
> +
>  #endif
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index 1126c65..3811162 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -212,13 +212,16 @@ MemMapInitialization (
>      // 0xFEC00000    IO-APIC                        4 KB
>      // 0xFEC01000    gap                         1020 KB
>      // 0xFED00000    HPET                           1 KB
> -    // 0xFED00400    gap                         1023 KB
> +    // 0xFED00400    gap                          111 KB
> +    // 0xFED1C000    RCRB                          16 KB

Should we make this conditional?
    // 0xFED1C000    gap (PIIX4) / RCRB (ICH9)     16 KB

... and make mHostBridgeDevId a global var, and then conditionally add
the memory io HOB?

I don't think it is critical, but with that
Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>

-Jordan

> +    // 0xFED20000    gap                          896 KB
>      // 0xFEE00000    LAPIC                          1 MB
>      //
>      AddIoMemoryRangeHob (TopOfLowRam < BASE_2GB ?
>                           BASE_2GB : TopOfLowRam, 0xFC000000);
>      AddIoMemoryBaseSizeHob (0xFEC00000, SIZE_4KB);
>      AddIoMemoryBaseSizeHob (0xFED00000, SIZE_1KB);
> +    AddIoMemoryBaseSizeHob (ICH9_ROOT_COMPLEX_BASE, SIZE_16KB);
>      AddIoMemoryBaseSizeHob (PcdGet32(PcdCpuLocalApicBaseAddress), SIZE_1MB);
>    }
>  }
> @@ -292,6 +295,15 @@ MiscInitialization (
>      //
>      PciOr8 (AcpiCtlReg, AcpiEnBit);
>    }
> +
> +  if (HostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
> +    //
> +    // Set Root Complex Register Block BAR
> +    //
> +    PciWrite32 (POWER_MGMT_REGISTER_Q35 (ICH9_RCBA),
> +                ICH9_ROOT_COMPLEX_BASE | ICH9_RCBA_EN
> +               );
> +  }
>  }
>  
>  
> -- 
> 2.1.0
> 
> 
> ------------------------------------------------------------------------------
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/edk2-devel

Re: [Qemu-devel] [edk2] [PATCH v3] OvmfPkg/PlatformPei: Initialise RCBA (B0:D31:F0 0xf0) register

[Laszlo Ersek]

On 06/09/15 00:49, Jordan Justen wrote:
> On 2015-06-08 15:07:13, 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 |  5 +++++
>>  OvmfPkg/PlatformPei/Platform.c                | 14 +++++++++++++-
>>  2 files changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
>> index 4f59a7c..18b34a3 100644
>> --- a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
>> +++ b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
>> @@ -77,6 +77,9 @@
>>  #define ICH9_GEN_PMCON_1          0xA0
>>  #define ICH9_GEN_PMCON_1_SMI_LOCK   BIT4
>>  
>> +#define ICH9_RCBA                 0xF0
>> +#define ICH9_RCBA_EN                BIT0
>> +
>>  //
>>  // IO ports
>>  //
>> @@ -90,4 +93,6 @@
>>  #define ICH9_SMI_EN_APMC_EN      BIT5
>>  #define ICH9_SMI_EN_GBL_SMI_EN   BIT0
>>  
>> +#define ICH9_ROOT_COMPLEX_BASE 0xFED1C000
>> +
>>  #endif
>> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
>> index 1126c65..3811162 100644
>> --- a/OvmfPkg/PlatformPei/Platform.c
>> +++ b/OvmfPkg/PlatformPei/Platform.c
>> @@ -212,13 +212,16 @@ MemMapInitialization (
>>      // 0xFEC00000    IO-APIC                        4 KB
>>      // 0xFEC01000    gap                         1020 KB
>>      // 0xFED00000    HPET                           1 KB
>> -    // 0xFED00400    gap                         1023 KB
>> +    // 0xFED00400    gap                          111 KB
>> +    // 0xFED1C000    RCRB                          16 KB
> 
> Should we make this conditional?
>     // 0xFED1C000    gap (PIIX4) / RCRB (ICH9)     16 KB
> 
> ... and make mHostBridgeDevId a global var, and then conditionally add
> the memory io HOB?

Good point.

Laszlo

> I don't think it is critical, but with that
> Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
> 
> -Jordan
> 
>> +    // 0xFED20000    gap                          896 KB
>>      // 0xFEE00000    LAPIC                          1 MB
>>      //
>>      AddIoMemoryRangeHob (TopOfLowRam < BASE_2GB ?
>>                           BASE_2GB : TopOfLowRam, 0xFC000000);
>>      AddIoMemoryBaseSizeHob (0xFEC00000, SIZE_4KB);
>>      AddIoMemoryBaseSizeHob (0xFED00000, SIZE_1KB);
>> +    AddIoMemoryBaseSizeHob (ICH9_ROOT_COMPLEX_BASE, SIZE_16KB);
>>      AddIoMemoryBaseSizeHob (PcdGet32(PcdCpuLocalApicBaseAddress), SIZE_1MB);
>>    }
>>  }
>> @@ -292,6 +295,15 @@ MiscInitialization (
>>      //
>>      PciOr8 (AcpiCtlReg, AcpiEnBit);
>>    }
>> +
>> +  if (HostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
>> +    //
>> +    // Set Root Complex Register Block BAR
>> +    //
>> +    PciWrite32 (POWER_MGMT_REGISTER_Q35 (ICH9_RCBA),
>> +                ICH9_ROOT_COMPLEX_BASE | ICH9_RCBA_EN
>> +               );
>> +  }
>>  }
>>  
>>  
>> -- 
>> 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
> 

Re: [Qemu-devel] [edk2] [PATCH v3] OvmfPkg/PlatformPei: Initialise RCBA (B0:D31:F0 0xf0) register

[Paulo Alcantara]

On Tue, 09 Jun 2015 01:09:19 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 06/09/15 00:49, Jordan Justen wrote:
> > On 2015-06-08 15:07:13, 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 |  5 +++++
> >>  OvmfPkg/PlatformPei/Platform.c                | 14 +++++++++++++-
> >>  2 files changed, 18 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
> >> b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h index
> >> 4f59a7c..18b34a3 100644 ---
> >> a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h +++
> >> b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h @@ -77,6 +77,9 @@
> >>  #define ICH9_GEN_PMCON_1          0xA0
> >>  #define ICH9_GEN_PMCON_1_SMI_LOCK   BIT4
> >>  
> >> +#define ICH9_RCBA                 0xF0
> >> +#define ICH9_RCBA_EN                BIT0
> >> +
> >>  //
> >>  // IO ports
> >>  //
> >> @@ -90,4 +93,6 @@
> >>  #define ICH9_SMI_EN_APMC_EN      BIT5
> >>  #define ICH9_SMI_EN_GBL_SMI_EN   BIT0
> >>  
> >> +#define ICH9_ROOT_COMPLEX_BASE 0xFED1C000
> >> +
> >>  #endif
> >> diff --git a/OvmfPkg/PlatformPei/Platform.c
> >> b/OvmfPkg/PlatformPei/Platform.c index 1126c65..3811162 100644
> >> --- a/OvmfPkg/PlatformPei/Platform.c
> >> +++ b/OvmfPkg/PlatformPei/Platform.c
> >> @@ -212,13 +212,16 @@ MemMapInitialization (
> >>      // 0xFEC00000    IO-APIC                        4 KB
> >>      // 0xFEC01000    gap                         1020 KB
> >>      // 0xFED00000    HPET                           1 KB
> >> -    // 0xFED00400    gap                         1023 KB
> >> +    // 0xFED00400    gap                          111 KB
> >> +    // 0xFED1C000    RCRB                          16 KB
> > 
> > Should we make this conditional?
> >     // 0xFED1C000    gap (PIIX4) / RCRB (ICH9)     16 KB
> > 
> > ... and make mHostBridgeDevId a global var, and then conditionally
> > add the memory io HOB?
> 
> Good point.

Good point, indeed. Unlike HPET, I/O APIC and other addresses that will
be shared between PIIX4 and ICH9, the RCRB will be exclusive to ICH9. I
can make these changes on top this patch, or do you guys prefer it to
be placed in another?

Thanks,

Paulo

-- 
Paulo Alcantara, C.E.S.A.R
Speaking for myself only.

Re: [Qemu-devel] [edk2] [PATCH v3] OvmfPkg/PlatformPei: Initialise RCBA (B0:D31:F0 0xf0) register

[Laszlo Ersek]

On 06/09/15 01:30, Paulo Alcantara wrote:
> On Tue, 09 Jun 2015 01:09:19 +0200
> Laszlo Ersek <lersek@redhat.com> wrote:
> 
>> On 06/09/15 00:49, Jordan Justen wrote:
>>> On 2015-06-08 15:07:13, 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 |  5 +++++
>>>>  OvmfPkg/PlatformPei/Platform.c                | 14 +++++++++++++-
>>>>  2 files changed, 18 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
>>>> b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h index
>>>> 4f59a7c..18b34a3 100644 ---
>>>> a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h +++
>>>> b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h @@ -77,6 +77,9 @@
>>>>  #define ICH9_GEN_PMCON_1          0xA0
>>>>  #define ICH9_GEN_PMCON_1_SMI_LOCK   BIT4
>>>>  
>>>> +#define ICH9_RCBA                 0xF0
>>>> +#define ICH9_RCBA_EN                BIT0
>>>> +
>>>>  //
>>>>  // IO ports
>>>>  //
>>>> @@ -90,4 +93,6 @@
>>>>  #define ICH9_SMI_EN_APMC_EN      BIT5
>>>>  #define ICH9_SMI_EN_GBL_SMI_EN   BIT0
>>>>  
>>>> +#define ICH9_ROOT_COMPLEX_BASE 0xFED1C000
>>>> +
>>>>  #endif
>>>> diff --git a/OvmfPkg/PlatformPei/Platform.c
>>>> b/OvmfPkg/PlatformPei/Platform.c index 1126c65..3811162 100644
>>>> --- a/OvmfPkg/PlatformPei/Platform.c
>>>> +++ b/OvmfPkg/PlatformPei/Platform.c
>>>> @@ -212,13 +212,16 @@ MemMapInitialization (
>>>>      // 0xFEC00000    IO-APIC                        4 KB
>>>>      // 0xFEC01000    gap                         1020 KB
>>>>      // 0xFED00000    HPET                           1 KB
>>>> -    // 0xFED00400    gap                         1023 KB
>>>> +    // 0xFED00400    gap                          111 KB
>>>> +    // 0xFED1C000    RCRB                          16 KB
>>>
>>> Should we make this conditional?
>>>     // 0xFED1C000    gap (PIIX4) / RCRB (ICH9)     16 KB
>>>
>>> ... and make mHostBridgeDevId a global var, and then conditionally
>>> add the memory io HOB?
>>
>> Good point.
> 
> Good point, indeed. Unlike HPET, I/O APIC and other addresses that will
> be shared between PIIX4 and ICH9, the RCRB will be exclusive to ICH9. I
> can make these changes on top this patch, or do you guys prefer it to
> be placed in another?

I think mHostBridgeDevId should be made an extern in the first patch,
and then the second patch could be this one, also including the
conditional stuff in MemMapInitialization().

Thanks
Laszlo