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

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

On Wed, Feb 05, 2025 at 03:42:30AM +0000, Chen, Jiqian wrote:
> On 2025/1/27 23:08, Roger Pau Monné wrote:
> > 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?
> Sorry to reply late, I just came back from an annual leave.
> Did you mean: why I added handler vpci_hw_write32 for CAP?
> If so, this is a change since V2, that you suggested to add it because there 
> is no write limitation for dom0.

Indeed, if there's no write limitation, you can just drop the addition
of the traps, as the hardware domain by default gets read and write
access to all PCI config space.  IOW: there's no need for a
vpci_add_register() call for PCI_REBAR_CAP if the handlers are just
vpci_hw_{read,write}32().

Thanks, Roger.