Re: [PATCH] vpci: Add resizable bar support

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

On 2024/11/18 18:17, 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.
>>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
>> ---
>>  xen/drivers/vpci/Makefile  |  2 +-
>>  xen/drivers/vpci/rebar.c   | 89 ++++++++++++++++++++++++++++++++++++++
>>  xen/include/xen/pci_regs.h | 11 +++++
>>  3 files changed, 101 insertions(+), 1 deletion(-)
>>  create mode 100644 xen/drivers/vpci/rebar.c
>>
>> diff --git a/xen/drivers/vpci/Makefile b/xen/drivers/vpci/Makefile
>> index 1a1413b93e76..a7c8a30a8956 100644
>> --- a/xen/drivers/vpci/Makefile
>> +++ b/xen/drivers/vpci/Makefile
>> @@ -1,2 +1,2 @@
>> -obj-y += vpci.o header.o
>> +obj-y += vpci.o header.o rebar.o
>>  obj-$(CONFIG_HAS_PCI_MSI) += msi.o msix.o
>> diff --git a/xen/drivers/vpci/rebar.c b/xen/drivers/vpci/rebar.c
>> new file mode 100644
>> index 000000000000..84dbd84b0745
>> --- /dev/null
>> +++ b/xen/drivers/vpci/rebar.c
>> @@ -0,0 +1,89 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
> 
> The GPL-2.0 identifier is deprecated, either use GPL-2.0-or-later or
> GPL-2.0-only.
> 
>> +/*
>> + * Copyright (C) 2024 Advanced Micro Devices, Inc. All Rights Reserved.
>> + *
>> + * Author: Jiqian Chen <Jiqian.Chen@xxxxxxx>
>> + */
>> +
>> +#include <xen/hypercall.h>
>> +#include <xen/vpci.h>
>> +
>> +/*
>> + * The number value of the BAR Size in PCI_REBAR_CTRL register reprent:
>> + * 0    1 MB (2^20 bytes)
>> + * 1    2 MB (2^21 bytes)
>> + * 2    4 MB (2^22 bytes)
>> + *  ...
>> + * 43   8 EB (2^63 bytes)
>> + */
>> +#define PCI_REBAR_CTRL_BAR_UNIT (1ULL << 20)
>> +
>> +static void cf_check rebar_ctrl_write(const struct pci_dev *pdev,
>> +                                      unsigned int reg,
>> +                                      uint32_t val,
>> +                                      void *data)
>> +{
>> +    uint32_t ctrl, index;
> 
> index should better be unsigned int, as it's the BAR index [0, 5], and
> so fits perfectly in an unsigned int.
> 
>> +    struct vpci_bar *bars = pdev->vpci->header.bars;
> 
> You could pass bars as the data parameter.
> 
> Additionally you need to check that memory decoding is not enabled for
> the device, otherwise attempting to change the BAR size will lead to
> the active p2m mappings getting out of sync w.r.t. the new BAR size.
> 
>> +
>> +    ctrl = pci_conf_read32(pdev->sbdf, reg);
>> +    if ( ctrl == val )
>> +        return;
> 
> I would still carry out the write in this case, as that's what the
> owner of the device requested.
> 
>> +
>> +    ctrl &= ~PCI_REBAR_CTRL_BAR_SIZE;
>> +    if ( ctrl != ( val & ~PCI_REBAR_CTRL_BAR_SIZE ) )
>                      ^ extra space here and         ^ here
>> +        return;
> 
> The feature only being exposed to dom0 ATM, I would suggest we allow
> it to write any bits it wants in the control register, as that would
> be what the OS would do when not running as a guest.
But only PCI_REBAR_CTRL_BAR_SIZE bits of REBAR_CTRL register are RW, others are 
RO.
Is removing the check here fine?

> 
>> +
>> +    index = ctrl & PCI_REBAR_CTRL_BAR_IDX;
> 
> Some sanity checking of the BAR index might be good.  At least a check
> that the BAR is of type VPCI_BAR_MEM64_LO or VPCI_BAR_MEM32.
But the information of the BAR that support resizing is from hardware(when 
init_rebar), that shouldn't have any problems and doesn't need to check the 
BAR's info.

> 
>> +    bars[index].size = (1 << ((val & PCI_REBAR_CTRL_BAR_SIZE) >>
>> +                              PCI_REBAR_CTRL_BAR_SHIFT)) *
>> +                       PCI_REBAR_CTRL_BAR_UNIT;
> 
> This would better be a macro in pci_regs.h I think, and you need to
> use 1UL, plus using MASK_EXTR() simplifies it:
> 
> PCI_REBAR_CTRL_SIZE(v) (1UL << (MASK_EXTR(v, PCI_REBAR_CTRL_BAR_SIZE) + 20))
OK, another question: Should I need to check the size is allowed by 
hardware(the PCI_REBAR_CAP_SIZES bits in PCI_REBAR_CAP)?

> 
>> +
>> +    pci_conf_write32(pdev->sbdf, reg, val);
>> +}
>> +
>> +static int cf_check init_rebar(struct pci_dev *pdev)
>> +{
>> +    unsigned int rebar_offset;
>> +    uint32_t ctrl, nbars;
> 
> nbars can be unsigned int.
> 
>> +    int rc = 0;
>> +
>> +    rebar_offset = pci_find_ext_capability(pdev->sbdf, 
>> PCI_EXT_CAP_ID_REBAR);
>> +
>> +    if ( !rebar_offset )
>> +        return rc;
> 
> Just return 0, it's clearer than having to figure out what rc is set
> to.
> 
>> +
>> +    ctrl = pci_conf_read32(pdev->sbdf, rebar_offset + PCI_REBAR_CTRL);
>> +    nbars = (ctrl & PCI_REBAR_CTRL_NBAR_MASK) >> PCI_REBAR_CTRL_NBAR_SHIFT;
> 
> MASK_EXTR(ctrl, PCI_REBAR_CTRL_NBAR_MASK).
> 
>> +
>> +    for ( int i = 0; i < nbars; i++, rebar_offset += 8 ) {
> 
> unsigned int, and defined outside of the loop.  Also coding style:
> braces need to be on a newline.
> 
> You could even reduce the scope of rc by defining it inside the
> loop.
> 
>> +        rc = vpci_add_register(pdev->vpci, vpci_hw_read32, NULL,
>> +                               rebar_offset + PCI_REBAR_CAP, 4, NULL);
> 
> I'm not sure we want to limit dom0 writes to the capabilities
> registers, as said above dom0 gets unfiltered access to almost all
> registers, so it can do what it would otherwise do when running on
> native hardware.
You mean, this register(PCI_REBAR_CAP) is not needed to be added?

> 
>> +        if ( rc ) {
>> +            printk("%s: %pp: add register for PCI_REBAR_CAP failed 
>> (rc=%d)\n",
>> +                   __func__, &pdev->sbdf, rc);
>> +            break;
>> +        }
>> +
>> +        rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rebar_ctrl_write,
>> +                               rebar_offset + PCI_REBAR_CTRL, 4, NULL);
>> +        if ( rc ) {
>> +            printk("%s: %pp: add register for PCI_REBAR_CTRL failed 
>> (rc=%d)\n",
>> +                   __func__, &pdev->sbdf, rc);
> 
> IMO I think you can forego printing __func__, and just use:
> 
> "%pp: add register for PCI_REBAR_CTRL failed: %d\n"
> 
>> +            break;
>> +        }
>> +    }
>> +
>> +    return rc;
>> +}
>> +REGISTER_VPCI_INIT(init_rebar, VPCI_PRIORITY_LOW);
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * tab-width: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/include/xen/pci_regs.h b/xen/include/xen/pci_regs.h
>> index 250ba106dbd3..5d2aa130916e 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      /* Resizable BAR */
>>  
>>  /* Advanced Error Reporting */
>>  #define PCI_ERR_UNCOR_STATUS        4       /* Uncorrectable Error Status */
>> @@ -541,6 +542,16 @@
>>  #define  PCI_VNDR_HEADER_REV(x)     (((x) >> 16) & 0xf)
>>  #define  PCI_VNDR_HEADER_LEN(x)     (((x) >> 20) & 0xfff)
>>  
>> +/* Resizable BARs */
>> +#define PCI_REBAR_CAP               4       /* capability register */
>> +#define  PCI_REBAR_CAP_SIZES                0x00FFFFF0  /* supported BAR 
>> sizes */
>> +#define PCI_REBAR_CTRL              8       /* control register */
>> +#define  PCI_REBAR_CTRL_BAR_IDX             0x00000007  /* BAR index */
>> +#define  PCI_REBAR_CTRL_NBAR_MASK   0x000000E0  /* # of resizable BARs */
>> +#define  PCI_REBAR_CTRL_NBAR_SHIFT  5           /* shift for # of BARs */
>> +#define  PCI_REBAR_CTRL_BAR_SIZE    0x00001F00  /* BAR size */
>> +#define  PCI_REBAR_CTRL_BAR_SHIFT   8           /* shift for BAR size */
> 
> If you use MASK_EXTR() there's no need for the _SHIFT macros I think?
Yes, and will change patch according to your all comments in next version.
Thanks!

> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.