On Fri, Mar 02, 2018 at 11:19:31AM +0100, Laszlo Ersek wrote: > On 03/01/18 11:48, Guo Heyi wrote: > > On Thu, Mar 01, 2018 at 11:17:30AM +0100, Laszlo Ersek wrote: > >> On 03/01/18 07:57, Heyi Guo wrote: > > >>> 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. > > Yes, if it's not a big problem for you, please implement my request. > Going forward I wouldn't like to depend on such intricate details as > described in your point (2). Namely, in any other C project, I would > suggest that we write: > > for (PrimaryBus = 0; PrimaryBus <= PCI_MAX_BUS; PrimaryBus = SubBus + 1) { > PCI_ROOT_BRIDGE_APERTURE Io = { .Base = MAX_UINT64 }, > Mem = Io, > MemAbove4G = Io, > PMem = Io, > PMemAbove4G = Io; > /* ... */ > } > > In other words, I would: > - move the definition of the structs into the loop (sort of accepted, > but not really liked in edk2), > - use real C initialization (forbidden in edk2), > - use designated initializers for the first object, which clears the > unlisted fields (C99, forbidden in edk2), > - initialize the rest of the structs from the first struct where I used > the designated initializer explicitly. > > Moving the ZeroMem() into the loop is the closest approximation of this, > for edk2.
OK, I can do that in the next version of patch. Thanks, Heyi > > Thanks! > Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel