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

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

Sure, I will create PR to merge the series.

Best Regards,
Hao Wu

> -----Original Message-----
> From: Luo, Heng <heng.luo@intel.com>
> Sent: Thursday, January 14, 2021 8:45 AM
> To: devel@edk2.groups.io; lersek@redhat.com; Ni, Ray <ray.ni@intel.com>;
> Wu, Hao A <hao.a.wu@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
> 
> Hi Laszlo,
> Sorry for the delay. I have gotten the feedback, we can merged them now.
> 
> Hi Hao,
> Could you help to merge the patches.
> 
> Thanks,
> Heng
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
> > Ersek
> > Sent: Wednesday, January 13, 2021 5:09 PM
> > To: Luo, Heng <heng.luo@intel.com>; Ni, Ray <ray.ni@intel.com>; Wu,
> > Hao A <hao.a.wu@intel.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
> >
> > On 01/13/21 07:15, Luo, Heng wrote:
> > > Hi Hao,
> > > Please hold on merging patch now, we are still waiting for some
> > > inputs, I
> > will let you know when we reach agreement.
> >
> > I disagree with waiting. The original patch caused a regression. The
> > currently pending patch fixes the regression. Any further input
> > ("agreement") should be processed *after* we have mitigated the
> regression.
> >
> > The tree is currently in a wrong state. The fix has been reviewed.
> > Hao, please proceed with merging the fix as soon as you can.
> >
> > Thanks
> > Laszlo
> >
> > >
> > > 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/t
> > >>>>>> h
> > >>>>>> 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
> >
> >
> >
> > 
> >