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

[Laszlo Ersek <lersek@redhat.com>]

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.

Thanks!
Laszlo