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

[Jonathan Cameron via <qemu-devel@nongnu.org>]

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;
> >  }  
>