Re: [PATCH v5 2/6] OvmfPkg/PciHostBridgeLib: Init PCI aperture to 0

[Laszlo Ersek <lersek@redhat.com>]

Hello Heyi,

On 03/01/18 07:57, Heyi Guo wrote:
> Use ZeroMem to initialize all fields in temporary
> PCI_ROOT_BRIDGE_APERTURE variables to zero. This is not mandatory but
> is helpful for future extension: when we add new fields to
> PCI_ROOT_BRIDGE_APERTURE and the default value of these fields can
> safely be zero, this code will not suffer from an additional
> change.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Julien Grall <julien.grall@linaro.org>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 4 ++++
>  OvmfPkg/Library/PciHostBridgeLib/XenSupport.c       | 5 +++++
>  2 files changed, 9 insertions(+)
>
> diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
> index ff837035caff..4a650a4c6df9 100644
> --- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
> +++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
> @@ -217,6 +217,10 @@ PciHostBridgeGetRootBridges (
>    PCI_ROOT_BRIDGE_APERTURE Mem;
>    PCI_ROOT_BRIDGE_APERTURE MemAbove4G;
>
> +  ZeroMem (&Io, sizeof (Io));
> +  ZeroMem (&Mem, sizeof (Mem));
> +  ZeroMem (&MemAbove4G, sizeof (MemAbove4G));
> +
>    if (PcdGetBool (PcdPciDisableBusEnumeration)) {
>      return ScanForRootBridges (Count);
>    }

This is OK. (Although for a trivial perf improvement, you could move the
ZeroMem() calls after the PcdGetBool() / return. Not necessary, up to
you.)

However:

> diff --git a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
> index 31c63ae19e0a..aaf101dfcb0e 100644
> --- a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
> +++ b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
> @@ -193,6 +193,11 @@ ScanForRootBridges (
>
>    *NumberOfRootBridges = 0;
>    RootBridges = NULL;
> +  ZeroMem (&Io, sizeof (Io));
> +  ZeroMem (&Mem, sizeof (Mem));
> +  ZeroMem (&MemAbove4G, sizeof (MemAbove4G));
> +  ZeroMem (&PMem, sizeof (PMem));
> +  ZeroMem (&PMemAbove4G, sizeof (PMemAbove4G));
>
>    //
>    // After scanning all the PCI devices on the PCI root bridge's primary bus,
>

these ZeroMem() calls are not in the correct place. Please move them
into the "PrimaryBus" loop just underneath. That loop works like this:

For each primary bus:

  (1) set all of the aperture variables to "nonexistent":

    Io.Base = Mem.Base = MemAbove4G.Base = PMem.Base = PMemAbove4G.Base = MAX_UINT64;
    Io.Limit = Mem.Limit = MemAbove4G.Limit = PMem.Limit = PMemAbove4G.Limit = 0;

  (2) accumulate the BARs of the devices on the bus into the aperture
      variables

  (3) call InitRootBridge() with the aperture variables


That is, the ZeroMem() calls that you are adding have to be part of step
(1). So, please replace the assignments

    Io.Base = Mem.Base = MemAbove4G.Base = PMem.Base = PMemAbove4G.Base = MAX_UINT64;
    Io.Limit = Mem.Limit = MemAbove4G.Limit = PMem.Limit = PMemAbove4G.Limit = 0;

with

    ZeroMem (&Io, sizeof (Io));
    ZeroMem (&Mem, sizeof (Mem));
    ZeroMem (&MemAbove4G, sizeof (MemAbove4G));
    ZeroMem (&PMem, sizeof (PMem));
    ZeroMem (&PMemAbove4G, sizeof (PMemAbove4G));
    Io.Base = Mem.Base = MemAbove4G.Base = PMem.Base = PMemAbove4G.Base = MAX_UINT64;

Thanks!
Laszlo