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

["Wu, Hao A" <hao.a.wu@intel.com>]

> -----Original Message-----
> From: Luo, Heng <heng.luo@intel.com>
> Sent: Wednesday, January 13, 2021 2:15 PM
> To: Ni, Ray <ray.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Laszlo
> Ersek <lersek@redhat.com>; devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: RE: [edk2-devel] [Patch V3 2/2] MdeModulePkg/Bus/Pci/PciBusDxe:
> Support PCIe Resizable BAR Capability
> 
> Hi Hao,
> Please hold on merging patch now, we are still waiting for some inputs, I will
> let you know when we reach agreement.


Got it. Thanks Heng.

Best Regards,
Hao Wu


> 
> Thanks,
> Heng
> 
> > -----Original Message-----
> > From: Ni, Ray <ray.ni@intel.com>
> > Sent: Wednesday, January 13, 2021 2:07 PM
> > To: Wu, Hao A <hao.a.wu@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> > devel@edk2.groups.io; Luo, Heng <heng.luo@intel.com>
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>
> > Subject: RE: [edk2-devel] [Patch V3 2/2]
> MdeModulePkg/Bus/Pci/PciBusDxe:
> > Support PCIe Resizable BAR Capability
> >
> > I've given R-b to the two patches. No comments from my side.
> >
> > > -----Original Message-----
> > > From: Wu, Hao A <hao.a.wu@intel.com>
> > > Sent: Wednesday, January 13, 2021 2:00 PM
> > > To: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> > > devel@edk2.groups.io; Luo, Heng <heng.luo@intel.com>
> > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>
> > > Subject: RE: [edk2-devel] [Patch V3 2/2]
> MdeModulePkg/Bus/Pci/PciBusDxe:
> > > Support PCIe Resizable BAR Capability
> > >
> > > > -----Original Message-----
> > > > From: Ni, Ray <ray.ni@intel.com>
> > > > Sent: Tuesday, January 12, 2021 10:28 AM
> > > > To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Luo,
> > > > Heng <heng.luo@intel.com>
> > > > Cc: Wu, Hao A <hao.a.wu@intel.com>; Kinney, Michael D
> > > > <michael.d.kinney@intel.com>
> > > > Subject: RE: [edk2-devel] [Patch V3 2/2]
> > MdeModulePkg/Bus/Pci/PciBusDxe:
> > > > Support PCIe Resizable BAR Capability
> > > >
> > > > > > 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.
> > > >
> > > > > > 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/
> > > > > > th
> > > > > > read
> > > > > > .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.)
> > > > >
> > > >
> > > >
> > > >
> > > > Your understanding is correct. Original idea is to let platform
> > > > supply the
> > > policy
> > > > about what the optimal BAR size is for each resizable BAR.
> > > > The current implementation is a try to avoid asking platform code
> > > > for such policy because we thought it's a burden for platform to
> > > > supply
> > the policy data.
> > > >
> > > > I agree that we set the PCD default value as disabled and after a
> > > > period of study, we will understand whether a platform policy is
> > > > really
> > needed.
> > >
> > >
> > > Hello Laszlo and Ray,
> > >
> > > I saw Heng's patch series to
> > >   1) Set the PCD default value to FALSE:
> > > https://edk2.groups.io/g/devel/message/70139
> > >   2) Update the UNI file:
> > > https://edk2.groups.io/g/devel/message/70140
> > > has got Reviewed-by/Acked-by tags from reviewers.
> > >
> > > Do you have further comments for the series?
> > > If not, I will merge this change in the next 24 hours.
> > >
> > > Thanks in advance.
> > >
> > > Best Regards,
> > > Hao Wu
> > >
> > >
> > > >
> > > > Thanks,
> > > > Ray