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