Re: [PATCH] vpci: Add resizable bar support

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

On 2024/11/15 01:36, Roger Pau Monné wrote:
> On Thu, Nov 14, 2024 at 04:52:18PM +0100, 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 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.
>>
>> I might have to provide a patch with additional messages to see what's
>> going on.
> 
> One nit that I've noticed with the patch I gave you previously is that
> the check for a size change in modify_bars() should be done ahead of
> pci_check_bar(), otherwise the check is possibly using an outdated
> size.
> 
> I've also added a debug message to notify when a BAR register is
> written and report the new address.  This is done unconditionally, but
> if you think it's too chatty you can limit to only printing for the
> device that has the ReBAR capability.
Errors are the same.
Attached the dmesg, xl dmesg, patch and lspci output.
I will also continue to debug your method on my side to try to get some 
findings.

> 
> Thanks, Roger.
> 
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index ef6c965c081c..d49d3588993b 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -346,6 +346,30 @@ static int modify_bars(const struct pci_dev *pdev, 
> uint16_t cmd, bool rom_only)
>               bar->enabled == !!(cmd & PCI_COMMAND_MEMORY) )
>              continue;
>  
> +        if ( bar->type != VPCI_BAR_ROM && header->bars_resizable &&
> +             (cmd & PCI_COMMAND_MEMORY) )
> +        {
> +            uint64_t addr, size;
> +
> +            pci_size_mem_bar(pdev->sbdf, PCI_BASE_ADDRESS_0 + i * 4,
> +                             &addr, &size, 0);
> +
> +            if ( bar->addr != addr )
> +                printk(XENLOG_G_ERR
> +                       "%pp: BAR#%u address mismatch %#lx vs %#lx\n",
> +                       &pdev->sbdf, i, bar->addr, addr);
> +
> +            if ( bar->size != size )
> +            {
> +                printk(XENLOG_G_DEBUG
> +                       "%pp: detected BAR#%u size change (%#lx -> %#lx)\n",
> +                       &pdev->sbdf, i, bar->size, size);
> +                bar->size = size;
> +                end = PFN_DOWN(bar->addr + size - 1);
> +                end_guest = PFN_DOWN(bar->guest_addr + size - 1);
> +            }
> +        }
> +
>          if ( !pci_check_bar(pdev, _mfn(start), _mfn(end)) )
>          {
>              printk(XENLOG_G_WARNING
> @@ -601,6 +625,9 @@ static void cf_check bar_write(
>      /* Update guest address, so hardware domain BAR is identity mapped. */
>      bar->guest_addr = bar->addr;
>  
> +gprintk(XENLOG_DEBUG, "%pp: updated BAR%zu address: %#lx\n",
> +                    &pdev->sbdf, bar - pdev->vpci->header.bars + hi, 
> bar->addr);
> +
>      /* Make sure Xen writes back the same value for the BAR RO bits. */
>      if ( !hi )
>      {
> @@ -870,6 +897,9 @@ static int cf_check init_header(struct pci_dev *pdev)
>      if ( pdev->ignore_bars )
>          return 0;
>  
> +    header->bars_resizable = pci_find_ext_capability(pdev->sbdf,
> +                                                     PCI_EXT_CAP_ID_REBAR);
> +
>      cmd = pci_conf_read16(pdev->sbdf, PCI_COMMAND);
>  
>      /*
> diff --git a/xen/include/xen/pci_regs.h b/xen/include/xen/pci_regs.h
> index 250ba106dbd3..c543a2b86778 100644
> --- a/xen/include/xen/pci_regs.h
> +++ b/xen/include/xen/pci_regs.h
> @@ -459,6 +459,7 @@
>  #define PCI_EXT_CAP_ID_ARI   14
>  #define PCI_EXT_CAP_ID_ATS   15
>  #define PCI_EXT_CAP_ID_SRIOV 16
> +#define PCI_EXT_CAP_ID_REBAR 21
>  
>  /* Advanced Error Reporting */
>  #define PCI_ERR_UNCOR_STATUS 4       /* Uncorrectable Error Status */
> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
> index 41e7c3bc2791..45ebc1bb3356 100644
> --- a/xen/include/xen/vpci.h
> +++ b/xen/include/xen/vpci.h
> @@ -129,6 +129,8 @@ struct vpci {
>           * upon to know whether BARs are mapped into the guest p2m.
>           */
>          bool bars_mapped      : 1;
> +        /* Device has the Resizable BARs capability. */
> +        bool bars_resizable   : 1;
>          /* FIXME: currently there's no support for SR-IOV. */
>      } header;
>  
> 

-- 
Best regards,
Jiqian Chen.

Attachment: 0001-Rebar-debug-of-Roger.patch
Description: 0001-Rebar-debug-of-Roger.patch

Attachment: rebar_dmesg.txt
Description: rebar_dmesg.txt

Attachment: rebar_lspci.txt
Description: rebar_lspci.txt

Attachment: rebar_xl_dmesg.txt
Description: rebar_xl_dmesg.txt