On 08/24/16 17:19, Ard Biesheuvel wrote: > On 24 August 2016 at 17:01, Laszlo Ersek <ler...@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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
