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

[Jonathan Cameron]

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>

>