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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> + * 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.)

> --- 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).

Jan