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

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

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