Patchwork [v2,1/3] move WORDS_ALIGNED to qemu-common.h

login
register
mail settings
Submitter Paolo Bonzini
Date June 6, 2011, 2:25 p.m.
Message ID <1307370348-28400-2-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/98944/
State New
Headers show

Comments

Paolo Bonzini - June 6, 2011, 2:25 p.m.
This is not a CPU interface.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpu-common.h  |    4 ----
 qemu-common.h |    4 ++++
 2 files changed, 4 insertions(+), 4 deletions(-)
Andreas Färber - June 6, 2011, 5:15 p.m.
Am 06.06.2011 um 16:25 schrieb Paolo Bonzini:

> This is not a CPU interface.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

> diff --git a/qemu-common.h b/qemu-common.h
> index b851b20..7484ef8 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -4,6 +4,10 @@
>
> #include "config-host.h"
>
> +#if defined(__arm__) || defined(__sparc__) || defined(__mips__) ||  
> defined(__hppa__) || defined(__ia64__)
> +#define WORDS_ALIGNED
> +#endif

Since it depends on the host and you're placing it directly under  
config-host.h inclusion, might it make sense to move the decision into  
configure instead, so that it ends up in config-host.h?

Andreas
Paolo Bonzini - June 6, 2011, 8:24 p.m.
On 06/06/2011 07:15 PM, Andreas Färber wrote:
> Am 06.06.2011 um 16:25 schrieb Paolo Bonzini:
>
>> This is not a CPU interface.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>
>> diff --git a/qemu-common.h b/qemu-common.h
>> index b851b20..7484ef8 100644
>> --- a/qemu-common.h
>> +++ b/qemu-common.h
>> @@ -4,6 +4,10 @@
>>
>> #include "config-host.h"
>>
>> +#if defined(__arm__) || defined(__sparc__) || defined(__mips__) ||
>> defined(__hppa__) || defined(__ia64__)
>> +#define WORDS_ALIGNED
>> +#endif
>
> Since it depends on the host and you're placing it directly under
> config-host.h inclusion, might it make sense to move the decision into
> configure instead, so that it ends up in config-host.h?

Richard's suggestion will get rid of this altogether.

Paolo
Richard Henderson - June 6, 2011, 10:15 p.m.
On 06/06/2011 10:15 AM, Andreas Färber wrote:
> Am 06.06.2011 um 16:25 schrieb Paolo Bonzini:
>> +#if defined(__arm__) || defined(__sparc__) || defined(__mips__) || defined(__hppa__) || defined(__ia64__)
>> +#define WORDS_ALIGNED
>> +#endif
> 
> Since it depends on the host and you're placing it directly under
> config-host.h inclusion, might it make sense to move the decision
> into configure instead, so that it ends up in config-host.h?

Hum, I now understand what Paulo was talking about elsewhere in the thread.

If he takes my suggestion to re-write the unaligned functions with GCC packed
support, these host ifdefs go away, and this objection disappears.  The
question becomes one of ordering.

Do we take his existing 3-part patch as-is, and the packed patch as a followup?
Do we convert to packed accesses first and move it around after?
Do we do it all in one step?


r~
Paolo Bonzini - June 7, 2011, 9:17 a.m.
On 06/07/2011 12:15 AM, Richard Henderson wrote:
> Do we take his existing 3-part patch as-is, and the packed patch as a followup?
> Do we convert to packed accesses first and move it around after?
> Do we do it all in one step?

Either of the first two works for me.

However, since this series was a start towards fixing real bugs reported 
by Coverity:

     qemu-kvm-0.14.0/hw/scsi-bus.c:190:
     sign_extension: Suspicious implicit sign extension:

     "cmd[10]" with type "unsigned char" (8 bits, unsigned) is promoted
     in "cmd[13] | (cmd[12] << 8) | (cmd[11] << 16) | (cmd[10] << 24)"
     to type "int" (32 bits, signed), then sign-extended to type
     "unsigned long" (64 bits, unsigned).

     If "cmd[13] | (cmd[12] << 8) | (cmd[11] << 16) | (cmd[10] << 24)"
     is greater than 0x7FFFFFFF, the upper bits of the result will all
     be 1.

... and there were objections on requiring recent GCC, perhaps it's 
better to just commit it as is.

Paolo

Patch

diff --git a/cpu-common.h b/cpu-common.h
index 7a86d63..7979a18 100644
--- a/cpu-common.h
+++ b/cpu-common.h
@@ -3,10 +3,6 @@ 
 
 /* CPU interfaces that are target indpendent.  */
 
-#if defined(__arm__) || defined(__sparc__) || defined(__mips__) || defined(__hppa__) || defined(__ia64__)
-#define WORDS_ALIGNED
-#endif
-
 #ifdef TARGET_PHYS_ADDR_BITS
 #include "targphys.h"
 #endif
diff --git a/qemu-common.h b/qemu-common.h
index b851b20..7484ef8 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -4,6 +4,10 @@ 
 
 #include "config-host.h"
 
+#if defined(__arm__) || defined(__sparc__) || defined(__mips__) || defined(__hppa__) || defined(__ia64__)
+#define WORDS_ALIGNED
+#endif
+
 #define QEMU_NORETURN __attribute__ ((__noreturn__))
 #ifdef CONFIG_GCC_ATTRIBUTE_WARN_UNUSED_RESULT
 #define QEMU_WARN_UNUSED_RESULT __attribute__((warn_unused_result))