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