Re: [PATCH] vpci: Add resizable bar support

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

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?
And I have one question: where does your patch do writing the resizing size 
into hardware?

> 
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index ef6c965c081c..045aa4bdadc8 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -356,6 +356,30 @@ static int modify_bars(const struct pci_dev *pdev, 
> uint16_t cmd, bool rom_only)
>  
>          ASSERT(rangeset_is_empty(bar->mem));
>  
> +        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);
> +            }
> +        }
> +
>          /*
>           * Make sure that the guest set address has the same page offset
>           * as the physical address on the host or otherwise things won't 
> work as
> @@ -870,6 +894,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.