Re: [PATCH v11 3/5] vpci/rebar: Implement cleanup function for Rebar

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

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.Chen@xxxxxxx>
>> ---
>> cc: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
>> ---
>> 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.