[PATCH 0/5] qom: Allow object to be aligned

[PATCH 0/5] qom: Allow object to be aligned

[Richard Henderson]

I've seen some failures on arm and s390x hosts after
enabling host vector support.  It turns out that the
malloc for these hosts does not provide 16-byte alignment.

We already have a function that can alloc with alignment,
but we need to pass this down from the structure.  We also
don't want to use this function unconditionally, because
the windows version does page allocation, which would be
overkill for the vast majority of the objects allocated.


r~


Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Daniel P. Berrangé" <berrange@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: Alistair Francis <Alistair.Francis@wdc.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Cc: qemu-arm@nongnu.org
Cc: qemu-ppc@nongnu.org
Cc: qemu-riscv@nongnu.org
Cc: qemu-s390x@nongnu.org


Richard Henderson (5):
  qom: Allow objects to be allocated with increased alignment
  target/arm: Set instance_align on CPUARM TypeInfo
  target/ppc: Set instance_align on PowerPCCPU TypeInfo
  target/riscv: Set instance_align on RISCVCPU TypeInfo
  target/s390x: Set instance_align on S390CPU TypeInfo

 include/qom/object.h            |  4 ++++
 qom/object.c                    | 16 +++++++++++++---
 target/arm/cpu.c                |  2 ++
 target/riscv/cpu.c              |  1 +
 target/s390x/cpu.c              |  1 +
 target/ppc/translate_init.c.inc |  1 +
 6 files changed, 22 insertions(+), 3 deletions(-)

-- 
2.25.1

[PATCH 0/5] qom: Allow object to be aligned

[Richard Henderson]

I've seen some failures on arm and s390x hosts after
enabling host vector support.  It turns out that the
malloc for these hosts does not provide 16-byte alignment.

We already have a function that can alloc with alignment,
but we need to pass this down from the structure.  We also
don't want to use this function unconditionally, because
the windows version does page allocation, which would be
overkill for the vast majority of the objects allocated.


r~


Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Daniel P. Berrangé" <berrange@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: Alistair Francis <Alistair.Francis@wdc.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Cc: qemu-arm@nongnu.org
Cc: qemu-ppc@nongnu.org
Cc: qemu-riscv@nongnu.org
Cc: qemu-s390x@nongnu.org


Richard Henderson (5):
  qom: Allow objects to be allocated with increased alignment
  target/arm: Set instance_align on CPUARM TypeInfo
  target/ppc: Set instance_align on PowerPCCPU TypeInfo
  target/riscv: Set instance_align on RISCVCPU TypeInfo
  target/s390x: Set instance_align on S390CPU TypeInfo

 include/qom/object.h            |  4 ++++
 qom/object.c                    | 16 +++++++++++++---
 target/arm/cpu.c                |  2 ++
 target/riscv/cpu.c              |  1 +
 target/s390x/cpu.c              |  1 +
 target/ppc/translate_init.c.inc |  1 +
 6 files changed, 22 insertions(+), 3 deletions(-)

-- 
2.25.1

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

[Richard Henderson]

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) {
+        obj = qemu_memalign(align, size);
+    } else {
+        obj = g_malloc(size);
+    }
+
+    object_initialize_with_type(obj, size, type);
+    obj->free = (align ? qemu_vfree : g_free);
 
     return obj;
 }
-- 
2.25.1

[PATCH 2/5] target/arm: Set instance_align on CPUARM TypeInfo

[Richard Henderson]

Fix alignment of CPUARMState.vfp.zregs.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-arm@nongnu.org
---
 target/arm/cpu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 7b5ea65fab..a7643deab4 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -2290,6 +2290,7 @@ void arm_cpu_register(const ARMCPUInfo *info)
     TypeInfo type_info = {
         .parent = TYPE_ARM_CPU,
         .instance_size = sizeof(ARMCPU),
+        .instance_align = __alignof__(ARMCPU),
         .instance_init = arm_cpu_instance_init,
         .class_size = sizeof(ARMCPUClass),
         .class_init = info->class_init ?: cpu_register_class_init,
@@ -2305,6 +2306,7 @@ static const TypeInfo arm_cpu_type_info = {
     .name = TYPE_ARM_CPU,
     .parent = TYPE_CPU,
     .instance_size = sizeof(ARMCPU),
+    .instance_align = __alignof__(ARMCPU),
     .instance_init = arm_cpu_initfn,
     .instance_finalize = arm_cpu_finalizefn,
     .abstract = true,
-- 
2.25.1

[PATCH 3/5] target/ppc: Set instance_align on PowerPCCPU TypeInfo

[Richard Henderson]

Fix alignment of CPUPPCState.vsr.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: qemu-ppc@nongnu.org
---
 target/ppc/translate_init.c.inc | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/ppc/translate_init.c.inc b/target/ppc/translate_init.c.inc
index 230a062d29..accb4f2fae 100644
--- a/target/ppc/translate_init.c.inc
+++ b/target/ppc/translate_init.c.inc
@@ -10960,6 +10960,7 @@ static const TypeInfo ppc_cpu_type_info = {
     .name = TYPE_POWERPC_CPU,
     .parent = TYPE_CPU,
     .instance_size = sizeof(PowerPCCPU),
+    .instance_align = __alignof__(PowerPCCPU),
     .instance_init = ppc_cpu_instance_init,
     .instance_finalize = ppc_cpu_instance_finalize,
     .abstract = true,
-- 
2.25.1

[PATCH 4/5] target/riscv: Set instance_align on RISCVCPU TypeInfo

[Richard Henderson]

Fix alignment of CPURISCVState.vreg.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
Cc: Alistair Francis <Alistair.Francis@wdc.com>
Cc: qemu-riscv@nongnu.org
---
 target/riscv/cpu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 57c006df5d..0bbfd7f457 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -628,6 +628,7 @@ static const TypeInfo riscv_cpu_type_infos[] = {
         .name = TYPE_RISCV_CPU,
         .parent = TYPE_CPU,
         .instance_size = sizeof(RISCVCPU),
+        .instance_align = __alignof__(RISCVCPU),
         .instance_init = riscv_cpu_init,
         .abstract = true,
         .class_size = sizeof(RISCVCPUClass),
-- 
2.25.1

[PATCH 4/5] target/riscv: Set instance_align on RISCVCPU TypeInfo

[Richard Henderson]

Fix alignment of CPURISCVState.vreg.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
Cc: Alistair Francis <Alistair.Francis@wdc.com>
Cc: qemu-riscv@nongnu.org
---
 target/riscv/cpu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 57c006df5d..0bbfd7f457 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -628,6 +628,7 @@ static const TypeInfo riscv_cpu_type_infos[] = {
         .name = TYPE_RISCV_CPU,
         .parent = TYPE_CPU,
         .instance_size = sizeof(RISCVCPU),
+        .instance_align = __alignof__(RISCVCPU),
         .instance_init = riscv_cpu_init,
         .abstract = true,
         .class_size = sizeof(RISCVCPUClass),
-- 
2.25.1

[PATCH 5/5] target/s390x: Set instance_align on S390CPU TypeInfo

[Richard Henderson]

Fix alignment of CPUS390XState.vregs.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
Cc: David Hildenbrand <david@redhat.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Cc: qemu-s390x@nongnu.org
---
 target/s390x/cpu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 749cd548f0..e350edc9f5 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -517,6 +517,7 @@ static const TypeInfo s390_cpu_type_info = {
     .name = TYPE_S390_CPU,
     .parent = TYPE_CPU,
     .instance_size = sizeof(S390CPU),
+    .instance_align = __alignof__(S390CPU),
     .instance_init = s390_cpu_initfn,
     .instance_finalize = s390_cpu_finalize,
     .abstract = true,
-- 
2.25.1

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

[Eduardo Habkost]

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*.

> +        obj = qemu_memalign(align, size);
> +    } else {
> +        obj = g_malloc(size);
> +    }
> +
> +    object_initialize_with_type(obj, size, type);
> +    obj->free = (align ? qemu_vfree : g_free);
>  
>      return obj;
>  }
> -- 
> 2.25.1
> 

-- 
Eduardo

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~

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

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

[Richard Henderson]

On 9/15/20 1:19 PM, Eduardo Habkost wrote:
> Once we move to C11, we can just use max_align_t.

Yes.

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

For i386, this is 4.


r~

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

[Eduardo Habkost]

On Tue, Sep 15, 2020 at 01:51:48PM -0700, Richard Henderson wrote:
> On 9/15/20 1:19 PM, Eduardo Habkost wrote:
> > Once we move to C11, we can just use max_align_t.
> 
> Yes.
> 
> > While we don't move to C11, why not just use
> >   __alignof__(union { long l; void *p; double d; long double ld;})
> > ?
> 
> For i386, this is 4.

Is i386-linux the only case where there are additional alignment
guarantees not covered by C99?

I would prefer a i386-linux-specific #ifdef for that case instead
of guessing based on undocumented libc internals.

-- 
Eduardo

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

[Richard Henderson]

On 9/15/20 2:27 PM, Eduardo Habkost wrote:
> On Tue, Sep 15, 2020 at 01:51:48PM -0700, Richard Henderson wrote:
>> On 9/15/20 1:19 PM, Eduardo Habkost wrote:
>>> Once we move to C11, we can just use max_align_t.
>>
>> Yes.
>>
>>> While we don't move to C11, why not just use
>>>   __alignof__(union { long l; void *p; double d; long double ld;})
>>> ?
>>
>> For i386, this is 4.
> 
> Is i386-linux the only case where there are additional alignment
> guarantees not covered by C99?

I think so.

> I would prefer a i386-linux-specific #ifdef for that case instead
> of guessing based on undocumented libc internals.

I was thinking abi, not internals.


r~

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

[Eduardo Habkost]

On Tue, Sep 15, 2020 at 02:30:34PM -0700, Richard Henderson wrote:
> On 9/15/20 2:27 PM, Eduardo Habkost wrote:
> > On Tue, Sep 15, 2020 at 01:51:48PM -0700, Richard Henderson wrote:
> >> On 9/15/20 1:19 PM, Eduardo Habkost wrote:
> >>> Once we move to C11, we can just use max_align_t.
> >>
> >> Yes.
> >>
> >>> While we don't move to C11, why not just use
> >>>   __alignof__(union { long l; void *p; double d; long double ld;})
> >>> ?
> >>
> >> For i386, this is 4.
> > 
> > Is i386-linux the only case where there are additional alignment
> > guarantees not covered by C99?
> 
> I think so.
> 
> > I would prefer a i386-linux-specific #ifdef for that case instead
> > of guessing based on undocumented libc internals.
> 
> I was thinking abi, not internals.

I see.  As long as we can point to the specification backing the
assumptions in the code, I'm OK with that.

To me, it seems simpler to start with something that works on all
hosts (the alignment guarantees provided by C99), and add
arch-specific optimizations later.

-- 
Eduardo

Re: [PATCH 0/5] qom: Allow object to be aligned

[Richard Henderson]

On 9/15/20 10:46 AM, Richard Henderson wrote:
> We already have a function that can alloc with alignment,
> but we need to pass this down from the structure.  We also
> don't want to use this function unconditionally, because
> the windows version does page allocation, which would be
> overkill for the vast majority of the objects allocated.

Stefan, why are we using VirtualAlloc in util/oslib-win32.c, qemu_try_memalign,
instead of _aligned_malloc?


r~

Re: [PATCH 0/5] qom: Allow object to be aligned

[Stefan Weil]

Am 16.09.20 um 00:47 schrieb Richard Henderson:

> On 9/15/20 10:46 AM, Richard Henderson wrote:
>> We already have a function that can alloc with alignment,
>> but we need to pass this down from the structure.  We also
>> don't want to use this function unconditionally, because
>> the windows version does page allocation, which would be
>> overkill for the vast majority of the objects allocated.
> Stefan, why are we using VirtualAlloc in util/oslib-win32.c, qemu_try_memalign,
> instead of _aligned_malloc?


I think we use it because Fabrice introduced VirtualAlloc in commit
6e4255f6a65091fbe7d17bfda546e2aa1b72f9a6 (with a comment "FIXME: this is
not exactly optimal solution [...]"). Maybe there was no _aligned_malloc
available 15 years ago. Commit 33f002714be2ed58ed05ae3870d5ea6915df4b47
then reused VirtualAlloc for qemu_try_memalign (without a FIXME comment).

Obviously nobody cared to find a better solution until now.

Stefan