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

[Jan Beulich <jbeulich@xxxxxxxx>]

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_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.

Jan

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