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

["Chen, Jiqian" <Jiqian.Chen@xxxxxxx>]

On 2025/1/27 22:52, 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_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.
Maybe we can remove the "continue" when failing to register CAP and keep moving 
forward to register CTRL? since
dom0 can access hardware directly without handler and CAP is RO.

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

-- 
Best regards,
Jiqian Chen.