[PATCH 1/3] hw/mem/cxl_type3: Add paired msix_uninit_exclusive_bar() call

[PATCH 1/3] hw/mem/cxl_type3: Add paired msix_uninit_exclusive_bar() call

[Li Zhijian]

msix_uninit_exclusive_bar() should be paired with msix_init_exclusive_bar()

Ensure proper resource cleanup by adding the missing
`msix_uninit_exclusive_bar()` call for the Type3 CXL device.

Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
 hw/mem/cxl_type3.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index f64af19ed6ae..9dad250f56d5 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -936,6 +936,7 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
 err_release_cdat:
     cxl_doe_cdat_release(cxl_cstate);
 err_free_special_ops:
+    msix_uninit_exclusive_bar(pci_dev);
     g_free(regs->special_ops);
 err_address_space_free:
     if (ct3d->dc.host_dc) {
@@ -959,6 +960,7 @@ static void ct3_exit(PCIDevice *pci_dev)
 
     pcie_aer_exit(pci_dev);
     cxl_doe_cdat_release(cxl_cstate);
+    msix_uninit_exclusive_bar(pci_dev);
     g_free(regs->special_ops);
     if (ct3d->dc.host_dc) {
         cxl_destroy_dc_regions(ct3d);
-- 
2.47.0

[PATCH 2/3] hw/mem/cxl_type3: Fix special_ops memory leak on msix_init_exclusive_bar() failure

[Li Zhijian]

Address a memory leak issue by ensuring `regs->special_ops` is freed when
`msix_init_exclusive_bar()` encounters an error during CXL Type3 device
initialization.

Additionally, this patch renames err_address_space_free to err_msix_uninit
for better clarity and logical flow

Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
 hw/mem/cxl_type3.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index 9dad250f56d5..9eb3d0979cf5 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -885,7 +885,7 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
     /* MSI(-X) Initialization */
     rc = msix_init_exclusive_bar(pci_dev, CXL_T3_MSIX_VECTOR_NR, 4, NULL);
     if (rc) {
-        goto err_address_space_free;
+        goto err_free_special_ops;
     }
     for (i = 0; i < CXL_T3_MSIX_VECTOR_NR; i++) {
         msix_vector_use(pci_dev, i);
@@ -899,7 +899,7 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
     cxl_cstate->cdat.free_cdat_table = ct3_free_cdat_table;
     cxl_cstate->cdat.private = ct3d;
     if (!cxl_doe_cdat_init(cxl_cstate, errp)) {
-        goto err_free_special_ops;
+        goto err_msix_uninit;
     }
 
     pcie_cap_deverr_init(pci_dev);
@@ -936,9 +936,9 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
 err_release_cdat:
     cxl_doe_cdat_release(cxl_cstate);
 err_free_special_ops:
-    msix_uninit_exclusive_bar(pci_dev);
     g_free(regs->special_ops);
-err_address_space_free:
+err_msix_uninit:
+    msix_uninit_exclusive_bar(pci_dev);
     if (ct3d->dc.host_dc) {
         cxl_destroy_dc_regions(ct3d);
         address_space_destroy(&ct3d->dc.host_dc_as);
-- 
2.47.0

Re: [PATCH 2/3] hw/mem/cxl_type3: Fix special_ops memory leak on msix_init_exclusive_bar() failure

[Jonathan Cameron]

On Mon, 20 Jan 2025 11:09:46 +0800
Li Zhijian <lizhijian@fujitsu.com> wrote:

> Address a memory leak issue by ensuring `regs->special_ops` is freed when
> `msix_init_exclusive_bar()` encounters an error during CXL Type3 device
> initialization.
> 
> Additionally, this patch renames err_address_space_free to err_msix_uninit
> for better clarity and logical flow
> 
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
Hi.

The need to reorder the cleanup calls made me look more
closely at this patch. Either the order was wrong here or in the previous
patch. I think the issue is here.

Jonathan

> ---
>  hw/mem/cxl_type3.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 9dad250f56d5..9eb3d0979cf5 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -885,7 +885,7 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
>      /* MSI(-X) Initialization */
>      rc = msix_init_exclusive_bar(pci_dev, CXL_T3_MSIX_VECTOR_NR, 4, NULL);
>      if (rc) {
> -        goto err_address_space_free;
> +        goto err_free_special_ops;
>      }
>      for (i = 0; i < CXL_T3_MSIX_VECTOR_NR; i++) {
>          msix_vector_use(pci_dev, i);
> @@ -899,7 +899,7 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
>      cxl_cstate->cdat.free_cdat_table = ct3_free_cdat_table;
>      cxl_cstate->cdat.private = ct3d;
>      if (!cxl_doe_cdat_init(cxl_cstate, errp)) {
> -        goto err_free_special_ops;
> +        goto err_msix_uninit;
>      }
>  
>      pcie_cap_deverr_init(pci_dev);
> @@ -936,9 +936,9 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
>  err_release_cdat:
>      cxl_doe_cdat_release(cxl_cstate);
>  err_free_special_ops:
> -    msix_uninit_exclusive_bar(pci_dev);
>      g_free(regs->special_ops);
> -err_address_space_free:
> +err_msix_uninit:
> +    msix_uninit_exclusive_bar(pci_dev);

This reorder doesn't look correct.

Should end up I think as
err_release_cdata:
    cxl_doe_cdata_release(cxl_cstate);
err_msix_uninit:
    msix_uninit_eclusive_bar(pci_dev);
err_free_special_ops:
    g_free(regs->special_ops)
err_address_space_free:
etc.

>      if (ct3d->dc.host_dc) {
>          cxl_destroy_dc_regions(ct3d);
>          address_space_destroy(&ct3d->dc.host_dc_as);

Re: [PATCH 2/3] hw/mem/cxl_type3: Fix special_ops memory leak on msix_init_exclusive_bar() failure

[Jonathan Cameron]

On Tue, 21 Jan 2025 14:58:12 +0000
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:

> On Mon, 20 Jan 2025 11:09:46 +0800
> Li Zhijian <lizhijian@fujitsu.com> wrote:
> 
> > Address a memory leak issue by ensuring `regs->special_ops` is freed when
> > `msix_init_exclusive_bar()` encounters an error during CXL Type3 device
> > initialization.
> > 
> > Additionally, this patch renames err_address_space_free to err_msix_uninit
> > for better clarity and logical flow
> > 
> > Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>  
> Hi.
> 
> The need to reorder the cleanup calls made me look more
> closely at this patch. Either the order was wrong here or in the previous
> patch. I think the issue is here.
> 
> Jonathan
> 
> > ---
> >  hw/mem/cxl_type3.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> > index 9dad250f56d5..9eb3d0979cf5 100644
> > --- a/hw/mem/cxl_type3.c
> > +++ b/hw/mem/cxl_type3.c
> > @@ -885,7 +885,7 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
> >      /* MSI(-X) Initialization */
> >      rc = msix_init_exclusive_bar(pci_dev, CXL_T3_MSIX_VECTOR_NR, 4, NULL);
> >      if (rc) {
> > -        goto err_address_space_free;
> > +        goto err_free_special_ops;
> >      }
> >      for (i = 0; i < CXL_T3_MSIX_VECTOR_NR; i++) {
> >          msix_vector_use(pci_dev, i);
> > @@ -899,7 +899,7 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
> >      cxl_cstate->cdat.free_cdat_table = ct3_free_cdat_table;
> >      cxl_cstate->cdat.private = ct3d;
> >      if (!cxl_doe_cdat_init(cxl_cstate, errp)) {
> > -        goto err_free_special_ops;
> > +        goto err_msix_uninit;
> >      }
> >  
> >      pcie_cap_deverr_init(pci_dev);
> > @@ -936,9 +936,9 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
> >  err_release_cdat:
> >      cxl_doe_cdat_release(cxl_cstate);
> >  err_free_special_ops:
> > -    msix_uninit_exclusive_bar(pci_dev);
> >      g_free(regs->special_ops);
> > -err_address_space_free:
> > +err_msix_uninit:
> > +    msix_uninit_exclusive_bar(pci_dev);  
> 
> This reorder doesn't look correct.
> 
> Should end up I think as
> err_release_cdata:
>     cxl_doe_cdata_release(cxl_cstate);
> err_msix_uninit:
>     msix_uninit_eclusive_bar(pci_dev);
> err_free_special_ops:
>     g_free(regs->special_ops)
> err_address_space_free:
This last label can go away.

I've applied your series with order modified as here to my CXL staging git
tree. I'll push out later today hopefully.  

Thanks,

Jonathan

> etc.
> 
> >      if (ct3d->dc.host_dc) {
> >          cxl_destroy_dc_regions(ct3d);
> >          address_space_destroy(&ct3d->dc.host_dc_as);  
> 
> 

Re: [PATCH 2/3] hw/mem/cxl_type3: Fix special_ops memory leak on msix_init_exclusive_bar() failure

[Zhijian Li (Fujitsu)]

On 21/01/2025 23:19, Jonathan Cameron wrote:
>>> -    msix_uninit_exclusive_bar(pci_dev);
>>>       g_free(regs->special_ops);
>>> -err_address_space_free:
>>> +err_msix_uninit:
>>> +    msix_uninit_exclusive_bar(pci_dev);
>> This reorder doesn't look correct.
>>
>> Should end up I think as
>> err_release_cdata:
>>      cxl_doe_cdata_release(cxl_cstate);
>> err_msix_uninit:
>>      msix_uninit_eclusive_bar(pci_dev);
>> err_free_special_ops:
>>      g_free(regs->special_ops)
>> err_address_space_free:
> This last label can go away.
> 
> I've applied your series with order modified as here to my CXL staging git
> tree. 

Thank you very much for your assistance.
Indeed, their order should be rearranged (I submitted them in the sequence in which I discovered the issues).



I'll push out later today hopefully.
> 
> Thanks,
> 
> Jonathan
> 
>> etc.

[PATCH 3/3] hw/mem/cxl_type3: Ensure errp is set on realization failure

[Li Zhijian]

Simply pass the errp to its callee which will set errp if needed, to
enhance error reporting for CXL Type 3 device initialization by setting
the errp when realization functions fail.

Previously, failing to set `errp` could result in errors being overlooked,
causing the system to mistakenly treat failure scenarios as successful and
potentially leading to redundant cleanup operations in ct3_exit().

Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
 hw/mem/cxl_type3.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index 9eb3d0979cf5..c3b6a1d6a612 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -883,7 +883,7 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
                      &ct3d->cxl_dstate.device_registers);
 
     /* MSI(-X) Initialization */
-    rc = msix_init_exclusive_bar(pci_dev, CXL_T3_MSIX_VECTOR_NR, 4, NULL);
+    rc = msix_init_exclusive_bar(pci_dev, CXL_T3_MSIX_VECTOR_NR, 4, errp);
     if (rc) {
         goto err_free_special_ops;
     }
@@ -904,7 +904,7 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
 
     pcie_cap_deverr_init(pci_dev);
     /* Leave a bit of room for expansion */
-    rc = pcie_aer_init(pci_dev, PCI_ERR_VER, 0x200, PCI_ERR_SIZEOF, NULL);
+    rc = pcie_aer_init(pci_dev, PCI_ERR_VER, 0x200, PCI_ERR_SIZEOF, errp);
     if (rc) {
         goto err_release_cdat;
     }
-- 
2.47.0

Re: [PATCH 3/3] hw/mem/cxl_type3: Ensure errp is set on realization failure

[Jonathan Cameron]

On Mon, 20 Jan 2025 11:09:47 +0800
Li Zhijian <lizhijian@fujitsu.com> wrote:

> Simply pass the errp to its callee which will set errp if needed, to
> enhance error reporting for CXL Type 3 device initialization by setting
> the errp when realization functions fail.
> 
> Previously, failing to set `errp` could result in errors being overlooked,
> causing the system to mistakenly treat failure scenarios as successful and
> potentially leading to redundant cleanup operations in ct3_exit().
> 
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
Looks good to me so just that reordering issue in patch 2.

Thanks for fixing this up.

Jonathan

> ---
>  hw/mem/cxl_type3.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 9eb3d0979cf5..c3b6a1d6a612 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -883,7 +883,7 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
>                       &ct3d->cxl_dstate.device_registers);
>  
>      /* MSI(-X) Initialization */
> -    rc = msix_init_exclusive_bar(pci_dev, CXL_T3_MSIX_VECTOR_NR, 4, NULL);
> +    rc = msix_init_exclusive_bar(pci_dev, CXL_T3_MSIX_VECTOR_NR, 4, errp);
>      if (rc) {
>          goto err_free_special_ops;
>      }
> @@ -904,7 +904,7 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
>  
>      pcie_cap_deverr_init(pci_dev);
>      /* Leave a bit of room for expansion */
> -    rc = pcie_aer_init(pci_dev, PCI_ERR_VER, 0x200, PCI_ERR_SIZEOF, NULL);
> +    rc = pcie_aer_init(pci_dev, PCI_ERR_VER, 0x200, PCI_ERR_SIZEOF, errp);
>      if (rc) {
>          goto err_release_cdat;
>      }