Re: [PATCH] vpci: Add resizable bar support

["Chen, Jiqian" <Jiqian.Chen@xxxxxxx>]

On 2024/11/14 23:52, Roger Pau Monné wrote:
> On Thu, Nov 14, 2024 at 06:11:46AM +0000, Chen, Jiqian wrote:
>> On 2024/11/13 18:30, Roger Pau Monné wrote:
>>> On Wed, Nov 13, 2024 at 10:00:33AM +0000, Chen, Jiqian wrote:
>>>> On 2024/11/13 17:30, Roger Pau Monné wrote:
>>>>> On Wed, Nov 13, 2024 at 04:00:27PM +0800, Jiqian Chen wrote:
>>>>>> Some devices, like discrete GPU of amd, support resizable bar capability,
>>>>>> but vpci of Xen doesn't support this feature, so they fail to resize bars
>>>>>> and then cause probing failure.
>>>>>>
>>>>>> According to PCIe spec, each bar that support resizing has two registers,
>>>>>> PCI_REBAR_CAP and PCI_REBAR_CTRL, so add these two registers and their
>>>>>> corresponding handler into vpci.
>>>>>>
>>>>>> PCI_REBAR_CAP is RO, only provide reading.
>>>>>>
>>>>>> PCI_REBAR_CTRL only has bar size is RW, so add write function to support
>>>>>> setting the new size.
>>>>>
>>>>> I think the logic to handle resizable BAR could be much simpler.  Some
>>>>> time ago I've made a patch to add support for it, but due to lack of
>>>>> hardware on my side to test it I've never submitted it.
>>>>>
>>>>> My approach would be to detect the presence of the
>>>>> PCI_EXT_CAP_ID_REBAR capability in init_header(), and if the
>>>>> capability is present force the sizing of BARs each time they are
>>>>> mapped in modify_bars().  I don't think we need to trap accesses to
>>>>> the capability itself, as resizing can only happen when memory
>>>>> decoding is not enabled for the device.  It's enough to fetch the size
>>>>> of the BARs ahead of each enabling of memory decoding.
>>>>>
>>>>> Note that memory decoding implies mapping the BARs into the p2m, which
>>>>> is already an expensive operation, the extra sizing is unlikely to
>>>>> make much of a difference performance wise.
>>>>>
>>>>> I've found the following on my git tree and rebased on top of staging:
>>>> OK.
>>>> Do you need me to validate your patch in my environment?
>>>
>>> Yes please, I have no way to test it.  Let's see what others think
>>> about the different approaches.
>> There are some errors with your method.
>> I attached the dmesg and xl dmesg logs.
>> From the dmesg logs, it seems that 0000:03:00.0 has addresse overlap with 
>> 0000:03:00.1
> 
> Do you have the output of lspci with the BAR sizes/positions before
> and after the resizing?
I will use your new patch to get these information.

> 
>>
>> I think there is a place that needs to be modified regarding your method,
>> although this modification does not help with the above-mentioned errors,
>> it is that whether to support resizing is specific to which bar, rather than 
>> just determining whether there is a Rebar capability.
> 
> Do we really need such fine-grained information?  It should be
> harmless (even if not strictly necessary) to size all BARs on the
> device before enabling memory decoding, even if some of them do not
> support resizing.
OK, I just think it can help to reduce some unnecessary calls(actions).

> 
> I might have to provide a patch with additional messages to see what's
> going on.
> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.