On Mon, 4 Mar 2024 11:44:06 +0100
Thomas Huth <th...@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 <th...@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)
