From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x233.google.com (mail-it0-x233.google.com [IPv6:2607:f8b0:4001:c0b::233]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 387691A1E47 for ; Wed, 31 Aug 2016 06:54:35 -0700 (PDT) Received: by mail-it0-x233.google.com with SMTP id e124so8976993ith.0 for ; Wed, 31 Aug 2016 06:54:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=8hqE4BC4MUbcVGZTTVaoic3BupVjrG5OChJ+LDNMqKo=; b=cBTy9p90Lj+N6xvi4ue7SJ0ijWDmfaP5CpSNzOUQ9DGXqVd+6DWUSiqXvve8RWIv5n YxDlqSDXk/fTjGr7LNjVof+1BeRqjH4UTBVZFSYMX6S+Hiafc08tWgXJE5V8OyLBF24Z p96/JRubFYH63K3hbWlbgwEUlA4MwVkGRwU1Y= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=8hqE4BC4MUbcVGZTTVaoic3BupVjrG5OChJ+LDNMqKo=; b=fwX1099HZgTSA/q1u+TzmfEMryT5K0uUmNMtnUc7Jcq19BS/n738enHSO4yIcPQQzD bkd1VCMwWtDYOr7wQJ064NaQtvRPNajwbhseisF0i6R9OdXNribBb19ETvR/btqiL/oB rxUs63ftRh1ihGS76btVk3dJTjNPdbX8eCgKEx34OThOuwq2rwJe4Tz0NZwljj5f/+JQ 0lZPtWExcqsrGUOKTlaOUN2wVWuNOoK2/2Mz+iwzCwJLQUUPqFJIT5Iw4b/QaK91BWrX 4N30OEEjEJHBGNoukr8+xBcNZwZSZ0DRf5zDTVZXdVnAm/jaXubV0PMuYrG2cvqnc06g ILTA== X-Gm-Message-State: AE9vXwOwaw+iMbPdQM2wxnxYs4vXIYhIGquiWZU5AuoITKdBRxPC2tFDefbfaGOooOJH0EVkMTfi3rL/uTaQS7jZ X-Received: by 10.36.57.215 with SMTP id l206mr33322512ita.5.1472651674372; Wed, 31 Aug 2016 06:54:34 -0700 (PDT) MIME-Version: 1.0 Received: by 10.36.204.195 with HTTP; Wed, 31 Aug 2016 06:54:33 -0700 (PDT) In-Reply-To: <43a8b0d8-3368-2841-f6eb-d34fcefb32b1@redhat.com> 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> <43a8b0d8-3368-2841-f6eb-d34fcefb32b1@redhat.com> From: Ard Biesheuvel Date: Wed, 31 Aug 2016 14:54:33 +0100 Message-ID: To: Laszlo Ersek Cc: "edk2-devel@lists.01.org" 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:54:35 -0000 Content-Type: text/plain; charset=UTF-8 On 31 August 2016 at 14:51, Laszlo Ersek wrote: > 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? > Yes, that was my idea as well. The FdtPciPcdProducerLib will need some work, but it is ultimately a much cleaner approach.