Re: [Qemu-devel] [PATCH] cpu_defs: Simplify CPUTLB padding logic

[Peter Crosthwaite]

On Mon, Jul 6, 2015 at 1:43 AM, Paolo Bonzini <address@hidden> wrote:
>
>
> On 05/07/2015 23:08, Peter Crosthwaite wrote:
>> There was a complicated subtractive arithmetic for determining the
>> padding on the CPUTLBEntry structure. Simplify this with a union.
>>
>> Signed-off-by: Peter Crosthwaite <address@hidden>
>> ---
>>  include/exec/cpu-defs.h | 23 ++++++++++++-----------
>>  1 file changed, 12 insertions(+), 11 deletions(-)
>>
>> diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h
>> index 98b9cff..5093be2 100644
>> --- a/include/exec/cpu-defs.h
>> +++ b/include/exec/cpu-defs.h
>> @@ -105,17 +105,18 @@ typedef struct CPUTLBEntry {
>>         bit 3                      : indicates that the entry is invalid
>>         bit 2..0                   : zero
>>      */
>> -    target_ulong addr_read;
>> -    target_ulong addr_write;
>> -    target_ulong addr_code;
>> -    /* Addend to virtual address to get host address.  IO accesses
>> -       use the corresponding iotlb value.  */
>> -    uintptr_t addend;
>> -    /* padding to get a power of two size */
>> -    uint8_t dummy[(1 << CPU_TLB_ENTRY_BITS) -
>> -                  (sizeof(target_ulong) * 3 +
>> -                   ((-sizeof(target_ulong) * 3) & (sizeof(uintptr_t) - 1)) +
>> -                   sizeof(uintptr_t))];
>> +    union {
>
> The struct CPUTLBEntry can be changed to union CPUTLBEntry directly,
> with no need for the anonymous struct.
>

True. Code gets even simpler! :)

>> +        struct {
>> +            target_ulong addr_read;
>> +            target_ulong addr_write;
>> +            target_ulong addr_code;
>> +            /* Addend to virtual address to get host address.  IO accesses
>> +               use the corresponding iotlb value.  */
>> +            uintptr_t addend;
>> +        };
>
> Which compiler version started implementing anonymous structs?
>

ISO C11 standardises it apparently. But various parts of the tree use
them now. target-arm/cpu.h, target-i386/kvm.c,
linux-user/syscall_defs.h and linux-headers/linux/kvm.h have liberal
use to name a few. I have seen it used in devs from time to time for
unionifying individual registers with an array form for SW access.

This led me to consider it open season on anonymous structs and unions.

> Or can we just add
>
>          __attribute__((__aligned__(1 << CPU_TLB_ENTRY_BITS)))
>

Is that more or less standard than anonymous structs?

Regards,
Peter

> to the struct?  I'm not sure if it affects the sizeof too, so that
> requires some care.  Alternatively, an
>
>         uint8_t padding[0]
>                 __attribute__((__aligned__(1 << CPU_TLB_ENTRY_BITS)));
>
> could maybe work?  Neither is exactly the same, as they also bump the
> alignment of the overall struct, but they do not require anonymous structs.
>
> Paolo
>
>> +        /* padding to get a power of two size */
>> +        uint8_t dummy[1 << CPU_TLB_ENTRY_BITS];
>> +    };
>>  } CPUTLBEntry;
>>
>>  QEMU_BUILD_BUG_ON(sizeof(CPUTLBEntry) != (1 << CPU_TLB_ENTRY_BITS));
>>
>