Re: [PATCH 3/3] hw/mem/cxl_type3: Fix problem with g_steal_pointer()

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Mon,  4 Mar 2024 11:44:06 +0100
Thomas Huth <thuth@redhat.com> wrote:

> When setting GLIB_VERSION_MAX_ALLOWED to GLIB_VERSION_2_58 or higher,
> glib adds type safety checks to the g_steal_pointer() macro. This
> triggers errors in the ct3_build_cdat_entries_for_mr() function which
> uses the g_steal_pointer() for type-casting from one pointer type to
> the other (which also looks quite weird since the local pointers have
> all been declared with g_autofree though they are never freed here).
> Fix it by using a proper typecast instead. For making this possible, we
> have to remove the QEMU_PACKED attribute from some structs since GCC
> otherwise complains that the source and destination pointer might
> have different alignment restrictions. Removing the QEMU_PACKED should
> be fine here since the structs are already naturally aligned. Anyway,
> add some QEMU_BUILD_BUG_ON() statements to make sure that we've got
> the right sizes (without padding in the structs).

I missed these as well when getting rid of the false handling
of failure of g_new0 calls.

Another alternative would be to point to the head structures rather
than the containing structure - would avoid need to cast.
That might be neater?  Should I think also remove the alignment
question?


> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  include/hw/cxl/cxl_cdat.h |  9 ++++++---
>  hw/mem/cxl_type3.c        | 24 ++++++++++++------------
>  2 files changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/include/hw/cxl/cxl_cdat.h b/include/hw/cxl/cxl_cdat.h
> index b44cefaad6..17a09066dc 100644
> --- a/include/hw/cxl/cxl_cdat.h
> +++ b/include/hw/cxl/cxl_cdat.h
> @@ -82,7 +82,8 @@ typedef struct CDATDsmas {
>      uint16_t reserved;
>      uint64_t DPA_base;
>      uint64_t DPA_length;
> -} QEMU_PACKED CDATDsmas;
> +} CDATDsmas;
> +QEMU_BUILD_BUG_ON(sizeof(CDATDsmas) != 24);
>  
>  /* Device Scoped Latency and Bandwidth Information Structure - CDAT Table 5 */
>  typedef struct CDATDslbis {
> @@ -95,7 +96,8 @@ typedef struct CDATDslbis {
>      uint64_t entry_base_unit;
>      uint16_t entry[3];
>      uint16_t reserved2;
> -} QEMU_PACKED CDATDslbis;
> +} CDATDslbis;
> +QEMU_BUILD_BUG_ON(sizeof(CDATDslbis) != 24);
>  
>  /* Device Scoped Memory Side Cache Information Structure - CDAT Table 6 */
>  typedef struct CDATDsmscis {
> @@ -122,7 +124,8 @@ typedef struct CDATDsemts {
>      uint16_t reserved;
>      uint64_t DPA_offset;
>      uint64_t DPA_length;
> -} QEMU_PACKED CDATDsemts;
> +} CDATDsemts;
> +QEMU_BUILD_BUG_ON(sizeof(CDATDsemts) != 24);
>  
>  /* Switch Scoped Latency and Bandwidth Information Structure - CDAT Table 9 */
>  typedef struct CDATSslbisHeader {
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index e8801805b9..b679dfae1c 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -46,12 +46,12 @@ static void ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
>                                            int dsmad_handle, MemoryRegion *mr,
>                                            bool is_pmem, uint64_t dpa_base)
>  {
> -    g_autofree CDATDsmas *dsmas = NULL;
> -    g_autofree CDATDslbis *dslbis0 = NULL;
> -    g_autofree CDATDslbis *dslbis1 = NULL;
> -    g_autofree CDATDslbis *dslbis2 = NULL;
> -    g_autofree CDATDslbis *dslbis3 = NULL;
> -    g_autofree CDATDsemts *dsemts = NULL;
> +    CDATDsmas *dsmas;
> +    CDATDslbis *dslbis0;
> +    CDATDslbis *dslbis1;
> +    CDATDslbis *dslbis2;
> +    CDATDslbis *dslbis3;
> +    CDATDsemts *dsemts;
>  
>      dsmas = g_malloc(sizeof(*dsmas));
>      *dsmas = (CDATDsmas) {
> @@ -135,12 +135,12 @@ static void ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
>      };
>  
>      /* Header always at start of structure */
> -    cdat_table[CT3_CDAT_DSMAS] = g_steal_pointer(&dsmas);
> -    cdat_table[CT3_CDAT_DSLBIS0] = g_steal_pointer(&dslbis0);
> -    cdat_table[CT3_CDAT_DSLBIS1] = g_steal_pointer(&dslbis1);
> -    cdat_table[CT3_CDAT_DSLBIS2] = g_steal_pointer(&dslbis2);
> -    cdat_table[CT3_CDAT_DSLBIS3] = g_steal_pointer(&dslbis3);
> -    cdat_table[CT3_CDAT_DSEMTS] = g_steal_pointer(&dsemts);
> +    cdat_table[CT3_CDAT_DSMAS] = (CDATSubHeader *)dsmas;
Could do
    cdat_table[CT3_CDAT_DSMAS] = &dsmas->header;
etc
> +    cdat_table[CT3_CDAT_DSLBIS0] = (CDATSubHeader *)dslbis0;
> +    cdat_table[CT3_CDAT_DSLBIS1] = (CDATSubHeader *)dslbis1;
> +    cdat_table[CT3_CDAT_DSLBIS2] = (CDATSubHeader *)dslbis2;
> +    cdat_table[CT3_CDAT_DSLBIS3] = (CDATSubHeader *)dslbis3;
> +    cdat_table[CT3_CDAT_DSEMTS] = (CDATSubHeader *)dsemts;
>  }
>  
>  static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv)