From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk0-x234.google.com (mail-qk0-x234.google.com [IPv6:2607:f8b0:400d:c09::234]) (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 A8B661A1EFE for ; Thu, 15 Sep 2016 08:28:34 -0700 (PDT) Received: by mail-qk0-x234.google.com with SMTP id t7so57114492qkh.2 for ; Thu, 15 Sep 2016 08:28:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=NA0eRpjLosgLlxmS2gPl6aVGTcuo1PsoVb4CZBhiHQk=; b=CKhJmChs4njHPLJD7TadSNXBZSv3RgpJiPjDh1YRfJrWUohFcopfY2zxUzYkNZwJWa gHAOnfMyl2LB/RD3OALRna5P5XKayw0gdhTwC6cLGMBevyVnMtJl405iXv/IGSOsJ4S8 dgxoJKF119b6iJMJkjUb8cWcE7pywTG8A0s+A= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=NA0eRpjLosgLlxmS2gPl6aVGTcuo1PsoVb4CZBhiHQk=; b=EiYXbTmV8qlIVyoaI0uana+d9RJrJOqu2aKgzVGf8Htw9unjbrXSWH+ZVjraf7cC1w nd8wSHM+BUSkJh1X8jdApXFSv5MZOElkCETC1nMAHSEw9nZ2ncaNdBfIdf/CNYFyEuPC Qjh765KHUmZ+SmZEqF/6q6rak3c9cxAD8UgWBk8ZGNygXre075VlR93BWjai1MDHkydr 5pMr+NzSpljr9WUjf/wxBpUGh12YT4ZPF5hHf8S0OC86kXMAiU5Gig72QeeCOC4AS+3L HUHxIht8PNdxqAG4vPO3MeVYtTOS7FDn9vCp+FEQPJNMpPh+nsct799rG9UfF7uaROmN IVKg== X-Gm-Message-State: AE9vXwPAVpn/cPFeTDuq9raaOUmScnduRtpyac9ItjpLnWTCcb9xXc0MkFPIWcQSCbb46OI9 X-Received: by 10.194.52.3 with SMTP id p3mr8448429wjo.55.1473953313757; Thu, 15 Sep 2016 08:28:33 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id n7sm3961970wjz.32.2016.09.15.08.28.32 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 15 Sep 2016 08:28:33 -0700 (PDT) Date: Thu, 15 Sep 2016 16:28:31 +0100 From: Leif Lindholm To: Ard Biesheuvel Cc: edk2-devel@lists.01.org, liming.gao@intel.com, star.zeng@intel.com, feng.tian@intel.com, ruiyu.ni@intel.com, afish@apple.com, michael.d.kinney@intel.com, lersek@redhat.com, jiewen.yao@intel.com Message-ID: <20160915152831.GH16080@bivouac.eciton.net> References: <1473951296-19120-1-git-send-email-ard.biesheuvel@linaro.org> MIME-Version: 1.0 In-Reply-To: <1473951296-19120-1-git-send-email-ard.biesheuvel@linaro.org> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [PATCH] MdeModulePkg/PciBusDxe: make OPROM BAR degradation X64 specific 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: Thu, 15 Sep 2016 15:28:35 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Thu, Sep 15, 2016 at 03:54:56PM +0100, Ard Biesheuvel wrote: > The 'universal' PCI bus driver in MdeModulePkg contains a quirk to > degrade 64-bit PCI MMIO BARs to 32-bit in the presence of an option > ROM on the same PCI controller. > > This quirk is highly specific to not just the X64 architecture in general, > but to the PC platform in particular, given that only X64 platforms that > require legacy PC BIOS compatibility require it. However, making the > quirk dependent on the presence of the legacy BIOS protocol met with > resistance, due to the fact that it introduces a dependency on the > IntelFrameworkModulePkg package. > > So instead, make the quirk dependent on whether we are compiling for the > X64 architecture, by putting the #ifdef MDE_CPU_X64/#endif preprocessor > directives around it. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel I'm happy with the change, but it may be worth putting a comment in the code to the extent that: --- In order to reliably support legacy Option ROMs, which may not be 64-bit safe, X64 needs to ensure PCI MMIO BARs are forced down to use only the lower 32 bits. Other architectures, or indeed X64 without legacy option ROM support, can safely leave these in normal operation mode (and may require so in order to function). --- For what it's worth: Reviewed-by: Leif Lindholm > --- > > Since nobody proposed any alternatives to the solution I proposed, > let's just make the quirk disappear on architectures that have no > use for it. > > Note that forcing non-X64 architectures to install a driver that > produces the IncompatiblePciDevice protocol only to make the PCI > bus driver behave normally (and which, incidentally, can only be > produced once per platform, making it difficult to provide a default > implementation that can be widely reused) is not acceptable IMO. > > If anything, this should require an IncompatiblePlatform protocol > that informs the PCI bus driver that a special quirk is required, > but I am aware that this is difficult to accomplish without breaking > backward compatibility. Hence this compromise. > > MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c > index b0632d53b82b..b4771a822d65 100644 > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c > @@ -1052,6 +1052,7 @@ DegradeResource ( > IN PCI_RESOURCE_NODE *PMem64Node > ) > { > +#ifdef MDE_CPU_X64 > PCI_IO_DEVICE *PciIoDevice; > LIST_ENTRY *ChildDeviceLink; > LIST_ENTRY *ChildNodeLink; > @@ -1101,6 +1102,7 @@ DegradeResource ( > } > ChildDeviceLink = ChildDeviceLink->ForwardLink; > } > +#endif > > // > // If firmware is in 32-bit mode, > -- > 2.7.4 >