[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/5] qom: Allow objects to be allocated with increased alignm
From: |
Richard Henderson |
Subject: |
Re: [PATCH 1/5] qom: Allow objects to be allocated with increased alignment |
Date: |
Tue, 15 Sep 2020 12:09:59 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 |
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~
- [PATCH 0/5] qom: Allow object to be aligned, Richard Henderson, 2020/09/15
- [PATCH 3/5] target/ppc: Set instance_align on PowerPCCPU TypeInfo, Richard Henderson, 2020/09/15
- [PATCH 2/5] target/arm: Set instance_align on CPUARM TypeInfo, Richard Henderson, 2020/09/15
- [PATCH 1/5] qom: Allow objects to be allocated with increased alignment, Richard Henderson, 2020/09/15
- Re: [PATCH 1/5] qom: Allow objects to be allocated with increased alignment, Eduardo Habkost, 2020/09/15
- Re: [PATCH 1/5] qom: Allow objects to be allocated with increased alignment,
Richard Henderson <=
- Re: [PATCH 1/5] qom: Allow objects to be allocated with increased alignment, Eduardo Habkost, 2020/09/15
- Re: [PATCH 1/5] qom: Allow objects to be allocated with increased alignment, Richard Henderson, 2020/09/15
- Re: [PATCH 1/5] qom: Allow objects to be allocated with increased alignment, Eduardo Habkost, 2020/09/15
- Re: [PATCH 1/5] qom: Allow objects to be allocated with increased alignment, Richard Henderson, 2020/09/15
- Re: [PATCH 1/5] qom: Allow objects to be allocated with increased alignment, Eduardo Habkost, 2020/09/15
[PATCH 4/5] target/riscv: Set instance_align on RISCVCPU TypeInfo, Richard Henderson, 2020/09/15
[PATCH 5/5] target/s390x: Set instance_align on S390CPU TypeInfo, Richard Henderson, 2020/09/15
Re: [PATCH 0/5] qom: Allow object to be aligned, Richard Henderson, 2020/09/15