Re: [edk2-devel] [Patch V3 2/2] MdeModulePkg/Bus/Pci/PciBusDxe: Support PCIe Resizable BAR Capability

["Laszlo Ersek" <lersek@redhat.com>]

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 <ray.ni@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Signed-off-by: Heng Luo <heng.luo@intel.com>
> ---
>  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