Re: [PATCH v5] vpci: Add resizable bar support

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

On 2025/1/20 23:35, Jan Beulich wrote:
> On 14.01.2025 04:26, Jiqian Chen wrote:
>> --- /dev/null
>> +++ b/xen/drivers/vpci/rebar.c
>> @@ -0,0 +1,135 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (C) 2024 Advanced Micro Devices, Inc. All Rights Reserved.
> 
> Nit: This has now gone stale.
Thanks, will change 2024 to 2025.

> 
>> + * Author: Jiqian Chen <Jiqian.Chen@xxxxxxx>
>> + */
>> +
>> +#include <xen/sched.h>
>> +#include <xen/vpci.h>
>> +
>> +static void cf_check rebar_ctrl_write(const struct pci_dev *pdev,
>> +                                      unsigned int reg,
>> +                                      uint32_t val,
>> +                                      void *data)
>> +{
>> +    unsigned int index;
>> +    struct vpci_bar *bar = data;
>> +    uint64_t size = PCI_REBAR_CTRL_SIZE(val);
>> +
>> +    if ( bar->enabled )
>> +    {
>> +        /*
>> +         * Refuse to resize a BAR while memory decoding is enabled, as
>> +         * otherwise the size of the mapped region in the p2m would become
>> +         * stale with the newly set BAR size, and the position of the BAR
>> +         * would be reset to undefined.  Note the PCIe specification also
>> +         * forbids resizing a BAR with memory decoding enabled.
>> +         */
>> +        if ( size != bar->size )
>> +            gprintk(XENLOG_ERR,
>> +                    "%pp: refuse to resize BAR with memory decoding 
>> enabled\n",
>> +                    &pdev->sbdf);
>> +        return;
>> +    }
>> +
>> +    if ( !((size >> PCI_REBAR_CTRL_SIZE_BIAS) & bar->resizable_sizes) )
>> +        gprintk(XENLOG_WARNING,
>> +                "%pp: new size %#lx is not supported by hardware\n",
>> +                &pdev->sbdf, size);
>> +
>> +    pci_conf_write32(pdev->sbdf, reg, val);
>> +
>> +    index = pci_conf_read32(pdev->sbdf, reg) & PCI_REBAR_CTRL_BAR_IDX;
>> +    pci_size_mem_bar(pdev->sbdf, PCI_BASE_ADDRESS_0 + index * 4, &bar->addr,
>> +                     &bar->size, ((index == PCI_HEADER_NORMAL_NR_BARS - 1) ?
>> +                                  PCI_BAR_LAST : 0));
> 
> Nit: Imo it's unhelpful to the reader if you put multiple arguments on a 
> single
> line, when the final one then needs wrapping across lines. (Putting multiple
> arguments on a single line is fine of course when they fully fit.)
Will change to put each argument in a single line in next version.

> 
>> --- 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,19 @@
>>  #define  PCI_VNDR_HEADER_REV(x)     (((x) >> 16) & 0xf)
>>  #define  PCI_VNDR_HEADER_LEN(x)     (((x) >> 20) & 0xfff)
>>  
>> +/* Resizable BARs */
>> +#define PCI_REBAR_CAP(n)    (4 + 8 * (n))   /* capability register */
>> +#define  PCI_REBAR_CAP_SIZES_MASK   0xFFFFFFF0U     /* supported BAR sizes 
>> in CAP */
>> +#define PCI_REBAR_CTRL(n)   (8 + 8 * (n))   /* 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_BAR_SIZE    0x00003F00      /* BAR size */
>> +#define  PCI_REBAR_CTRL_SIZES_MASK  0xFFFF0000U     /* supported BAR sizes 
>> in CTRL */
>> +#define  PCI_REBAR_CTRL_SIZE_BIAS   20
>> +#define  PCI_REBAR_CTRL_SIZE(v) \
>> +            (1UL << (MASK_EXTR(v, PCI_REBAR_CTRL_BAR_SIZE) \
>> +                     + PCI_REBAR_CTRL_SIZE_BIAS))
> 
> On x86 (being 64-bit only) and Arm64 1UL may be good enough here, but
> I expect we'll need 1ULL for any 32-bit architecture.
> 
> Plus, as indicated before, these two auxiliary #define-s would imo
> better be separated from those directly pertaining to the control
> register fields (and then also not be padded like those).
Thank you!
Will use a space line to separate these two #define-s from the others and 
change 1UL to 1ULL.

> 
> Jan

-- 
Best regards,
Jiqian Chen.