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

[Richard Henderson]

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.

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?


r~