[PATCH 0/3] cxl: Fix issues with g_steal_pointer()

[PATCH 0/3] cxl: Fix issues with g_steal_pointer()

[Thomas Huth]

When setting GLIB_VERSION_MAX_ALLOWED to GLIB_VERSION_2_58 or higher
(which we'll certainly do in the not too distant future), glib adds
type safety checks to the g_steal_pointer() macro. This triggers
errors in the cxl code since the pointer types do not always match
here. Let's fix those errors now so we can switch to a newer version
of the glib in a future version of QEMU.

Thomas Huth (3):
  hw/cxl/cxl-cdat: Fix type of buf in ct3_load_cdat()
  hw/pci-bridge/cxl_upstream: Fix problem with g_steal_pointer()
  hw/mem/cxl_type3: Fix problem with g_steal_pointer()

 include/hw/cxl/cxl_cdat.h    | 17 +++++++++++------
 hw/cxl/cxl-cdat.c            |  4 ++--
 hw/mem/cxl_type3.c           | 24 ++++++++++++------------
 hw/pci-bridge/cxl_upstream.c |  8 ++++----
 4 files changed, 29 insertions(+), 24 deletions(-)

-- 
2.44.0

[PATCH 1/3] hw/cxl/cxl-cdat: Fix type of buf in ct3_load_cdat()

[Thomas Huth]

When setting GLIB_VERSION_MAX_ALLOWED to GLIB_VERSION_2_58 or higher
(which we'll certainly do in the not too distant future), glib adds
type safety checks to the g_steal_pointer() macro. This trigger an
error in the ct3_load_cdat() function: The local char *buf variable is
assigned to uint8_t *buf in CDATObject, i.e. a pointer of a different
type. Change the local variable to the same type as buf in CDATObject
to avoid the error.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/cxl/cxl-cdat.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
index 2fea975671..551545f782 100644
--- a/hw/cxl/cxl-cdat.c
+++ b/hw/cxl/cxl-cdat.c
@@ -114,7 +114,7 @@ static void ct3_build_cdat(CDATObject *cdat, Error **errp)
 static void ct3_load_cdat(CDATObject *cdat, Error **errp)
 {
     g_autofree CDATEntry *cdat_st = NULL;
-    g_autofree char *buf = NULL;
+    g_autofree uint8_t *buf = NULL;
     uint8_t sum = 0;
     int num_ent;
     int i = 0, ent = 1;
@@ -171,7 +171,7 @@ static void ct3_load_cdat(CDATObject *cdat, Error **errp)
         cdat_st[ent].base = hdr;
         cdat_st[ent].length = hdr->length;
 
-        while (buf + i < (char *)cdat_st[ent].base + cdat_st[ent].length) {
+        while (buf + i < (uint8_t *)cdat_st[ent].base + cdat_st[ent].length) {
             assert(i < file_size);
             sum += buf[i++];
         }
-- 
2.44.0

Re: [PATCH 1/3] hw/cxl/cxl-cdat: Fix type of buf in ct3_load_cdat()

[Jonathan Cameron via]

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

> When setting GLIB_VERSION_MAX_ALLOWED to GLIB_VERSION_2_58 or higher
> (which we'll certainly do in the not too distant future), glib adds
> type safety checks to the g_steal_pointer() macro. This trigger an
> error in the ct3_load_cdat() function: The local char *buf variable is
> assigned to uint8_t *buf in CDATObject, i.e. a pointer of a different
> type. Change the local variable to the same type as buf in CDATObject
> to avoid the error.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  hw/cxl/cxl-cdat.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
> index 2fea975671..551545f782 100644
> --- a/hw/cxl/cxl-cdat.c
> +++ b/hw/cxl/cxl-cdat.c
> @@ -114,7 +114,7 @@ static void ct3_build_cdat(CDATObject *cdat, Error **errp)
>  static void ct3_load_cdat(CDATObject *cdat, Error **errp)
>  {
>      g_autofree CDATEntry *cdat_st = NULL;
> -    g_autofree char *buf = NULL;
> +    g_autofree uint8_t *buf = NULL;
>      uint8_t sum = 0;
>      int num_ent;
>      int i = 0, ent = 1;
> @@ -171,7 +171,7 @@ static void ct3_load_cdat(CDATObject *cdat, Error **errp)
>          cdat_st[ent].base = hdr;
>          cdat_st[ent].length = hdr->length;
>  
> -        while (buf + i < (char *)cdat_st[ent].base + cdat_st[ent].length) {
> +        while (buf + i < (uint8_t *)cdat_st[ent].base + cdat_st[ent].length) {
>              assert(i < file_size);
>              sum += buf[i++];
>          }

[PATCH 2/3] hw/pci-bridge/cxl_upstream: Fix problem with g_steal_pointer()

[Thomas Huth]

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 build_cdat_table() 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). Let's 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).

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 include/hw/cxl/cxl_cdat.h    | 8 +++++---
 hw/pci-bridge/cxl_upstream.c | 8 ++++----
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/include/hw/cxl/cxl_cdat.h b/include/hw/cxl/cxl_cdat.h
index 8e3d094608..b44cefaad6 100644
--- a/include/hw/cxl/cxl_cdat.h
+++ b/include/hw/cxl/cxl_cdat.h
@@ -130,7 +130,8 @@ typedef struct CDATSslbisHeader {
     uint8_t data_type;
     uint8_t reserved[3];
     uint64_t entry_base_unit;
-} QEMU_PACKED CDATSslbisHeader;
+} CDATSslbisHeader;
+QEMU_BUILD_BUG_ON(sizeof(CDATSslbisHeader) != 16);
 
 #define CDAT_PORT_ID_USP 0x100
 /* Switch Scoped Latency and Bandwidth Entry - CDAT Table 10 */
@@ -139,12 +140,13 @@ typedef struct CDATSslbe {
     uint16_t port_y_id;
     uint16_t latency_bandwidth;
     uint16_t reserved;
-} QEMU_PACKED CDATSslbe;
+} CDATSslbe;
+QEMU_BUILD_BUG_ON(sizeof(CDATSslbe) != 8);
 
 typedef struct CDATSslbis {
     CDATSslbisHeader sslbis_header;
     CDATSslbe sslbe[];
-} QEMU_PACKED CDATSslbis;
+} CDATSslbis;
 
 typedef struct CDATEntry {
     void *base;
diff --git a/hw/pci-bridge/cxl_upstream.c b/hw/pci-bridge/cxl_upstream.c
index e87eb40177..537f9affb8 100644
--- a/hw/pci-bridge/cxl_upstream.c
+++ b/hw/pci-bridge/cxl_upstream.c
@@ -192,8 +192,8 @@ enum {
 
 static int build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
 {
-    g_autofree CDATSslbis *sslbis_latency = NULL;
-    g_autofree CDATSslbis *sslbis_bandwidth = NULL;
+    CDATSslbis *sslbis_latency;
+    CDATSslbis *sslbis_bandwidth;
     CXLUpstreamPort *us = CXL_USP(priv);
     PCIBus *bus = &PCI_BRIDGE(us)->sec_bus;
     int devfn, sslbis_size, i;
@@ -270,8 +270,8 @@ static int build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
     *cdat_table = g_new0(CDATSubHeader *, CXL_USP_CDAT_NUM_ENTRIES);
 
     /* Header always at start of structure */
-    (*cdat_table)[CXL_USP_CDAT_SSLBIS_LAT] = g_steal_pointer(&sslbis_latency);
-    (*cdat_table)[CXL_USP_CDAT_SSLBIS_BW] = g_steal_pointer(&sslbis_bandwidth);
+    (*cdat_table)[CXL_USP_CDAT_SSLBIS_LAT] = (CDATSubHeader *)sslbis_latency;
+    (*cdat_table)[CXL_USP_CDAT_SSLBIS_BW] = (CDATSubHeader *)sslbis_bandwidth;
 
     return CXL_USP_CDAT_NUM_ENTRIES;
 }
-- 
2.44.0

Re: [PATCH 2/3] hw/pci-bridge/cxl_upstream: Fix problem with g_steal_pointer()

[Jonathan Cameron via]

On Mon,  4 Mar 2024 11:44:05 +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 build_cdat_table() 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).

Left over of an earlier cleanup where I failed to notice there were no
longer any error return paths. Great to clean this out.

> Let's 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).

Ok. In these cases indeed seem to be fine unpacked. That's
not in general true of the CXL spec structures.
Maybe alternative if we run into problems with future versions
of these structures will be to define struct CDATSubHeader as packed.

Still we can cross that bridge when we come to it.

> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  include/hw/cxl/cxl_cdat.h    | 8 +++++---
>  hw/pci-bridge/cxl_upstream.c | 8 ++++----
>  2 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/include/hw/cxl/cxl_cdat.h b/include/hw/cxl/cxl_cdat.h
> index 8e3d094608..b44cefaad6 100644
> --- a/include/hw/cxl/cxl_cdat.h
> +++ b/include/hw/cxl/cxl_cdat.h
> @@ -130,7 +130,8 @@ typedef struct CDATSslbisHeader {
>      uint8_t data_type;
>      uint8_t reserved[3];
>      uint64_t entry_base_unit;
> -} QEMU_PACKED CDATSslbisHeader;
> +} CDATSslbisHeader;
> +QEMU_BUILD_BUG_ON(sizeof(CDATSslbisHeader) != 16);
>  
>  #define CDAT_PORT_ID_USP 0x100
>  /* Switch Scoped Latency and Bandwidth Entry - CDAT Table 10 */
> @@ -139,12 +140,13 @@ typedef struct CDATSslbe {
>      uint16_t port_y_id;
>      uint16_t latency_bandwidth;
>      uint16_t reserved;
> -} QEMU_PACKED CDATSslbe;
> +} CDATSslbe;
> +QEMU_BUILD_BUG_ON(sizeof(CDATSslbe) != 8);
>  
>  typedef struct CDATSslbis {
>      CDATSslbisHeader sslbis_header;
>      CDATSslbe sslbe[];
> -} QEMU_PACKED CDATSslbis;
> +} CDATSslbis;
>  
>  typedef struct CDATEntry {
>      void *base;
> diff --git a/hw/pci-bridge/cxl_upstream.c b/hw/pci-bridge/cxl_upstream.c
> index e87eb40177..537f9affb8 100644
> --- a/hw/pci-bridge/cxl_upstream.c
> +++ b/hw/pci-bridge/cxl_upstream.c
> @@ -192,8 +192,8 @@ enum {
>  
>  static int build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
>  {
> -    g_autofree CDATSslbis *sslbis_latency = NULL;
> -    g_autofree CDATSslbis *sslbis_bandwidth = NULL;
> +    CDATSslbis *sslbis_latency;
> +    CDATSslbis *sslbis_bandwidth;
>      CXLUpstreamPort *us = CXL_USP(priv);
>      PCIBus *bus = &PCI_BRIDGE(us)->sec_bus;
>      int devfn, sslbis_size, i;
> @@ -270,8 +270,8 @@ static int build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
>      *cdat_table = g_new0(CDATSubHeader *, CXL_USP_CDAT_NUM_ENTRIES);
>  
>      /* Header always at start of structure */
> -    (*cdat_table)[CXL_USP_CDAT_SSLBIS_LAT] = g_steal_pointer(&sslbis_latency);
> -    (*cdat_table)[CXL_USP_CDAT_SSLBIS_BW] = g_steal_pointer(&sslbis_bandwidth);
> +    (*cdat_table)[CXL_USP_CDAT_SSLBIS_LAT] = (CDATSubHeader *)sslbis_latency;
> +    (*cdat_table)[CXL_USP_CDAT_SSLBIS_BW] = (CDATSubHeader *)sslbis_bandwidth;
>  
>      return CXL_USP_CDAT_NUM_ENTRIES;
>  }

Re: [PATCH 2/3] hw/pci-bridge/cxl_upstream: Fix problem with g_steal_pointer()

[Jonathan Cameron via]

On Mon, 4 Mar 2024 15:06:51 +0000
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Mon,  4 Mar 2024 11:44:05 +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 build_cdat_table() 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).  
> 
> Left over of an earlier cleanup where I failed to notice there were no
> longer any error return paths. Great to clean this out.
> 
> > Let's 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).  
> 
> Ok. In these cases indeed seem to be fine unpacked. That's
> not in general true of the CXL spec structures.
> Maybe alternative if we run into problems with future versions
> of these structures will be to define struct CDATSubHeader as packed.
> 
> Still we can cross that bridge when we come to it.
I notice in next patch we could just assign the pointer to the contained
structure header.  Maybe a cleaner solution and would make it clear
why it is valid to assign this lot to a pointer of the
CDATSubHeader type. 

Jonathan


> 
> > 
> > Signed-off-by: Thomas Huth <thuth@redhat.com>  
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> > ---
> >  include/hw/cxl/cxl_cdat.h    | 8 +++++---
> >  hw/pci-bridge/cxl_upstream.c | 8 ++++----
> >  2 files changed, 9 insertions(+), 7 deletions(-)
> > 
> > diff --git a/include/hw/cxl/cxl_cdat.h b/include/hw/cxl/cxl_cdat.h
> > index 8e3d094608..b44cefaad6 100644
> > --- a/include/hw/cxl/cxl_cdat.h
> > +++ b/include/hw/cxl/cxl_cdat.h
> > @@ -130,7 +130,8 @@ typedef struct CDATSslbisHeader {
> >      uint8_t data_type;
> >      uint8_t reserved[3];
> >      uint64_t entry_base_unit;
> > -} QEMU_PACKED CDATSslbisHeader;
> > +} CDATSslbisHeader;
> > +QEMU_BUILD_BUG_ON(sizeof(CDATSslbisHeader) != 16);
> >  
> >  #define CDAT_PORT_ID_USP 0x100
> >  /* Switch Scoped Latency and Bandwidth Entry - CDAT Table 10 */
> > @@ -139,12 +140,13 @@ typedef struct CDATSslbe {
> >      uint16_t port_y_id;
> >      uint16_t latency_bandwidth;
> >      uint16_t reserved;
> > -} QEMU_PACKED CDATSslbe;
> > +} CDATSslbe;
> > +QEMU_BUILD_BUG_ON(sizeof(CDATSslbe) != 8);
> >  
> >  typedef struct CDATSslbis {
> >      CDATSslbisHeader sslbis_header;
> >      CDATSslbe sslbe[];
> > -} QEMU_PACKED CDATSslbis;
> > +} CDATSslbis;
> >  
> >  typedef struct CDATEntry {
> >      void *base;
> > diff --git a/hw/pci-bridge/cxl_upstream.c b/hw/pci-bridge/cxl_upstream.c
> > index e87eb40177..537f9affb8 100644
> > --- a/hw/pci-bridge/cxl_upstream.c
> > +++ b/hw/pci-bridge/cxl_upstream.c
> > @@ -192,8 +192,8 @@ enum {
> >  
> >  static int build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
> >  {
> > -    g_autofree CDATSslbis *sslbis_latency = NULL;
> > -    g_autofree CDATSslbis *sslbis_bandwidth = NULL;
> > +    CDATSslbis *sslbis_latency;
> > +    CDATSslbis *sslbis_bandwidth;
> >      CXLUpstreamPort *us = CXL_USP(priv);
> >      PCIBus *bus = &PCI_BRIDGE(us)->sec_bus;
> >      int devfn, sslbis_size, i;
> > @@ -270,8 +270,8 @@ static int build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
> >      *cdat_table = g_new0(CDATSubHeader *, CXL_USP_CDAT_NUM_ENTRIES);
> >  
> >      /* Header always at start of structure */
> > -    (*cdat_table)[CXL_USP_CDAT_SSLBIS_LAT] = g_steal_pointer(&sslbis_latency);
> > -    (*cdat_table)[CXL_USP_CDAT_SSLBIS_BW] = g_steal_pointer(&sslbis_bandwidth);
> > +    (*cdat_table)[CXL_USP_CDAT_SSLBIS_LAT] = (CDATSubHeader *)sslbis_latency;
> > +    (*cdat_table)[CXL_USP_CDAT_SSLBIS_BW] = (CDATSubHeader *)sslbis_bandwidth;
> >  
> >      return CXL_USP_CDAT_NUM_ENTRIES;
> >  }  
> 

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

[Thomas Huth]

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).

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;
+    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)
-- 
2.44.0

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

[Jonathan Cameron via]

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)

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

[Thomas Huth]

On 04/03/2024 16.10, Jonathan Cameron wrote:
> 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?

I gave it a try, but it does not help against the alignment issue, I still get:

../../devel/qemu/hw/mem/cxl_type3.c: In function 
‘ct3_build_cdat_entries_for_mr’:
../../devel/qemu/hw/mem/cxl_type3.c:138:34: error: taking address of packed 
member of ‘struct CDATDsmas’ may result in an unaligned pointer value 
[-Werror=address-of-packed-member]
   138 |     cdat_table[CT3_CDAT_DSMAS] = &dsmas->header;
       |                                  ^~~~~~~~~~~~~~

 From my experience, it's better anyway to avoid __attribute__((packed)) on 
structures unless it is really really required. At least we should avoid it 
as good as possible as long as we still support running QEMU on Sparc hosts 
(that don't support misaligned memory accesses), since otherwise you can end 
up with non-working code there, see e.g.:

  https://www.mail-archive.com/qemu-devel@nongnu.org/msg439899.html

or:

  https://gitlab.com/qemu-project/qemu/-/commit/cb89b349074310ff9eb7ebe18a

Thus I'd rather prefer to keep this patch as it is right now.

  Thomas

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

[Jonathan Cameron via]

On Tue, 5 Mar 2024 08:27:52 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 04/03/2024 16.10, Jonathan Cameron wrote:
> > 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?  
> 
> I gave it a try, but it does not help against the alignment issue, I still get:
> 
> ../../devel/qemu/hw/mem/cxl_type3.c: In function 
> ‘ct3_build_cdat_entries_for_mr’:
> ../../devel/qemu/hw/mem/cxl_type3.c:138:34: error: taking address of packed 
> member of ‘struct CDATDsmas’ may result in an unaligned pointer value 
> [-Werror=address-of-packed-member]
>    138 |     cdat_table[CT3_CDAT_DSMAS] = &dsmas->header;
>        |                                  ^~~~~~~~~~~~~~
> 
>  From my experience, it's better anyway to avoid __attribute__((packed)) on 
> structures unless it is really really required. At least we should avoid it 
> as good as possible as long as we still support running QEMU on Sparc hosts 
> (that don't support misaligned memory accesses), since otherwise you can end 
> up with non-working code there, see e.g.:
> 
>   https://www.mail-archive.com/qemu-devel@nongnu.org/msg439899.html
> 
> or:
> 
>   https://gitlab.com/qemu-project/qemu/-/commit/cb89b349074310ff9eb7ebe18a
> 
> Thus I'd rather prefer to keep this patch as it is right now.
> 
>   Thomas
Fair enough.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> 

Re: [PATCH 0/3] cxl: Fix issues with g_steal_pointer()

[Michael Tokarev]

04.03.2024 13:44, Thomas Huth wrote:
> When setting GLIB_VERSION_MAX_ALLOWED to GLIB_VERSION_2_58 or higher
> (which we'll certainly do in the not too distant future), glib adds
> type safety checks to the g_steal_pointer() macro. This triggers
> errors in the cxl code since the pointer types do not always match
> here. Let's fix those errors now so we can switch to a newer version
> of the glib in a future version of QEMU.

Picked up for qemu-trivial, thank you!

/mjt