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

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

On Mon, Jan 27, 2025 at 03:52:31PM +0100, Jan Beulich wrote:
> On 27.01.2025 15:41, Roger Pau Monné wrote:
> > 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_read32vpci_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@xxxxxxxx>
> >>
> >> 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.
> 
> Hmm, true. Maybe with the exception of the case where the first reg
> registration works, but the 2nd fails. Since CTRL is writable but
> CAP is r/o (and data there is simply being handed through) I wonder
> whether we need to intercept CAP at all, and if we do, whether we
> wouldn't better try to register CTRL first.

Indeed, Jiqian is that a leftover from a previous version when writes
to CAP where ignored for being a read-only register?

The current adding of a handler with vpci_hw_{read,write}32() result
in the exact same behavior for a hardware domain, which is the only
domain where ReBAR will be exposed.

Thanks, Roger.