Re: [PATCH 2/5] ArmVirtPkg: implement FdtPciHostBridgeLib

[Laszlo Ersek <lersek@redhat.com>]

On 08/24/16 17:19, Ard Biesheuvel wrote:
> On 24 August 2016 at 17:01, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 08/22/16 02:35, Ard Biesheuvel wrote:

>>> +  PcdSet64 (PcdPciIoTranslation, IoTranslation);
>>
>> I agree this is necessary, but it's not in the right place, plus as-is,
>> it is not sufficient.
>>
>> In <https://tianocore.acgmultimedia.com/show_bug.cgi?id=65#c0> I wrote
>> -- and I'm slightly reformatting it here, because the formatting got
>> lost in the GitHub -> BZ migration --:
>>
>>     The main customization in ArmVirtPkg/PciHostBridgeDxe is that it
>>     emulates IO port accesses with an MMIO range, whose base address is
>>     parsed from the DTB.
>>
>>     The core PciHostBridgeDxe driver delegates the IO port access
>>     implementation to EFI_CPU_IO2_PROTOCOL. @ardbiesheuvel recently
>>     implemented ArmPkg/Drivers/ArmPciCpuIo2Dxe (in commit 3dece14)
>>     which provides this protocol, backed by the same kind of
>>     translation as described above. The base address comes from
>>     gArmTokenSpaceGuid.PcdPciIoTranslation.
>>
>>     Therefore:
>>
>>     (1) We should extend our ArmVirtPkg/Library/FdtPciPcdProducerLib
>>         plugin library to locate the IO translation offset in the DTB,
>>         and store it in the above PCD.
>>
>>     (2) We should include ArmPciCpuIo2Dxe in the ArmVirtQemu builds,
>>         plugging the above library into it.
>>
>>     (3) We should implement a PciHostBridgeLib instance, and plug it
>>         into the core PciHostBridgeDxe driver.
>>
>>         We should create one PCI_ROOT_BRIDGE object, populating it with
>>         the FTD Client code we are currently using in
>>         ArmVirtPkg/PciHostBridgeDxe.
>>
>>     (4) Extra benefit: the core PciHostBridgeDxe driver supports 64-bit
>>         MMIO, we just have to parse those values from the DTB as well.
>>
>> Steps (2) through (4) are implemented by this series, but I don't think
>> the above PcdSet64() call satisfies (1). What guarantees that by the
>> time ArmPciCpuIo2Dxe looks at it, PcdPciIoTranslation will be set?
>>
>> ... Especially because PciHostBridgeDxe.inf has a DEPEX on
>> gEfiCpuIo2ProtocolGuid, so the PcdSet64() above is bound to run *after*
>> ArmPciCpuIo2Dxe is dispatched?
>>
>> Hmmmm... Are you making the argument that the PCD is set *between*
>> ArmPciCpuIo2Dxe's dispatch and actual use of the PCD? That is:
>>
>> - ArmPciCpuIo2Dxe is dispatched
>> - PciHostBridgeDxe is dispatched
>> - we set the PCD in this lib (as part of PciHostBridgeDxe's startup)
>> - (much later) something makes a PCI IO or MEM access that causes
>>   PciHostBridgeDxe to talk to ArmPciCpuIo2Dxe
>> - ArmPciCpuIo2Dxe consumes the PCD
>>
>> I think this is a valid argument to make -- I've even checked:
>> ArmPciCpuIo2Dxe indeed performs a PcdGet64() on every CpuIoServiceRead()
>> / CpuIoServiceWrite(), and none in its entry point --, but it has to be
>> explained in detail.
>>
>> This borders on a hack -- because ArmPciCpuIo2Dxe is not actually ready
>> for use when its entry point exits with success --, but I think we can
>> allow it, both ArmPciCpuIo2Dxe and this lib being platform specific, as
>> long as we document it profusely.
>>
>> So please describe this quirk in the commit message, and add a large
>> comment right above the PcdSet64() call.
>>
>> (Also, did you test this series with std VGA, on TCG? That will actually
>> put the IO translation to use.)
>>
>> The alternative to the commit msg addition / code comment would be my
>> suggestion in (1), that is, to extend FdtPciPcdProducerLib with setting
>> PcdPciIoTranslation, and to link it into ArmPciCpuIo2Dxe via NULL
>> library resolution. I guess you don't like that, which is fine, but then
>> please add the docs. Thanks.
>>
> 
> Actually, I prefer your original suggestion, to add it to
> FdtPciPcdProducerLib. That way, we no longer have to set any PCDs in
> this code, which is much cleaner imo

Sounds good, thanks!

In this case, the IoTranslation variable should become local to
ProcessPciHost() -- and ProcessPciHost() only -- in this patch, plus I
think we should also add

  ASSERT (PcdGet64 (PcdPciIoTranslation) == IoTranslation);

to ProcessPciHost() here, under the DTB_PCI_HOST_RANGE_IO branch.

Furthermore, the zero-assignment of the now-local IoTranslation variable
should probably be moved out from under the
'-Werror=maybe-uninitialized' comment, because it won't be an output
parameter any longer. What do you think?

Thanks
Laszlo