On Thu, Mar 01, 2018 at 11:17:30AM +0100, Laszlo Ersek wrote: > 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 <[email protected]> > > > > Cc: Jordan Justen <[email protected]> > > Cc: Anthony Perard <[email protected]> > > Cc: Julien Grall <[email protected]> > > Cc: Ruiyu Ni <[email protected]> > > Cc: Laszlo Ersek <[email protected]> > > Cc: Ard Biesheuvel <[email protected]> > > --- > > 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;
Will it cause functional issue? My idea of making the change is like this: 1. ZeroMem() is used to initialize all fields of APERTURE to 0; it can make it in the current place of the patch; 2. In the loop, some fields may be changed by the end of each iteration, and it is the responsibility of the existing code to re-initialize the changed fields to expected values explicitly. It seems not necessary to re-initialize the other fields which will not be changed. However, your advice may be better that merges the initialization code together. I can make the change in the next version of patch. Thanks, Heyi > > Thanks! > Laszlo _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
