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