1. Patchew
  2. Xen
  3. [PATCH v3 00/11] Support hiding capability when its initialization fails
  4. View patch

[PATCH v3 11/11] vpci/msix: Add function to clean MSIX resources

Jiqian Chen posted 11 patches 6 months, 1 week ago
There is a newer version of this series
[PATCH v3 11/11] vpci/msix: Add function to clean MSIX resources
Posted by Jiqian Chen 6 months, 1 week ago 
When init_msix() fails, it needs to clean all MSIX resources.
So, add a new function to do that.

Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
---
cc: "Roger Pau Monné" <roger.pau@citrix.com>
---
v2->v3 changes:
* Remove unnecessary clean operations in fini_msix().

v1->v2 changes:
new patch.

Best regards,
Jiqian Chen.
---
 xen/drivers/vpci/msix.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
index 0228ffd9fda9..e322c260f6bc 100644
--- a/xen/drivers/vpci/msix.c
+++ b/xen/drivers/vpci/msix.c
@@ -703,6 +703,25 @@ int vpci_make_msix_hole(const struct pci_dev *pdev)
     return 0;
 }
 
+static void fini_msix(struct pci_dev *pdev)
+{
+    struct vpci *vpci = pdev->vpci;
+    unsigned int msix_pos = pdev->msix_pos;
+
+    if ( !msix_pos || !vpci->msix )
+        return;
+
+    list_del(&vpci->msix->next);
+
+    for ( unsigned int i = 0; i < ARRAY_SIZE(vpci->msix->table); i++ )
+        if ( vpci->msix->table[i] )
+            iounmap(vpci->msix->table[i]);
+
+    vpci_remove_registers(vpci, msix_control_reg(msix_pos), 2);
+    xfree(vpci->msix);
+    vpci->msix = NULL;
+}
+
 static int cf_check init_msix(struct pci_dev *pdev)
 {
     struct domain *d = pdev->domain;
@@ -757,7 +776,7 @@ static int cf_check init_msix(struct pci_dev *pdev)
 
     return rc;
 }
-REGISTER_VPCI_LEGACY_CAP(PCI_CAP_ID_MSIX, init_msix, NULL);
+REGISTER_VPCI_LEGACY_CAP(PCI_CAP_ID_MSIX, init_msix, fini_msix);
 
 /*
  * Local variables:
-- 
2.34.1
Re: [PATCH v3 11/11] vpci/msix: Add function to clean MSIX resources
Posted by Roger Pau Monné 5 months, 3 weeks ago 
On Mon, Apr 21, 2025 at 02:19:03PM +0800, Jiqian Chen wrote:
> When init_msix() fails, it needs to clean all MSIX resources.
> So, add a new function to do that.
> 
> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
> ---
> cc: "Roger Pau Monné" <roger.pau@citrix.com>
> ---
> v2->v3 changes:
> * Remove unnecessary clean operations in fini_msix().
> 
> v1->v2 changes:
> new patch.
> 
> Best regards,
> Jiqian Chen.
> ---
>  xen/drivers/vpci/msix.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
> index 0228ffd9fda9..e322c260f6bc 100644
> --- a/xen/drivers/vpci/msix.c
> +++ b/xen/drivers/vpci/msix.c
> @@ -703,6 +703,25 @@ int vpci_make_msix_hole(const struct pci_dev *pdev)
>      return 0;
>  }
>  
> +static void fini_msix(struct pci_dev *pdev)
> +{
> +    struct vpci *vpci = pdev->vpci;
> +    unsigned int msix_pos = pdev->msix_pos;
> +
> +    if ( !msix_pos || !vpci->msix )

That's not fully correct here.  See how in init_msix() vpci->msix is
set at the tail of the function, after having added the register
handlers.

I think you instead want:

if ( !msix_pos )
    return;

vpci_remove_registers(vpci, msix_control_reg(msix_pos), 2);

if ( !(vpci->msix )
    return;

list_del(&vpci->msix->next);
...

> +        return;
> +
> +    list_del(&vpci->msix->next);
> +
> +    for ( unsigned int i = 0; i < ARRAY_SIZE(vpci->msix->table); i++ )
> +        if ( vpci->msix->table[i] )
> +            iounmap(vpci->msix->table[i]);
> +

Since you have added to all previous cleanup functions, do you also
need a comment here to mention the capability header is not handled?

TBH I'm not sure whether that's relevant in the context here (and
other cleanup functions), as the capability header traps are not added
by the REGISTER_VPCI_{LEGACY,EXTENDED}_CAP() init hooks either, so it
would seem asymmetric for the cleanup hook to attempt to remove those
in the first place.

> +    vpci_remove_registers(vpci, msix_control_reg(msix_pos), 2);
> +    xfree(vpci->msix);
> +    vpci->msix = NULL;

XFREE();

Thanks, Roger.
Re: [PATCH v3 11/11] vpci/msix: Add function to clean MSIX resources
Posted by Chen, Jiqian 5 months, 3 weeks ago 
On 2025/5/8 18:04, Roger Pau Monné wrote:
> On Mon, Apr 21, 2025 at 02:19:03PM +0800, Jiqian Chen wrote:
>> When init_msix() fails, it needs to clean all MSIX resources.
>> So, add a new function to do that.
>>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
>> ---
>> cc: "Roger Pau Monné" <roger.pau@citrix.com>
>> ---
>> v2->v3 changes:
>> * Remove unnecessary clean operations in fini_msix().
>>
>> v1->v2 changes:
>> new patch.
>>
>> Best regards,
>> Jiqian Chen.
>> ---
>>  xen/drivers/vpci/msix.c | 21 ++++++++++++++++++++-
>>  1 file changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
>> index 0228ffd9fda9..e322c260f6bc 100644
>> --- a/xen/drivers/vpci/msix.c
>> +++ b/xen/drivers/vpci/msix.c
>> @@ -703,6 +703,25 @@ int vpci_make_msix_hole(const struct pci_dev *pdev)
>>      return 0;
>>  }
>>  
>> +static void fini_msix(struct pci_dev *pdev)
>> +{
>> +    struct vpci *vpci = pdev->vpci;
>> +    unsigned int msix_pos = pdev->msix_pos;
>> +
>> +    if ( !msix_pos || !vpci->msix )
> 
> That's not fully correct here.  See how in init_msix() vpci->msix is
> set at the tail of the function, after having added the register
> handlers.
Thanks! You are more meticulous. I didn't notice that.
Will change in next version.

> 
> I think you instead want:
> 
> if ( !msix_pos )
>     return;
> 
> vpci_remove_registers(vpci, msix_control_reg(msix_pos), 2);
> 
> if ( !(vpci->msix )
>     return;
> 
> list_del(&vpci->msix->next);
> ...
> 
>> +        return;
>> +
>> +    list_del(&vpci->msix->next);
>> +
>> +    for ( unsigned int i = 0; i < ARRAY_SIZE(vpci->msix->table); i++ )
>> +        if ( vpci->msix->table[i] )
>> +            iounmap(vpci->msix->table[i]);
>> +
> 
> Since you have added to all previous cleanup functions, do you also
> need a comment here to mention the capability header is not handled?
> 
> TBH I'm not sure whether that's relevant in the context here (and
> other cleanup functions), as the capability header traps are not added
> by the REGISTER_VPCI_{LEGACY,EXTENDED}_CAP() init hooks either, so it
> would seem asymmetric for the cleanup hook to attempt to remove those
> in the first place.
Indeed, you are right.
For symmetry consistency, I should not have to add these comments.
I will remove them for MSI and Rebar in next version.

> 
>> +    vpci_remove_registers(vpci, msix_control_reg(msix_pos), 2);
>> +    xfree(vpci->msix);
>> +    vpci->msix = NULL;
> 
> XFREE();
> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.