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.c...@amd.com>
> + */
> +
> +#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
