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

["Roger Pau Monné" <roger.pau@citrix.com>]

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@amd.com>
> ---
> cc: "Roger Pau Monné" <roger.pau@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?

> +
> +    /*
> +     * 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?

Thanks, Roger.