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 751EE1A1E0A for ; Mon, 12 Sep 2016 05:29:35 -0700 (PDT) Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (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 83F06E1CBA; Mon, 12 Sep 2016 12:29:34 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-102.phx2.redhat.com [10.3.116.102]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u8CCTXIp026056; Mon, 12 Sep 2016 08:29:33 -0400 To: Leif Lindholm , Ard Biesheuvel References: <1473674479-20207-1-git-send-email-ard.biesheuvel@linaro.org> <1473674479-20207-2-git-send-email-ard.biesheuvel@linaro.org> <20160912102356.GS16080@bivouac.eciton.net> Cc: edk2-devel@ml01.01.org From: Laszlo Ersek Message-ID: Date: Mon, 12 Sep 2016 14:29:32 +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: <20160912102356.GS16080@bivouac.eciton.net> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Mon, 12 Sep 2016 12:29:34 +0000 (UTC) Subject: Re: [PATCH 1/3] ArmPkg: add driver to force 64-bit MMIO BARs to be allocated above 4 GB 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: Mon, 12 Sep 2016 12:29:35 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 09/12/16 12:23, Leif Lindholm wrote: > On Mon, Sep 12, 2016 at 11:01:17AM +0100, Ard Biesheuvel wrote: >> By default, the generic PciBusDxe driver uses the presence of a ROM BAR >> as a hint that all MMIO BARs should be allocated in the 32-bit region, >> even the ones that could be allocated above 4 GB. The reason is that >> such a ROM BAR may contain code that runs in the context of a legacy >> BIOS (i.e., a CSM), and allocating the 64-bit BARs above 4 GB may put >> them out of reach. >> >> Of course, none of this matters on ARM, and so we can unconditionally >> override this decision. So take the OVMF implementation of the >> IncompatiblePciDeviceSupportDxe driver, rip out the bits that care >> about the presence of a legacy BIOS, and wire it up to the ArmPkg PCI >> PCDs. > > While I agree this solves a real problem, we're now at a point where > we're adding multiple new implementations of a library s/library/protocol/ > to give > standards-compliant platforms the ability to circumvent legacy > workarounds in core code. > > Should this not be the other way around? Yes, it should be. ... IMO, the specific degradation logic in the PCI Bus driver is simplistic; for example, if it finds a PCI oprom for the device, it could parse it to see if there's a legacy BIOS image (too) in that oprom, before summarily degrading the resource. But, implementing that in PciBusDxe now, or even eliminating the quirk completely, would require all downstreams to make up for it in their IncompatiblePciDeviceSupportDxe drivers. Getting shortcuts into core code is easy (initally), getting them out is not so much. So, I completely agree with you, especially that we're abusing this protocol as a resource allocation hint -- as the patches themselves say, there's nothing "incompatible" about the devices we handle. But, changing the status quo at this stage looks really impractical. > And if not, is that not a policy decision that suggests the workaround > belongs as a core library anyway? Based on the PI spec (1.4a, vol 5, "11.5 Incompatible PCI Device Support Protocol"), the protocol should be provided by a platform driver. Now, PciHostBridgeDriverDxe is also a platform driver. I think it's different from this case because PciHostBridgeDriverDxe has so much common / shared logic that it could be meaningfully split into a PciHostBridgeLib class, and a common driver base (linking against the lib class). For IncompatiblePciDeviceSupportDxe however, at least on the level that we work with it, there's zero common logic to factor out. (Okay, yes, you could centralize one or two PcdGet() calls, and perhaps one of the PCDs could be moved to a central .DEC file.) I don't like this situation either, but it's how ABI specs designed for the proprietary world work in practice, I guess. Thanks Laszlo