| Message ID | 20240304104406.59855-3-thuth@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cxl: Fix issues with g_steal_pointer() | expand |
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; > }
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; > > } >
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; }
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(-)