From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web10.1958.1610393890057022898 for ; Mon, 11 Jan 2021 11:38:10 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=EayHhV04; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1610393889; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=8kX9jbr05C5DsV9KK3++TwNwnCbqi6Z8bCxYqpLDfm0=; b=EayHhV04YIxpmrBl+0uHoYjr5k2noYfQtBKjS5gH82VeUeWEuq3hXQpg3vbfGEgvdSYv6H un2yzcqvUJHt+PW+XvIasIxM9/mTyiesZ2iiFjWLFAXb3q38n7BrQcJgixl8mEh/LsX4tj xJ8pmQlW54o/NpeCwrZ/8SGHB6tcqVQ= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-195-I9PurT0wP4aMbOoy6-p-aw-1; Mon, 11 Jan 2021 14:38:04 -0500 X-MC-Unique: I9PurT0wP4aMbOoy6-p-aw-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 3030215720; Mon, 11 Jan 2021 19:38:03 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-114.ams2.redhat.com [10.36.112.114]) by smtp.corp.redhat.com (Postfix) with ESMTP id 04D6C1A266; Mon, 11 Jan 2021 19:38:01 +0000 (UTC) Subject: Re: [edk2-devel] [Patch V3 2/2] MdeModulePkg/Bus/Pci/PciBusDxe: Support PCIe Resizable BAR Capability To: devel@edk2.groups.io, heng.luo@intel.com Cc: Ray Ni , Hao A Wu References: <20210104065954.3901-1-heng.luo@intel.com> <20210104065954.3901-2-heng.luo@intel.com> From: "Laszlo Ersek" Message-ID: <9a081622-94c8-d921-1631-b595f189c807@redhat.com> Date: Mon, 11 Jan 2021 20:38:01 +0100 MIME-Version: 1.0 In-Reply-To: <20210104065954.3901-2-heng.luo@intel.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hi, On 01/04/21 07:59, Heng Luo wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=313 > > Add PcdPcieResizableBarSupport to enable/disable PCIe Resizable > BAR Capability fearture. > Program the Resizable BAR Register if the device suports PCIe > Resizable BAR Capability and PcdPcieResizableBarSupport is TRUE. > > Cc: Ray Ni > Cc: Hao A Wu > Signed-off-by: Heng Luo > --- > MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h | 4 +++- > MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf | 3 ++- > MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 27 ++++++++++++++++++++++++++- > MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.h | 12 +++++++++++- > MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c | 185 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------- > MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.h | 22 +++++++++++++++++++++- > MdeModulePkg/MdeModulePkg.dec | 8 +++++++- > 7 files changed, 241 insertions(+), 20 deletions(-) I have brought this feature to the attention of our PCI experts. First of all, let me say that the bugzilla ticket (3138), and the commit message on this patch, are utterly useless. (To add insult to injury, the bugzilla reference in the commit message even has a typo -- it misses the last digit "8".) So, given the lack of information in the bugzilla and the commit message, I could only attempt to decipher the goal / use case myself. This was my interpretation: > It seems like the max BAR size is selected first, but if there's a > "resource conflict" (running out of a particular resource type > aperture), then the minimum BAR size is selected. I don't know what > set of devices and/or resizable BARs this logic applies to, if there > are multiple of them. *IF* my interpretation of the code is correct -- which is admittedly a "big if" --, then the logic seems misguided. This is the feedback I got internally: > Per the PCIe specification (revision 5.0, version 0.9) 7.8.6: > > Software determines, through a proprietary mechanism, what the > optimal size is for the resource, and programs that size via the BAR > Size field of the Resizable BAR Control register. > > Furthermore, Table 7-114 defines the Bar Size field of the control > register stating: > > The default value of this field is equal to the default size of the > address space that the BAR resource is requesting via the BAR's > read-only bits. > > Therefore the maximum size is not necessarily optimal, nor should the > minimum size be considered the default. In fact, [we] tested various > handoff BAR sizes for [a particular] GPU and found that Windows didn't > like the maximum BAR size. > > Elsewhere in the discussion [1] the AMD author of the kernel support > for resizeable BARs indicates that FPGA devices might implement the > REBAR capability as part of their standard PCI wrapper ([our] > interpretation), but the BAR usage would be determined by the actual > bitstream written to the device, therefore there might be a full > bitmask for the BAR sizes supported by the device. > > [1] https://lists.freedesktop.org/archives/dri-devel/2021-January/thread.html > > It would certainly make sense for the firmware to take REBAR > capabilities into account when sizing bridge apertures, but to > generically enable extended BAR sizes would make lots of assumptions > about the device usage and compatibility. > > [...] At least for GPUs the expectation would be a default, smaller > compatibility size expanding to some representation that allows direct > DMA to the entire memory of the card. So this patch should either be reverted; or minimally, the default value of "PcdPcieResizableBarSupport" should be set to FALSE, as the policy for BAR sizing doesn't look robust or portable. General request for the future: if you implement some kind of policy in core edk2, please at least *document* the policy somewhere. It's unacceptable to have to decipher the source code for such a possibly impactful change in the core. There is no need for a wiki page or an RFC, but a sane bugzilla ticket and a sane commit message are required. (The documentation of the PCD in the "MdeModulePkg.dec" file is unsatisfactory too, and the UNI file has not been updated at all.) Thanks Laszlo