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

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

[Peter Crosthwaite]

There was a complicated subtractive arithmetic for determining the
padding on the CPUTLBEntry structure. Simplify this with a union.

Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
 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 {
+        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;
+        };
+        /* 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));
-- 
1.9.1

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

[Paolo Bonzini]

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 <crosthwaite.peter@gmail.com>
> ---
>  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.

> +        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?

Or can we just add

	 __attribute__((__aligned__(1 << CPU_TLB_ENTRY_BITS)))

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));
> 

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

[Peter Crosthwaite]

On Mon, Jul 6, 2015 at 1:43 AM, Paolo Bonzini <pbonzini@redhat.com> 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 <crosthwaite.peter@gmail.com>
>> ---
>>  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));
>>
>

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

[Paolo Bonzini]

On 06/07/2015 10:58, Peter Crosthwaite wrote:
>> > 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.

Ok, I wasn't sure about anonymous structs.  We definitely use anonymous
unions a lot.

>> > Or can we just add
>> >
>> >          __attribute__((__aligned__(1 << CPU_TLB_ENTRY_BITS)))
>> >
> Is that more or less standard than anonymous structs?

Not standard (though there is something in C11 too) but decades old as a
GCC extension.

Paolo

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

[Richard Henderson]

On 07/06/2015 09:43 AM, Paolo Bonzini 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 <crosthwaite.peter@gmail.com>
>> ---
>>   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.

Um, no it can't.  That would put all of the members at the same address.

> Which compiler version started implementing anonymous structs?

A long long time ago -- gcc 2 era.

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

The structure isn't currently aligned, and it needn't be.  We only need the 
size to be a power of two for the addressing.



r~

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

[Paolo Bonzini]

On 06/07/2015 13:42, Richard Henderson wrote:
> On 07/06/2015 09:43 AM, Paolo Bonzini 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 <crosthwaite.peter@gmail.com>
>>> ---
>>>   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.
> 
> Um, no it can't.  That would put all of the members at the same address.

Of course. :-(  With no need for the anonymous _union_.  *blush*.

>> Which compiler version started implementing anonymous structs?
> 
> A long long time ago -- gcc 2 era.

Great.  I now remember that the recent feature is anonymous tagged
structs, coming from the Plan 9 compiler.

Paolo

>> Or can we just add
>>
>>      __attribute__((__aligned__(1 << CPU_TLB_ENTRY_BITS)))
> 
> The structure isn't currently aligned, and it needn't be.  We only need
> the size to be a power of two for the addressing.
> 
> 
> 
> r~

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

[Peter Crosthwaite]

On Mon, Jul 6, 2015 at 4:52 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 06/07/2015 13:42, Richard Henderson wrote:
>> On 07/06/2015 09:43 AM, Paolo Bonzini 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 <crosthwaite.peter@gmail.com>
>>>> ---
>>>>   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.
>>
>> Um, no it can't.  That would put all of the members at the same address.
>
> Of course. :-(  With no need for the anonymous _union_.  *blush*.
>

Yeh this is what I assumed you meant. You still need one anonymous
struct, but it saves one level of indent and one less anonymous thing.

Regards,
Peter

>>> Which compiler version started implementing anonymous structs?
>>
>> A long long time ago -- gcc 2 era.
>
> Great.  I now remember that the recent feature is anonymous tagged
> structs, coming from the Plan 9 compiler.
>
> Paolo
>
>>> Or can we just add
>>>
>>>      __attribute__((__aligned__(1 << CPU_TLB_ENTRY_BITS)))
>>
>> The structure isn't currently aligned, and it needn't be.  We only need
>> the size to be a power of two for the addressing.
>>
>>
>>
>> r~
>