On Mon, 4 Mar 2024 15:06:51 +0000
Jonathan Cameron <jonathan.came...@huawei.com> wrote:
> On Mon, 4 Mar 2024 11:44:05 +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 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 <th...@redhat.com>
> Reviewed-by: Jonathan Cameron <jonathan.came...@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;
> > }
>
