Re: [PATCH v2 1/1] vpci: Add resizable bar support

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.12.2024 07:09, Jiqian Chen wrote:
> --- /dev/null
> +++ b/xen/drivers/vpci/rebar.c
> @@ -0,0 +1,93 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */

Was this a deliberate decision? We default to GPL-2.0-only, I think.

> +/*
> + * Copyright (C) 2024 Advanced Micro Devices, Inc. All Rights Reserved.
> + *
> + * Author: Jiqian Chen <Jiqian.Chen@xxxxxxx>
> + */
> +
> +#include <xen/hypercall.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)
> +{
> +    uint64_t size;
> +    unsigned int index;
> +    struct vpci_bar *bars = data;
> +
> +    if ( pci_conf_read16(pdev->sbdf, PCI_COMMAND) & PCI_COMMAND_MEMORY )
> +        return;

I don't think something like this can go uncommented. I don't think the
spec mandates to drop writes in this situation?

> +    index = pci_conf_read32(pdev->sbdf, reg) & PCI_REBAR_CTRL_BAR_IDX;
> +    if ( index >= PCI_HEADER_NORMAL_NR_BARS )
> +        return;
> +
> +    if ( bars[index].type != VPCI_BAR_MEM64_LO &&
> +         bars[index].type != VPCI_BAR_MEM32 )
> +        return;
> +
> +    size = PCI_REBAR_CTRL_SIZE(val);
> +    if ( !((size >> 20) &
> +         MASK_EXTR(pci_conf_read32(pdev->sbdf, reg - 4), 
> PCI_REBAR_CAP_SIZES)) )

No such literal 4 please. What I think you mean is reg - PCI_REBAR_CTRL +
PCI_REBAR_CAP.

Also indentation is off (by 2) here.

> +        gprintk(XENLOG_WARNING,
> +                "%pp: new size %#lx for BAR%u isn't supported\n",
> +                &pdev->sbdf, size, index);
> +
> +    bars[index].size = size;
> +    bars[index].addr = 0;
> +    bars[index].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);
> +
> +    if ( !rebar_offset )
> +        return 0;
> +
> +    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;
> +
> +        rc = vpci_add_register(pdev->vpci, vpci_hw_read32, vpci_hw_write32,
> +                               rebar_offset + PCI_REBAR_CAP, 4, NULL);

The capability register is r/o aiui. While permitting hwdom to write it is
fine, DomU-s shouldn't be permitted doing so, just in case. (An alternative
to making handler selection conditional here would be to bail early for the
!hwdom case, accompanied by a TODO comment. This would then also address
the lack of virtualization of the extended capability chain, as we may not
blindly expose all capabilities to DomU-s.)

> +        if ( rc )
> +        {
> +            printk("%pp: add register for PCI_REBAR_CAP failed (rc=%d)\n",
> +                   &pdev->sbdf, rc);
> +            break;
> +        }
> +
> +        rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rebar_ctrl_write,
> +                               rebar_offset + PCI_REBAR_CTRL, 4,
> +                               pdev->vpci->header.bars);
> +        if ( rc )
> +        {
> +            printk("%pp: add register for PCI_REBAR_CTRL failed %d\n",
> +                   &pdev->sbdf, rc);
> +            break;

Is it correct to keep the other handler installed? After all ...

> +        }
> +    }
> +
> +    return 0;

... you - imo sensibly - aren't communicating the error back up (to allow
the device to be used without BAR resizing.

> @@ -541,6 +542,16 @@
>  #define  PCI_VNDR_HEADER_REV(x)      (((x) >> 16) & 0xf)
>  #define  PCI_VNDR_HEADER_LEN(x)      (((x) >> 20) & 0xfff)
>  
> +/* Resizable BARs */
> +#define PCI_REBAR_CAP                4       /* capability register */
> +#define  PCI_REBAR_CAP_SIZES         0xFFFFFFF0  /* supported BAR sizes */

Misra demands that this have a U suffix.

> +#define PCI_REBAR_CTRL               8       /* 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     0x00001F00  /* BAR size */
> +#define  PCI_REBAR_CTRL_SIZE(v) \
> +            (1UL << (MASK_EXTR(v, PCI_REBAR_CTRL_BAR_SIZE) + 20))

The literal 20 (appearing here the 2nd time) also wants hiding behind a
#define.

Jan