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 <Jiqian.Chen@amd.com>
---
cc: "Roger Pau Monné" <roger.pau@citrix.com>
---
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)
+{
+    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) )
+        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;
         }
 
         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;
         }
 
         rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rebar_ctrl_write,
@@ -97,14 +117,7 @@ static int cf_check init_rebar(struct pci_dev *pdev)
         {
             printk(XENLOG_ERR "%pd %pp: BAR%u fail to add reg of REBAR_CTRL rc=%d\n",
                    pdev->domain, &pdev->sbdf, index, rc);
-            /*
-             * Ideally we would hide the ReBar capability on error, but code
-             * for doing so still needs to be written. Use continue instead
-             * to keep any already setup register hooks, as returning an
-             * error will cause the hardware domain to get unmediated access
-             * to all device registers.
-             */
-            continue;
+            return rc;
         }
 
         bar->resizable_sizes =
@@ -118,7 +131,7 @@ static int cf_check init_rebar(struct pci_dev *pdev)
 
     return 0;
 }
-REGISTER_VPCI_EXTENDED_CAP(PCI_EXT_CAP_ID_REBAR, init_rebar, NULL);
+REGISTER_VPCI_EXTENDED_CAP(PCI_EXT_CAP_ID_REBAR, init_rebar, fini_rebar);
 
 /*
  * Local variables:
-- 
2.34.1

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 <Jiqian.Chen@amd.com>
> ---
> cc: "Roger Pau Monné" <roger.pau@citrix.com>
> ---
> 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)
> +{
> +    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.

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

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

Overall looks good.

Thanks, Roger.

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