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

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

On Mon, Jan 27, 2025 at 03:20:40PM +0100, Jan Beulich wrote:
> On 23.01.2025 04:50, Jiqian Chen wrote:
> > 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.
> 
> I'm not convinced this was a good change to make. While ...
> 
> > +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;
> > +        if ( index >= PCI_HEADER_NORMAL_NR_BARS )
> > +        {
> > +            printk(XENLOG_ERR "%pd %pp: too big BAR number %u in REBAR_CTRL\n",
> > +                   pdev->domain, &pdev->sbdf, index);
> > +            continue;
> > +        }
> > +
> > +        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);
> > +            continue;
> > +        }
> 
> ... for these two cases we can permit Dom0 direct access because the BAR
> isn't going to work anyway (as far as we can tell), ...
> 
> > +        rc = vpci_add_register(pdev->vpci, vpci_hw_read32, vpci_hw_write32,
> > +                               rebar_offset + PCI_REBAR_CAP(i), 4, NULL);
> > +        if ( rc )
> > +        {
> > +            /*
> > +             * TODO: for failed pathes, need to hide ReBar capability
> > +             * from hardware domain instead of returning an error.
> > +             */
> > +            printk(XENLOG_ERR "%pd %pp: BAR%u fail to add reg of REBAR_CAP rc=%d\n",
> > +                   pdev->domain, &pdev->sbdf, index, rc);
> > +            continue;
> > +        }
> > +
> > +        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: BAR%u fail to add reg of REBAR_CTRL rc=%d\n",
> > +                   pdev->domain, &pdev->sbdf, index, rc);
> > +            continue;
> > +        }
> 
> ... in these two cases we had an issue internally, and would hence wrongly
> allow Dom0 direct access (and in case it's the 2nd one that failed, in fact
> only partially direct access, with who knows what resulting inconsistencies).
> 
> Only with this particular change undone:
R> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> Otherwise you and Roger (who needs to at least ack the change anyway) will
> need to sort that out, with me merely watching.

Ideally errors here should be dealt with by masking the capability.
However Xen doesn't yet have that support.  The usage of continue is
to merely attempt to keep any possible setup hooks working (header,
MSI, MSI-X). Returning failure from init_rebar() will cause all
vPCI hooks to be removed, and thus the hardware domain to have
unmediated access to the device, which is likely worse than just
continuing here.

This already happens in other capability init paths, that are much less
careful about returning errors, so Jan might be right that if nothing
else for consistency we return an error.  With the hope that
initialization error of capabilities in vPCI will eventually lead to
such capabilities being hidden instead of removing all vPCI handlers
from the device.

Thanks, Roger.