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

[Thomas Huth <thuth@redhat.com>]

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