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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Fri, Dec 13, 2024 at 01:42:32PM +0800, Jiqian Chen wrote:
> Some devices, like discrete GPU of amd, 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 for them to support resizing the size of BARs.
> 
> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
> ---
> Hi all,
> 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.
> 
> Best regards,
> Jiqian Chen.
> 
> 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   | 130 +++++++++++++++++++++++++++++++++++++
>  xen/drivers/vpci/vpci.c    |   6 ++
>  xen/include/xen/pci_regs.h |  13 ++++
>  xen/include/xen/vpci.h     |   2 +
>  5 files changed, 152 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..39432c3271a4
> --- /dev/null
> +++ b/xen/drivers/vpci/rebar.c
> @@ -0,0 +1,130 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2024 Advanced Micro Devices, Inc. All Rights Reserved.
> + *
> + * Author: Jiqian Chen <Jiqian.Chen@amd.com>
> + */
> +
> +#include <xen/hypercall.h>

Do you really need the hypercall header?

> +#include <xen/vpci.h>
> +
> +static void cf_check rebar_ctrl_write(const struct pci_dev *pdev,
> +                                      unsigned int reg,
> +                                      uint32_t val,
> +                                      void *data)
> +{
> +    uint64_t size;
> +    struct vpci_bar *bar = data;
> +
> +    size = PCI_REBAR_CTRL_SIZE(val);
> +    if ( bar->enabled )
> +    {
> +        if ( size == bar->size )
> +            return;
> +
> +        /*

Indentation

> +        * 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.
> +        */
> +        gprintk(XENLOG_ERR,
> +                "%pp: refuse to resize BAR with memory decoding enabled\n",
> +                &pdev->sbdf);
> +        return;
> +    }

Nit: just realized this could be made shorter:

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_SIZE_BIAS) &
> +           MASK_EXTR(pci_conf_read32(pdev->sbdf,
> +                                     reg - PCI_REBAR_CTRL + PCI_REBAR_CAP),
> +                                     PCI_REBAR_CAP_SIZES)) )

Would it be possible to cache this information.  It's my understand
the supported sizes won't change, and hence Xen could read and cache
them in init_rebar() to avoid repeated reads to the register on every
access?

> +        gprintk(XENLOG_WARNING,
> +                "%pp: new size %#lx is not supported by hardware\n",
> +                &pdev->sbdf, size);
> +
> +    bar->size = size;
> +    bar->addr = 0;
> +    bar->guest_addr = 0;
> +    pci_conf_write32(pdev->sbdf, reg, val);
> +}
> +
> +static int cf_check init_rebar(struct pci_dev *pdev)
> +{
> +    uint32_t ctrl;
> +    unsigned int rebar_offset, nbars;
> +
> +    rebar_offset = pci_find_ext_capability(pdev->sbdf, PCI_EXT_CAP_ID_REBAR);

You can do the init at definition:

    uint32_t ctrl;
    unsigned int nbars;
    unsigned int rebar_offset = pci_find_ext_capability(pdev->sbdf,
                                                        PCI_EXT_CAP_ID_REBAR);


> +
> +    if ( !rebar_offset )
> +        return 0;
> +
> +    if ( !is_hardware_domain(pdev->domain) )
> +    {
> +        printk("ReBar is not supported for domUs\n");

This needs a bit more information IMO:

printk(XENLOG_ERR
       "%pd %pp: resizable BAR capability not supported for unprivileged domains\n",
       pdev->domain, &pdev->sbdf);

I wonder if this should instead be an XSM check, but that would
require a new XSM hook to process permissions for PCI capabilities.

> +        return -EOPNOTSUPP;
> +    }
> +
> +    ctrl = pci_conf_read32(pdev->sbdf, rebar_offset + PCI_REBAR_CTRL);
> +    nbars = MASK_EXTR(ctrl, PCI_REBAR_CTRL_NBAR_MASK);
> +
> +    for ( unsigned int i = 0; i < nbars; i++, rebar_offset += PCI_REBAR_CTRL )
> +    {
> +        int rc;
> +        unsigned int index;
> +        struct vpci_bar *bars = pdev->vpci->header.bars;
> +
> +        index = pci_conf_read32(pdev->sbdf, rebar_offset + PCI_REBAR_CTRL) &
> +                PCI_REBAR_CTRL_BAR_IDX;

You could initialize index at definition.

> +
> +        if ( index >= PCI_HEADER_NORMAL_NR_BARS )
> +        {
> +            /*
> +             * TODO: for failed pathes, need to hide ReBar capability
> +             * from hardware domain instead of returning an error.
> +             */
> +            printk("%pp: BAR number %u in REBAR_CTRL register is too big\n",
> +                   &pdev->sdf, index);

XENLOG_ERR, plus we could print the domain the device was assigned to
(pdev->domain).

> +            return -EINVAL;
> +        }
> +
> +        if ( bars[index].type != VPCI_BAR_MEM64_LO &&
> +             bars[index].type != VPCI_BAR_MEM32 )
> +        {
> +            printk("%pp: BAR%u is not in memory space\n", &pdev->sbdf, index);
> +            return -EINVAL;
> +        }
> +
> +        rc = vpci_add_register(pdev->vpci, vpci_hw_read32, vpci_hw_write32,
> +                               rebar_offset + PCI_REBAR_CAP, 4, NULL);
> +        if ( rc )
> +        {
> +            printk("%pp: add register for PCI_REBAR_CAP failed (rc=%d)\n",
> +                   &pdev->sbdf, rc);
> +            return rc;
> +        }
> +
> +        rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rebar_ctrl_write,
> +                               rebar_offset + PCI_REBAR_CTRL, 4,
> +                               &bars[index]);
> +        if ( rc )
> +        {
> +            printk("%pp: add register for PCI_REBAR_CTRL failed %d\n",
> +                   &pdev->sbdf, rc);
> +            return rc;
> +        }

All the log messages above need the XENLOG_ERR prefix, plus possibly
printing the assigned domain.

Thanks, Roger.