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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.12.2024 06:21, Jiqian Chen wrote:
> --- /dev/null
> +++ b/xen/drivers/vpci/rebar.c
> @@ -0,0 +1,131 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2024 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;
> +    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_SIZE_BIAS) & bar->resizable_sizes) )
> +        gprintk(XENLOG_WARNING,
> +                "%pp: new size %#lx is not supported by hardware\n",
> +                &pdev->sbdf, size);
> +
> +    bar->size = size;

Shouldn't at least this be in an "else" to the if() above?

> +    bar->addr = 0;

For maximum compatibility with the behavior on bare metal, would we
perhaps better ...

> +    bar->guest_addr = 0;
> +    pci_conf_write32(pdev->sbdf, reg, val);

... re-read the BAR from hardware after this write?

Similar consideration may apply to ->guest_addr: Driver writers knowing
how their hardware behaves may expect that merely some of the bits of
the address get cleared (if the size increases).

> +static int cf_check init_rebar(struct pci_dev *pdev)
> +{
> +    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(XENLOG_ERR "%pp: resizable BARs unsupported for unpriv %pd\n",
> +               &pdev->sbdf, pdev->domain);
> +        return -EOPNOTSUPP;
> +    }
> +
> +    ctrl = pci_conf_read32(pdev->sbdf, rebar_offset + PCI_REBAR_CTRL(0));
> +    nbars = MASK_EXTR(ctrl, PCI_REBAR_CTRL_NBAR_MASK);
> +
> +    for ( unsigned int i = 0; i < nbars; i++ )
> +    {
> +        int rc;
> +        struct vpci_bar *bar;
> +        unsigned int index;
> +
> +        ctrl = pci_conf_read32(pdev->sbdf, rebar_offset + PCI_REBAR_CTRL(i));
> +        index = ctrl & PCI_REBAR_CTRL_BAR_IDX;;

Nit: No double semicolons please.

> +        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(XENLOG_ERR "%pd %pp: too big BAR number %u in 
> REBAR_CTRL\n",
> +                   pdev->domain, &pdev->sbdf, index);
> +            return -EINVAL;

With the TODO unaddressed, is it actually appropriate to return an error
here? Shouldn't we continue in a best effort manner? (Question also to
Roger as the maintainer.)

> +        }
> +
> +        bar = &pdev->vpci->header.bars[index];
> +        if ( bar->type != VPCI_BAR_MEM64_LO && bar->type != VPCI_BAR_MEM32 )
> +        {
> +            printk(XENLOG_ERR "%pd %pp: BAR%u is not in memory space\n",
> +                   pdev->domain, &pdev->sbdf, index);
> +            return -EINVAL;

Same question here then.

> +        }
> +
> +        rc = vpci_add_register(pdev->vpci, vpci_hw_read32, vpci_hw_write32,
> +                               rebar_offset + PCI_REBAR_CAP(i), 4, NULL);
> +        if ( rc )
> +        {
> +            printk(XENLOG_ERR "%pd %pp: fail to add reg of REBAR_CAP 
> rc=%d\n",
> +                   pdev->domain, &pdev->sbdf, rc);
> +            return rc;
> +        }
> +
> +        rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rebar_ctrl_write,
> +                               rebar_offset + PCI_REBAR_CTRL(i), 4, bar);
> +        if ( rc )
> +        {
> +            printk(XENLOG_ERR "%pd %pp: fail to add reg of REBAR_CTRL 
> rc=%d\n",
> +                   pdev->domain, &pdev->sbdf, rc);
> +            return rc;
> +        }
> +
> +        bar->resizable_sizes |=
> +            (pci_conf_read32(pdev->sbdf, rebar_offset + PCI_REBAR_CAP(i)) >>
> +             PCI_REBAR_CAP_SHIFT);

Imo this would better use = in place of |= and (see also below) would also
better use MASK_EXTR() just like ...

> +        bar->resizable_sizes |=
> +            ((uint64_t)MASK_EXTR(ctrl, PCI_REBAR_CTRL_SIZES) <<
> +             (32 - PCI_REBAR_CAP_SHIFT));

... this one does.

Further I think you want to truncate the value for 32-bit BARs, such that
rebar_ctrl_write() would properly reject attempts to set sizes of 4G and
above for them.

> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -232,6 +232,12 @@ void cf_check vpci_hw_write16(
>      pci_conf_write16(pdev->sbdf, reg, val);
>  }
>  
> +void cf_check vpci_hw_write32(
> +    const struct pci_dev *pdev, unsigned int reg, uint32_t val, void *data)
> +{
> +    pci_conf_write32(pdev->sbdf, reg, val);
> +}

This function is being added just to handle writing of a r/o register.
Can't you better re-use vpci_ignored_write()?

> --- 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_SIZE_BIAS  20

I think it would be best if all register definitions came first, and
auxiliary ones followed afterwards (maybe even separated by a brief
comment for clarity).

> +#define PCI_REBAR_CAP(n)     (4 + 8 * (n))   /* capability register */
> +#define  PCI_REBAR_CAP_SHIFT         4               /* shift for supported 
> BAR sizes */
> +#define PCI_REBAR_CTRL(n)    (8 + 8 * (n))   /* control register */

Something's odd with the padding here. Please be consistent with the use
of whitespace (ought to be only hard tabs here afaict).

> +#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     0x00001F00      /* BAR size */

This field is 6 bits wide in the spec I'm looking at. Or else BAR sizes
2^^52 and up can't be encoded.

> +#define  PCI_REBAR_CTRL_SIZE(v) \
> +            (1UL << (MASK_EXTR(v, PCI_REBAR_CTRL_BAR_SIZE) \
> +                     + PCI_REBAR_SIZE_BIAS))
> +#define  PCI_REBAR_CTRL_SIZES                0xFFFF0000U     /* supported 
> BAR sizes */

PCI_REBAR_CAP_SHIFT and PCI_REBAR_CTRL_SIZES don't fit together very well.
Imo both want representing as masks.

Jan