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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 31 August 2016 at 14:51, Laszlo Ersek <lersek@redhat.com> wrote:
> 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?
>

Yes, that was my idea as well. The FdtPciPcdProducerLib will need some
work, but it is ultimately a much cleaner approach.