On 2025/8/29 18:22, Roger Pau Monné wrote:
> On Fri, Aug 08, 2025 at 04:03:35PM +0800, Jiqian Chen wrote:
>> When Rebar initialization fails, vPCI hides the capability, but
>> removing handlers and datas won't be performed until the device is
>> deassigned. So, implement Rebar cleanup hook that will be called to
>> cleanup Rebar related handlers and free it's associated data when
>> initialization fails.
>>
>> Signed-off-by: Jiqian Chen <jiqian.c...@amd.com>
>> ---
>> cc: "Roger Pau Monné" <roger....@citrix.com>
>> ---
>> v10->v11 changes:
>> * Add ASSERT_UNREACHABLE() when vpci_remove_registers() fails
>> * When hide == true, add handlers to let Rebar ctrl be RO.
>> * Remove Roger's Reviewed-by since patch change.
>>
>> v9->v10 changes:
>> v8->v9 changes:
>> No.
>>
>> v7->v8 changes:
>> * Add Roger's Reviewed-by.
>>
>> v6->v7 changes:
>> * Change the pointer parameter of cleanup_rebar() to be const.
>> * Print error when vpci_remove_registers() fail in cleanup_rebar().
>>
>> v5->v6 changes:
>> No.
>>
>> v4->v5 changes:
>> * Change definition "static void cleanup_rebar" to "static int cf_check
>> cleanup_rebar" since cleanup hook is changed to be int.
>>
>> v3->v4 changes:
>> * Change function name from fini_rebar() to cleanup_rebar().
>> * Change the error number to be E2BIG and ENXIO in init_rebar().
>>
>> 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 | 66 +++++++++++++++++++++++++++++++++-------
>> 1 file changed, 55 insertions(+), 11 deletions(-)
>>
>> diff --git a/xen/drivers/vpci/rebar.c b/xen/drivers/vpci/rebar.c
>> index 3c18792d9bcd..91d5369d75e2 100644
>> --- a/xen/drivers/vpci/rebar.c
>> +++ b/xen/drivers/vpci/rebar.c
>> @@ -49,6 +49,57 @@ static void cf_check rebar_ctrl_write(const struct
>> pci_dev *pdev,
>> bar->guest_addr = bar->addr;
>> }
>>
>> +static int cf_check cleanup_rebar(const struct pci_dev *pdev, bool hide)
>> +{
>> + int rc;
>> + 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) )
>> + {
>> + ASSERT_UNREACHABLE();
>> + return 0;
>> + }
>> +
>> + ctrl = pci_conf_read32(pdev->sbdf, rebar_offset + PCI_REBAR_CTRL(0));
>> + nbars = MASK_EXTR(ctrl, PCI_REBAR_CTRL_NBAR_MASK);
>> +
>> + rc = vpci_remove_registers(pdev->vpci, rebar_offset + PCI_REBAR_CAP(0),
>> + PCI_REBAR_CTRL(nbars - 1));
>> + if ( rc )
>> + {
>> + printk(XENLOG_ERR "%pd %pp: fail to remove Rebar handlers rc=%d\n",
>> + pdev->domain, &pdev->sbdf, rc);
>> + ASSERT_UNREACHABLE();
>> + return rc;
>> + }
>> +
>> + if ( !hide )
>> + return 0;
>
> Now that the handler can differentiate between calls to hide the
> capability versus calls from device deassign, do we need to call
> vpci_remove_registers() for the non-hiding case?
>
> The non-hiding case is only used from vpci_deassign_device(), and just
> after having called all the cleanup hooks that function purges any
> remaining registered handlers. It would be OK to do something like:
>
> static int cf_check cleanup_rebar(const struct pci_dev *pdev, bool hide)
> {
> int rc;
> 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) )
> {
> ASSERT_UNREACHABLE();
> return 0;
> }
>
> if ( !hide )
> return 0;
>
> ... remove handler + mask register ...
>
> Thoughts?
Got it.
But why not moving it above the first check " if ( !rebar_offset ||
!is_hardware_domain(pdev->domain) )" ?
>
>> +
>> + /*
>> + * The driver may not traverse the capability list and think device
>> + * supports Rebar by default. So here let the control register of Rebar
>> + * be Read-Only is to ensure Rebar disabled.
>> + */
>> + for ( unsigned int i = 0; i < nbars; i++ )
>> + {
>> + rc = vpci_add_register(pdev->vpci, vpci_hw_read32, NULL,
>> + rebar_offset + PCI_REBAR_CTRL(i), 4, NULL);
>> + if ( rc )
>> + {
>> + printk(XENLOG_ERR
>> + "%pd %pp: fail to add Rebar ctrl handler rc=%d\n",
>> + pdev->domain, &pdev->sbdf, rc);
>> + return rc;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static int cf_check init_rebar(struct pci_dev *pdev)
>> {
>> uint32_t ctrl;
>> @@ -80,7 +131,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 -E2BIG;
>> }
>>
>> bar = &pdev->vpci->header.bars[index];
>> @@ -88,7 +139,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 -ENXIO;
>
> I'm unsure we want to return an error here and in the check above,
> given this capability is dom0 only, we might want to just skip the BAR
> and continue, aiming for the other resizable BARs to be functional?
Why here need to use continue, but below vpci_add_register() fail return error?
>
> Thanks, Roger.
--
Best regards,
Jiqian Chen.
