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

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

On 2025/1/21 18:29, Jan Beulich wrote:
> On 21.01.2025 10:29, Roger Pau Monné wrote:
>> On Tue, Jan 21, 2025 at 09:10:26AM +0000, Chen, Jiqian wrote:
>>> On 2025/1/21 16:46, Roger Pau Monné wrote:
>>>> On Tue, Jan 14, 2025 at 11:26:36AM +0800, Jiqian Chen wrote:
>>>>> +    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;
>>>>> +        }
>>>>> +
>>>>> +        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: fail to add reg of REBAR_CAP 
>>>>> rc=%d\n",
>>>>> +                   pdev->domain, &pdev->sbdf, rc);
>>>>> +            return rc;
>>>>> +        }
>>>>> +
>>>>> +        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: fail to add reg of REBAR_CTRL 
>>>>> rc=%d\n",
>>>>> +                   pdev->domain, &pdev->sbdf, rc);
>>>>> +            return rc;
>>>>
>>>> I think we said we wanted to attempt to continue here, rather than
>>>> returning an error and thus removing all vPCI handlers from the
>>>> device?
>>> I thought the result of your discussion with Jan was that I only needed to 
>>> change the above two error paths to be "continue".
>>> If these two also need to be changed, I will modify them in the next 
>>> version.
>>
>> Hm, let's wait for Jan to confirm, but even if handler cannot be setup
>> for some of the registers, it's better than just allowing dom0
>> unmediated access to the capability.
> 
> I remained silent on this because I accepted this middle ground as ...
> 
>> None of this is ideal, but it seems to be the option that gives dom0
>> most options to successfully boot.
> 
> ... perhaps the most reasonable compromise.
OK, I see.
I will change to "continue" in next version and send v6 soon.
Thank you.

> 
> Jan

-- 
Best regards,
Jiqian Chen.