On 2025/5/8 17:39, Roger Pau Monné wrote:
> On Mon, Apr 21, 2025 at 02:19:01PM +0800, Jiqian Chen wrote:
>> When init_rebar() fails, the previous new changes will hide Rebar
>> capability, it can't rely on vpci_deassign_device() to remove all
>> Rebar related registers anymore, those registers must be removed
>> fini_rebar().
>>
>> To do that, call vpci_remove_registers() to remove all possible
>> registered registers.
>>
>> Signed-off-by: Jiqian Chen <[email protected]>
>> ---
>> cc: "Roger Pau Monné" <[email protected]>
>> ---
>> v2->v3 changes:
>> * Use fini_rebar() to remove all register instead of in the failure path of
>> init_rebar();
>>
>> v1->v2 changes:
>> * Called vpci_remove_registers() to remove all possible registered registers
>> instead of using a array to record all registered register.
>>
>> Best regards,
>> Jiqian Chen.
>> ---
>> xen/drivers/vpci/rebar.c | 35 ++++++++++++++++++++++++-----------
>> 1 file changed, 24 insertions(+), 11 deletions(-)
>>
>> diff --git a/xen/drivers/vpci/rebar.c b/xen/drivers/vpci/rebar.c
>> index 026f8f7972d9..325090afb0f8 100644
>> --- a/xen/drivers/vpci/rebar.c
>> +++ b/xen/drivers/vpci/rebar.c
>> @@ -49,6 +49,26 @@ static void cf_check rebar_ctrl_write(const struct
>> pci_dev *pdev,
>> bar->guest_addr = bar->addr;
>> }
>>
>> +static void fini_rebar(struct pci_dev *pdev)
By the way, I will rename this to be cleanup_rebar since the hook name will be
changed in next version.
>> +{
>> + uint32_t ctrl;
>> + unsigned int nbars;
>> + unsigned int rebar_offset = pci_find_ext_capability(pdev->sbdf,
>> +
>> PCI_EXT_CAP_ID_REBAR);
>> +
>> + if ( !rebar_offset || !is_hardware_domain(pdev->domain) )
>
> Maybe add an ASSERT_UNREACHABLE() here? I don't think we are expected
> to get into the cleanup function for the capability if it's not
> present, or if the owner of the device is not the hardware domain.
Yes, we don't expect that.
Will add an ASSERT_UNREACHABLE() here in next version.
>
>> + return;
>> +
>> + ctrl = pci_conf_read32(pdev->sbdf, rebar_offset + PCI_REBAR_CTRL(0));
>> + nbars = MASK_EXTR(ctrl, PCI_REBAR_CTRL_NBAR_MASK);
>> + /*
>> + * Remove all possible registered registers except header.
>> + * Header register will be removed in mask function.
>> + */
>> + vpci_remove_registers(pdev->vpci, rebar_offset + PCI_REBAR_CAP(0),
>> + PCI_REBAR_CTRL(nbars - 1));
>> +}
>> +
>> static int cf_check init_rebar(struct pci_dev *pdev)
>> {
>> uint32_t ctrl;
>> @@ -80,7 +100,7 @@ static int cf_check init_rebar(struct pci_dev *pdev)
>> {
>> printk(XENLOG_ERR "%pd %pp: too big BAR number %u in
>> REBAR_CTRL\n",
>> pdev->domain, &pdev->sbdf, index);
>> - continue;
>> + return -EINVAL;
>
> -E2BIG might be better here. In general I try to avoid using EINVAL,
> as it's a catch all that makes differentiating error later on harder.
Got it, will change.
>
>> }
>>
>> bar = &pdev->vpci->header.bars[index];
>> @@ -88,7 +108,7 @@ static int cf_check init_rebar(struct pci_dev *pdev)
>> {
>> printk(XENLOG_ERR "%pd %pp: BAR%u is not in memory space\n",
>> pdev->domain, &pdev->sbdf, index);
>> - continue;
>> + return -EINVAL;
>
> Maybe -EDOM here? -ENXIO or EIO might also be appropriate.
Will change to -ENXIO, it seems more suitable.
Thanks.
>
> Overall looks good.
>
> Thanks, Roger.
--
Best regards,
Jiqian Chen.
