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

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

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.

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

-- 
Best regards,
Jiqian Chen.