[PATCH 0/2] hw/cxl: Two CXL emulation fixes.

[PATCH 0/2] hw/cxl: Two CXL emulation fixes.

[Jonathan Cameron]

Peter Maydell reported both these issues, having looked into Coverity
identified issues. The memory leak was straight forward, but testing the
second patch identified a bug in the Linux kernel.

This bug has been fixed in the series
https://lore.kernel.org/linux-cxl/165973125417.1526540.14425647258796609596.stgit@dwillia2-xfh.jf.intel.com/T/#t
and is now available in the cxl.git pending branch.

Another clear example of why QEMU emulation is useful for kernel development.

Jonathan Cameron (2):
  hw/cxl: Fix memory leak in error paths
  hw/cxl: Fix wrong query of target ports

 hw/cxl/cxl-host.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

-- 
2.32.0

[PATCH 0/2] hw/cxl: Two CXL emulation fixes.

[Jonathan Cameron via]

Peter Maydell reported both these issues, having looked into Coverity
identified issues. The memory leak was straight forward, but testing the
second patch identified a bug in the Linux kernel.

This bug has been fixed in the series
https://lore.kernel.org/linux-cxl/165973125417.1526540.14425647258796609596.stgit@dwillia2-xfh.jf.intel.com/T/#t
and is now available in the cxl.git pending branch.

Another clear example of why QEMU emulation is useful for kernel development.

Jonathan Cameron (2):
  hw/cxl: Fix memory leak in error paths
  hw/cxl: Fix wrong query of target ports

 hw/cxl/cxl-host.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

-- 
2.32.0

[PATCH 1/2] hw/cxl: Fix memory leak in error paths

[Jonathan Cameron]

Use g_autofree to free the CXLFixedWindow structure if an
error occurs in configuration before we have added to
the list (via g_steal_pointer())

Fix Coverity CID: 1488872

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 hw/cxl/cxl-host.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c
index 483d8eb13f..faa68ef038 100644
--- a/hw/cxl/cxl-host.c
+++ b/hw/cxl/cxl-host.c
@@ -26,7 +26,7 @@ static void cxl_fixed_memory_window_config(CXLState *cxl_state,
                                            CXLFixedMemoryWindowOptions *object,
                                            Error **errp)
 {
-    CXLFixedWindow *fw = g_malloc0(sizeof(*fw));
+    g_autofree CXLFixedWindow *fw = g_malloc0(sizeof(*fw));
     strList *target;
     int i;
 
@@ -64,7 +64,8 @@ static void cxl_fixed_memory_window_config(CXLState *cxl_state,
         fw->enc_int_gran = 0;
     }
 
-    cxl_state->fixed_windows = g_list_append(cxl_state->fixed_windows, fw);
+    cxl_state->fixed_windows = g_list_append(cxl_state->fixed_windows,
+                                             g_steal_pointer(&fw));
 
     return;
 }
-- 
2.32.0

[PATCH 1/2] hw/cxl: Fix memory leak in error paths

[Jonathan Cameron via]

Use g_autofree to free the CXLFixedWindow structure if an
error occurs in configuration before we have added to
the list (via g_steal_pointer())

Fix Coverity CID: 1488872

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 hw/cxl/cxl-host.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c
index 483d8eb13f..faa68ef038 100644
--- a/hw/cxl/cxl-host.c
+++ b/hw/cxl/cxl-host.c
@@ -26,7 +26,7 @@ static void cxl_fixed_memory_window_config(CXLState *cxl_state,
                                            CXLFixedMemoryWindowOptions *object,
                                            Error **errp)
 {
-    CXLFixedWindow *fw = g_malloc0(sizeof(*fw));
+    g_autofree CXLFixedWindow *fw = g_malloc0(sizeof(*fw));
     strList *target;
     int i;
 
@@ -64,7 +64,8 @@ static void cxl_fixed_memory_window_config(CXLState *cxl_state,
         fw->enc_int_gran = 0;
     }
 
-    cxl_state->fixed_windows = g_list_append(cxl_state->fixed_windows, fw);
+    cxl_state->fixed_windows = g_list_append(cxl_state->fixed_windows,
+                                             g_steal_pointer(&fw));
 
     return;
 }
-- 
2.32.0

[PATCH 2/2] hw/cxl: Fix wrong query of target ports

[Jonathan Cameron]

Two issues were present in this code:
1) Check on which register to look in was inverted.
2) Both branches use the _LO register.

Whilst here moved to extract32() rather than hand rolling
the field extraction as simpler and hopefully less error prone.

Fixes Coverity CID: 1488873

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 hw/cxl/cxl-host.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c
index faa68ef038..1adf61231a 100644
--- a/hw/cxl/cxl-host.c
+++ b/hw/cxl/cxl-host.c
@@ -104,7 +104,6 @@ static bool cxl_hdm_find_target(uint32_t *cache_mem, hwaddr addr,
     uint32_t ctrl;
     uint32_t ig_enc;
     uint32_t iw_enc;
-    uint32_t target_reg;
     uint32_t target_idx;
 
     ctrl = cache_mem[R_CXL_HDM_DECODER0_CTRL];
@@ -116,14 +115,13 @@ static bool cxl_hdm_find_target(uint32_t *cache_mem, hwaddr addr,
     iw_enc = FIELD_EX32(ctrl, CXL_HDM_DECODER0_CTRL, IW);
     target_idx = (addr / cxl_decode_ig(ig_enc)) % (1 << iw_enc);
 
-    if (target_idx > 4) {
-        target_reg = cache_mem[R_CXL_HDM_DECODER0_TARGET_LIST_LO];
-        target_reg >>= target_idx * 8;
+    if (target_idx < 4) {
+        *target = extract32(cache_mem[R_CXL_HDM_DECODER0_TARGET_LIST_LO],
+                            target_idx * 8, 8);
     } else {
-        target_reg = cache_mem[R_CXL_HDM_DECODER0_TARGET_LIST_LO];
-        target_reg >>= (target_idx - 4) * 8;
+        *target = extract32(cache_mem[R_CXL_HDM_DECODER0_TARGET_LIST_HI],
+                            (target_idx - 4) * 8, 8);
     }
-    *target = target_reg & 0xff;
 
     return true;
 }
-- 
2.32.0

[PATCH 2/2] hw/cxl: Fix wrong query of target ports

[Jonathan Cameron via]

Two issues were present in this code:
1) Check on which register to look in was inverted.
2) Both branches use the _LO register.

Whilst here moved to extract32() rather than hand rolling
the field extraction as simpler and hopefully less error prone.

Fixes Coverity CID: 1488873

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 hw/cxl/cxl-host.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c
index faa68ef038..1adf61231a 100644
--- a/hw/cxl/cxl-host.c
+++ b/hw/cxl/cxl-host.c
@@ -104,7 +104,6 @@ static bool cxl_hdm_find_target(uint32_t *cache_mem, hwaddr addr,
     uint32_t ctrl;
     uint32_t ig_enc;
     uint32_t iw_enc;
-    uint32_t target_reg;
     uint32_t target_idx;
 
     ctrl = cache_mem[R_CXL_HDM_DECODER0_CTRL];
@@ -116,14 +115,13 @@ static bool cxl_hdm_find_target(uint32_t *cache_mem, hwaddr addr,
     iw_enc = FIELD_EX32(ctrl, CXL_HDM_DECODER0_CTRL, IW);
     target_idx = (addr / cxl_decode_ig(ig_enc)) % (1 << iw_enc);
 
-    if (target_idx > 4) {
-        target_reg = cache_mem[R_CXL_HDM_DECODER0_TARGET_LIST_LO];
-        target_reg >>= target_idx * 8;
+    if (target_idx < 4) {
+        *target = extract32(cache_mem[R_CXL_HDM_DECODER0_TARGET_LIST_LO],
+                            target_idx * 8, 8);
     } else {
-        target_reg = cache_mem[R_CXL_HDM_DECODER0_TARGET_LIST_LO];
-        target_reg >>= (target_idx - 4) * 8;
+        *target = extract32(cache_mem[R_CXL_HDM_DECODER0_TARGET_LIST_HI],
+                            (target_idx - 4) * 8, 8);
     }
-    *target = target_reg & 0xff;
 
     return true;
 }
-- 
2.32.0

Re: [PATCH 0/2] hw/cxl: Two CXL emulation fixes.

[Jonathan Cameron]

On Mon, 8 Aug 2022 13:20:49 +0100
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:

> Peter Maydell reported both these issues, having looked into Coverity
> identified issues. The memory leak was straight forward, but testing the
> second patch identified a bug in the Linux kernel.
> 
> This bug has been fixed in the series
> https://lore.kernel.org/linux-cxl/165973125417.1526540.14425647258796609596.stgit@dwillia2-xfh.jf.intel.com/T/#t
> and is now available in the cxl.git pending branch.
> 
> Another clear example of why QEMU emulation is useful for kernel development.

Hi,

Any feedback on these two fixes?

Along with a two more fixes to follow in a few mins these are needed for the region code
in Linux 6.0-rc1 to work (there is a kernel fix as well that came from testing against
QEMU).

For full support we need one more feature (serial number provisioning) but without that
we get regions working as long as you create them fresh on each boot.

Thanks,

Jonathan

> 
> Jonathan Cameron (2):
>   hw/cxl: Fix memory leak in error paths
>   hw/cxl: Fix wrong query of target ports
> 
>  hw/cxl/cxl-host.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 

Re: [PATCH 0/2] hw/cxl: Two CXL emulation fixes.

[Jonathan Cameron via]

On Mon, 8 Aug 2022 13:20:49 +0100
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:

> Peter Maydell reported both these issues, having looked into Coverity
> identified issues. The memory leak was straight forward, but testing the
> second patch identified a bug in the Linux kernel.
> 
> This bug has been fixed in the series
> https://lore.kernel.org/linux-cxl/165973125417.1526540.14425647258796609596.stgit@dwillia2-xfh.jf.intel.com/T/#t
> and is now available in the cxl.git pending branch.
> 
> Another clear example of why QEMU emulation is useful for kernel development.

Hi,

Any feedback on these two fixes?

Along with a two more fixes to follow in a few mins these are needed for the region code
in Linux 6.0-rc1 to work (there is a kernel fix as well that came from testing against
QEMU).

For full support we need one more feature (serial number provisioning) but without that
we get regions working as long as you create them fresh on each boot.

Thanks,

Jonathan

> 
> Jonathan Cameron (2):
>   hw/cxl: Fix memory leak in error paths
>   hw/cxl: Fix wrong query of target ports
> 
>  hw/cxl/cxl-host.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
>