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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Feb 11, 2025 at 10:22:57AM +0800, Jiqian Chen wrote:
> Some devices, like AMDGPU, 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 supports resizing has
> two registers, PCI_REBAR_CAP and PCI_REBAR_CTRL. So, add
> handlers to support resizing the size of BARs.
> 
> Note that Xen will only trap PCI_REBAR_CTRL, as PCI_REBAR_CAP
> is read-only register and the hardware domain already gets
> access to it without needing any setup.
> 
> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

> ---
> Hi all,
> v7->v8 changes:
> * Modified commit message and some comments.
> * Deleted unused function vpci_hw_write32.
> 
> Best regards,
> Jiqian Chen.
> 
> v6->v7 changes:
> * Deleted codes that add register for PCI_REBAR_CAP, and added comments to 
> explain why.
> * Added comments to explain why use "continue" when fail to add register for 
> PCI_REBAR_CTRL.
> 
> v5->v6 changes:
> * Changed "1UL" to "1ULL" in PCI_REBAR_CTRL_SIZE idefinition for 32 bit 
> architecture.
> * In rebar_ctrl_write used "bar - pdev->vpci->header.bars" to get index 
> instead of reading
>   from register.
> * Added the index of BAR to error messages.
> * Changed to "continue" instead of "return an error" when vpci_add_register 
> failed.
> 
> v4->v5 changes:
> * Called pci_size_mem_bar in rebar_ctrl_write to get addr and size of BAR 
> instead of setting
>   their values directly after writing new size to hardware.
> * Changed from "return" to "continue" when index/type of BAR are not correct 
> during initializing
>   BAR.
> * Corrected the value of PCI_REBAR_CTRL_BAR_SIZE from "0x00001F00" to 
> "0x00003F00".
> * Renamed PCI_REBAR_SIZE_BIAS to PCI_REBAR_CTRL_SIZE_BIAS.
> * Re-defined "PCI_REBAR_CAP_SHIFT 4" to "PCI_REBAR_CAP_SIZES_MASK 
> 0xFFFFFFF0U".
> 
> v3->v4 changes:
> * Removed PCI_REBAR_CAP_SIZES since it was not needed, and added
>   PCI_REBAR_CAP_SHIFT and PCI_REBAR_CTRL_SIZES.
> * Added parameter resizable_sizes to struct vpci_bar to cache the support 
> resizable sizes and
>   added the logic in init_rebar().
> * Changed PCI_REBAR_CAP to PCI_REBAR_CAP(n) (4+8*(n)), changed PCI_REBAR_CTRL 
> to
>   PCI_REBAR_CTRL(n) (8+8*(n)).
> * Added domain info of pci_dev to printings of init_rebar().
> 
> v2->v3 changes:
> * Used "bar->enabled" to replace "pci_conf_read16(pdev->sbdf, PCI_COMMAND) & 
> PCI_COMMAND_MEMORY",
>   and added comments why it needs this check.
> * Added "!is_hardware_domain(pdev->domain)" check in init_rebar() to return 
> EOPNOTSUPP for domUs.
> * Moved BAR type and index check into init_rebar(), then only need to check 
> once.
> * Added 'U' suffix for macro PCI_REBAR_CAP_SIZES.
> * Added macro PCI_REBAR_SIZE_BIAS to represent 20.
> TODO: need to hide ReBar capability from hardware domain when init_rebar() 
> fails.
> 
> v1->v2 changes:
> * In rebar_ctrl_write, to check if memory decoding is enabled, and added
>   some checks for the type of Bar.
> * Added vpci_hw_write32 to handle PCI_REBAR_CAP's write, since there is
>   no write limitation of dom0.
> * And has many other minor modifications as well.
> ---
>  xen/drivers/vpci/Makefile  |   2 +-
>  xen/drivers/vpci/rebar.c   | 131 +++++++++++++++++++++++++++++++++++++
>  xen/include/xen/pci_regs.h |  15 +++++
>  xen/include/xen/vpci.h     |   1 +
>  4 files changed, 148 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..794f1168adf8
> --- /dev/null
> +++ b/xen/drivers/vpci/rebar.c
> @@ -0,0 +1,131 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2025 Advanced Micro Devices, Inc. All Rights Reserved.
> + *
> + * 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)
> +{
> +    struct vpci_bar *bar = data;
> +    const unsigned int index = bar - pdev->vpci->header.bars;
> +    uint64_t size = PCI_REBAR_CTRL_SIZE(val);

Since you define index as const you could also do the same with size.
Can adjust at commit, but I also don't have a strong opinion about
it.

Thanks, Roger.