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

[Paulo Alcantara <pcacjr@zytor.com>]

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.