Re: [PATCH 1/5] qom: Allow objects to be allocated with increased alignment

[Eduardo Habkost]

On Tue, Sep 15, 2020 at 12:09:59PM -0700, Richard Henderson wrote:
> On 9/15/20 11:07 AM, Eduardo Habkost wrote:
> > On Tue, Sep 15, 2020 at 10:46:31AM -0700, Richard Henderson wrote:
> >> It turns out that some hosts have a default malloc alignment less
> >> than that required for vectors.
> >>
> >> We assume that, with compiler annotation on CPUArchState, that we
> >> can properly align the vector portion of the guest state.  Fix the
> >> alignment of the allocation by using qemu_memalloc when required.
> >>
> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> >> ---
> >> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >> Cc: "Daniel P. Berrangé" <berrange@redhat.com>
> >> Cc: Eduardo Habkost <ehabkost@redhat.com>
> >> ---
> >>  include/qom/object.h |  4 ++++
> >>  qom/object.c         | 16 +++++++++++++---
> >>  2 files changed, 17 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/include/qom/object.h b/include/qom/object.h
> >> index 056f67ab3b..d52d0781a3 100644
> >> --- a/include/qom/object.h
> >> +++ b/include/qom/object.h
> >> @@ -770,6 +770,9 @@ struct Object
> >>   * @instance_size: The size of the object (derivative of #Object).  If
> >>   *   @instance_size is 0, then the size of the object will be the size of 
> >> the
> >>   *   parent object.
> >> + * @instance_align: The required alignment of the object.  If 
> >> @instance_align
> >> + *   is 0, then normal malloc alignment is sufficient; if non-zero, then 
> >> we
> >> + *   must use qemu_memalign for allocation.
> >>   * @instance_init: This function is called to initialize an object.  The 
> >> parent
> >>   *   class will have already been initialized so the type is only 
> >> responsible
> >>   *   for initializing its own members.
> >> @@ -807,6 +810,7 @@ struct TypeInfo
> >>      const char *parent;
> >>  
> >>      size_t instance_size;
> >> +    size_t instance_align;
> >>      void (*instance_init)(Object *obj);
> >>      void (*instance_post_init)(Object *obj);
> >>      void (*instance_finalize)(Object *obj);
> >> diff --git a/qom/object.c b/qom/object.c
> >> index 387efb25eb..2e53cb44a6 100644
> >> --- a/qom/object.c
> >> +++ b/qom/object.c
> >> @@ -50,6 +50,7 @@ struct TypeImpl
> >>      size_t class_size;
> >>  
> >>      size_t instance_size;
> >> +    size_t instance_align;
> >>  
> >>      void (*class_init)(ObjectClass *klass, void *data);
> >>      void (*class_base_init)(ObjectClass *klass, void *data);
> >> @@ -114,6 +115,7 @@ static TypeImpl *type_new(const TypeInfo *info)
> >>  
> >>      ti->class_size = info->class_size;
> >>      ti->instance_size = info->instance_size;
> >> +    ti->instance_align = info->instance_align;
> >>  
> >>      ti->class_init = info->class_init;
> >>      ti->class_base_init = info->class_base_init;
> >> @@ -691,13 +693,21 @@ static void object_finalize(void *data)
> >>  static Object *object_new_with_type(Type type)
> >>  {
> >>      Object *obj;
> >> +    size_t size, align;
> >>  
> >>      g_assert(type != NULL);
> >>      type_initialize(type);
> >>  
> >> -    obj = g_malloc(type->instance_size);
> >> -    object_initialize_with_type(obj, type->instance_size, type);
> >> -    obj->free = g_free;
> >> +    size = type->instance_size;
> >> +    align = type->instance_align;
> >> +    if (align) {
> > 
> > If we check for (align > G_MEM_ALIGN) instead, we will be able to
> > set instance_align automatically at OBJECT_DEFINE_TYPE*.
> 
> I agree a value check would be good here, as well as setting this by default.
> 
> As for the value check itself...
> 
> I see that G_MEM_ALIGN isn't actually defined in an interesting or even 
> correct
> way.  E.g. it doesn't take the long double type into account.
> 
> The usual mechanism is
> 
> struct s {
>   char pad;
>   union {
>     long l;
>     void *p;
>     double d;
>     long double ld;
>   } u;
> };
> 
> offsetof(s, u)
> 
> since all of these types are required to be taken into account by the system
> malloc.

Due to the existence of G_MEM_ALIGN, I thought GLib alignment
guarantees could be weaker than malloc().  I've just learned that
GLib uses system malloc() since 2.46.

> 
> E.g it doesn't take other host guarantees into account, e.g. i386-linux
> guarantees 16-byte alignment.  This possibly dubious ABI change was made 20+
> years ago with the introduction of SSE and is now set in stone.
> 
> Glibc has a "malloc-alignment.h" internal header that defaults to
> 
>   MIN(2 * sizeof(size_t), __alignof__(long double))
> 
> and is overridden for i386.  Sadly, it doesn't export MALLOC_ALIGNMENT.
> 
> Musl has two different malloc implementations.  One has UNIT = 16; the other
> has SIZE_ALIGN = 4*sizeof(size_t).  Both have a minimum value of 16, and this
> is not target-specific.
> 
> Any further comments on the subject, or should I put together something that
> computes the MAX of the above?

Once we move to C11, we can just use max_align_t.

While we don't move to C11, why not just use
  __alignof__(union { long l; void *p; double d; long double ld;})
?

-- 
Eduardo