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
