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

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

On Tue, Jan 07, 2025 at 11:06:33AM +0100, Jan Beulich wrote:
> 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?

I think this was already raised in a previous version - would be good
to know how real hardware behaves when an invalid size is set.  Is the
BAR register still reset?

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

Since we only plan to enable the capability for the hardware domain,
and in that case addr == guest_addr always, it's fine to just read
from the BAR register and update the fields.  If we do this we might
as well check that the newly reported BAR size matches what Xen
expects on debug builds at least.

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

It would indeed be better to shallow the error and return 0, however
the handlers added in this loop would need removing if no error is
returned.

> > +        }
> > +
> > +        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.

For the hardware domain at least we shouldn't add such restriction -
Xen in general allows dom0 to do things it would otherwise consider
invalid, in case it has to deal with hardware quirks.

Rather than reject Xen should just print a warning that the sizes
supported by the device are likely invalid.

> > --- 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()?

But vpci_ignored_write() ignores the write, OTOH here the write is
propagated to the hardware.

Thanks, Roger.