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

[Guo Heyi <heyi.guo@linaro.org>]

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