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 <address@hidden>
> ---
>  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