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

["Heng Luo" <heng.luo@intel.com>]

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/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
> 
> 
> 
> 
>