From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id D67621A1E46 for ; Wed, 31 Aug 2016 06:51:02 -0700 (PDT) Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1BBF4BD00E; Wed, 31 Aug 2016 13:51:02 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-108.phx2.redhat.com [10.3.116.108]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u7VDp0ht004160; Wed, 31 Aug 2016 09:51:01 -0400 To: Ard Biesheuvel References: <1471847752-26574-1-git-send-email-ard.biesheuvel@linaro.org> <1471847752-26574-3-git-send-email-ard.biesheuvel@linaro.org> <8ba58ec8-9360-4805-c9a5-b9d5c193fdb0@redhat.com> Cc: "edk2-devel@lists.01.org" From: Laszlo Ersek Message-ID: <43a8b0d8-3368-2841-f6eb-d34fcefb32b1@redhat.com> Date: Wed, 31 Aug 2016 15:51:00 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.68 on 10.5.11.27 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Wed, 31 Aug 2016 13:51:02 +0000 (UTC) Subject: Re: [PATCH 2/5] ArmVirtPkg: implement FdtPciHostBridgeLib X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 31 Aug 2016 13:51:03 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 08/24/16 17:19, Ard Biesheuvel wrote: > On 24 August 2016 at 17:01, Laszlo Ersek 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 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