diff mbox

[RFC,v2,10/34] include/exec: Split target_long def to new header

Message ID 74cc0d2f8d1d9d52638a1b3633e1a861b51907f4.1433052532.git.crosthwaite.peter@gmail.com
State New
Headers show

Commit Message

Peter Crosthwaite May 31, 2015, 6:11 a.m. UTC 
This is currently provided by cpu-defs and is a target specific
definition. However, to prepare for multi-arch only the bare minimum
content from cpu-defs.h should be exported to core code. And this is
all we need. So split it to a new header that the target_multi cpu.h
can include to save on having to include the ill-defined cpu-defs.h.

Allow multiple inclusion for multi-arch where multiple cpu.h's need
to be included and target_long will vary for each.

Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
 include/exec/cpu-defs.h    | 23 +-------------------
 include/exec/target-long.h | 52 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 22 deletions(-)
 create mode 100644 include/exec/target-long.h

Comments

Richard Henderson June 1, 2015, 7:24 p.m. UTC | #1 
On 05/30/2015 11:11 PM, Peter Crosthwaite wrote:
> This is currently provided by cpu-defs and is a target specific
> definition. However, to prepare for multi-arch only the bare minimum
> content from cpu-defs.h should be exported to core code. And this is
> all we need. So split it to a new header that the target_multi cpu.h
> can include to save on having to include the ill-defined cpu-defs.h.
> 
> Allow multiple inclusion for multi-arch where multiple cpu.h's need
> to be included and target_long will vary for each.
> 
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> ---
>  include/exec/cpu-defs.h    | 23 +-------------------
>  include/exec/target-long.h | 52 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 53 insertions(+), 22 deletions(-)
>  create mode 100644 include/exec/target-long.h

Multiple inclusion with a typedef?  How's that supposed to work?


r~
Paolo Bonzini June 1, 2015, 7:51 p.m. UTC | #2 
On 01/06/2015 21:24, Richard Henderson wrote:
> On 05/30/2015 11:11 PM, Peter Crosthwaite wrote:
>> This is currently provided by cpu-defs and is a target specific
>> definition. However, to prepare for multi-arch only the bare minimum
>> content from cpu-defs.h should be exported to core code. And this is
>> all we need. So split it to a new header that the target_multi cpu.h
>> can include to save on having to include the ill-defined cpu-defs.h.
>>
>> Allow multiple inclusion for multi-arch where multiple cpu.h's need
>> to be included and target_long will vary for each.
>>
>> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>> ---
>>  include/exec/cpu-defs.h    | 23 +-------------------
>>  include/exec/target-long.h | 52 ++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 53 insertions(+), 22 deletions(-)
>>  create mode 100644 include/exec/target-long.h
> 
> Multiple inclusion with a typedef?  How's that supposed to work?

He later #defines target_{,u}long to e.g. arm_target_{,u}long.

Paolo
Peter Maydell June 1, 2015, 8:25 p.m. UTC | #3 
On 1 June 2015 at 20:51, Paolo Bonzini <pbonzini@redhat.com> wrote:
> He later #defines target_{,u}long to e.g. arm_target_{,u}long.

Yikes.

-- PMM
Paolo Bonzini June 1, 2015, 8:27 p.m. UTC | #4 
----- Original Message -----
> From: "Peter Maydell" <peter.maydell@linaro.org>
> To: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: "Richard Henderson" <rth@twiddle.net>, "Peter Crosthwaite" <crosthwaitepeter@gmail.com>, "QEMU Developers"
> <qemu-devel@nongnu.org>, "Edgar E. Iglesias" <edgar.iglesias@gmail.com>, "Peter Crosthwaite"
> <crosthwaite.peter@gmail.com>, "Andreas Färber" <afaerber@suse.de>
> Sent: Monday, June 1, 2015 10:25:03 PM
> Subject: Re: [RFC v2 10/34] include/exec: Split target_long def to new header
> 
> On 1 June 2015 at 20:51, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > He later #defines target_{,u}long to e.g. arm_target_{,u}long.
> 
> Yikes.

Heh. :)

It's actually a pretty clean patchset, once one gets over the initial
feeling of O_O-ness.

Paolo
Richard Henderson June 1, 2015, 8:32 p.m. UTC | #5 
On 06/01/2015 12:51 PM, Paolo Bonzini wrote:
>
>
> On 01/06/2015 21:24, Richard Henderson wrote:
>> On 05/30/2015 11:11 PM, Peter Crosthwaite wrote:
>>> This is currently provided by cpu-defs and is a target specific
>>> definition. However, to prepare for multi-arch only the bare minimum
>>> content from cpu-defs.h should be exported to core code. And this is
>>> all we need. So split it to a new header that the target_multi cpu.h
>>> can include to save on having to include the ill-defined cpu-defs.h.
>>>
>>> Allow multiple inclusion for multi-arch where multiple cpu.h's need
>>> to be included and target_long will vary for each.
>>>
>>> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>>> ---
>>>   include/exec/cpu-defs.h    | 23 +-------------------
>>>   include/exec/target-long.h | 52 ++++++++++++++++++++++++++++++++++++++++++++++
>>>   2 files changed, 53 insertions(+), 22 deletions(-)
>>>   create mode 100644 include/exec/target-long.h
>>
>> Multiple inclusion with a typedef?  How's that supposed to work?
>
> He later #defines target_{,u}long to e.g. arm_target_{,u}long.

Ok, here's where I'm not liking things.  It shouldn't be a typedef in some 
places and a define others.  From this description, it sounds like it ought to 
always be a define.


r~
Paolo Bonzini June 1, 2015, 8:39 p.m. UTC | #6 
> > He later #defines target_{,u}long to e.g. arm_target_{,u}long.
> 
> Ok, here's where I'm not liking things.  It shouldn't be a typedef in some
> places and a define others.  From this description, it sounds like it ought
> to always be a define.

target_long expands to arm_target_long, which in turn is a typedef
provided by include/exec/target_long.h.  See the "multi-arch"izing
patches for arm and microblaze.

Paolo
Peter Crosthwaite June 2, 2015, 10:14 a.m. UTC | #7 
On Mon, Jun 1, 2015 at 1:32 PM, Richard Henderson <rth@twiddle.net> wrote:
> On 06/01/2015 12:51 PM, Paolo Bonzini wrote:
>>
>>
>>
>> On 01/06/2015 21:24, Richard Henderson wrote:
>>>
>>> On 05/30/2015 11:11 PM, Peter Crosthwaite wrote:
>>>>
>>>> This is currently provided by cpu-defs and is a target specific
>>>> definition. However, to prepare for multi-arch only the bare minimum
>>>> content from cpu-defs.h should be exported to core code. And this is
>>>> all we need. So split it to a new header that the target_multi cpu.h
>>>> can include to save on having to include the ill-defined cpu-defs.h.
>>>>
>>>> Allow multiple inclusion for multi-arch where multiple cpu.h's need
>>>> to be included and target_long will vary for each.
>>>>
>>>> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>>>> ---
>>>>   include/exec/cpu-defs.h    | 23 +-------------------
>>>>   include/exec/target-long.h | 52
>>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>>>   2 files changed, 53 insertions(+), 22 deletions(-)
>>>>   create mode 100644 include/exec/target-long.h
>>>
>>>
>>> Multiple inclusion with a typedef?  How's that supposed to work?
>>
>>
>> He later #defines target_{,u}long to e.g. arm_target_{,u}long.
>
>
> Ok, here's where I'm not liking things.  It shouldn't be a typedef in some
> places and a define others.  From this description, it sounds like it ought
> to always be a define.
>

The #define-always change does make for a cleaner end result but I
stayed away from it purely because I was thinking typedefs are better
for type-definitions. But if we are open to the change of the #define
based implementation I am all for it as the target-foo/cpu.h change
pattern in minimised.

We still have a similar problems with cpu-defs.h/CPUTLBEntry though. I
have to think harder about how that can be done, but one solution is
to conditionally change the tlb_table defs in CPU_COMMON to be just a
dummy uint8_t[] in MULTI_ARCH case. This is ok, as the struct fields
are only accessible by arch-obj-y which will get the full-service
definition via non TARGET_MULTI_ARCH arch-obj-y compile. The work is
half done for us, as CPUTLBTable already has a uint8_t padding system
in place.

CPUIOTLBEntry can be moved to another header as it has no arch specific deps.

All in all, we can do this with 0 #define foo arm_foo in arch cpu.h's,
with these edits.

Regards,
Peter

>
> r~
>
>
Paolo Bonzini June 3, 2015, 10:01 a.m. UTC | #8 
On 02/06/2015 12:14, Peter Crosthwaite wrote:
> The #define-always change does make for a cleaner end result but I
> stayed away from it purely because I was thinking typedefs are better
> for type-definitions. But if we are open to the change of the #define
> based implementation I am all for it as the target-foo/cpu.h change
> pattern in minimised.
> 
> We still have a similar problems with cpu-defs.h/CPUTLBEntry though. I
> have to think harder about how that can be done, but one solution is
> to conditionally change the tlb_table defs in CPU_COMMON to be just a
> dummy uint8_t[] in MULTI_ARCH case. This is ok, as the struct fields
> are only accessible by arch-obj-y which will get the full-service
> definition via non TARGET_MULTI_ARCH arch-obj-y compile. The work is
> half done for us, as CPUTLBTable already has a uint8_t padding system
> in place.

I guess you would hardcode CPUTLBEntry to 32 bytes in this case.

> CPUIOTLBEntry can be moved to another header as it has no arch specific deps.
> 
> All in all, we can do this with 0 #define foo arm_foo in arch cpu.h's,
> with these edits.

That would be nice to see for v3.

Paolo
diff mbox

Patch

diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h
index 0f4886d..1c52d2a 100644
--- a/include/exec/cpu-defs.h
+++ b/include/exec/cpu-defs.h
@@ -32,28 +32,7 @@ 
 #endif
 #include "exec/memattrs.h"
 
-#ifndef TARGET_LONG_BITS
-#error TARGET_LONG_BITS must be defined before including this header
-#endif
-
-#define TARGET_LONG_SIZE (TARGET_LONG_BITS / 8)
-
-/* target_ulong is the type of a virtual address */
-#if TARGET_LONG_SIZE == 4
-typedef int32_t target_long;
-typedef uint32_t target_ulong;
-#define TARGET_FMT_lx "%08x"
-#define TARGET_FMT_ld "%d"
-#define TARGET_FMT_lu "%u"
-#elif TARGET_LONG_SIZE == 8
-typedef int64_t target_long;
-typedef uint64_t target_ulong;
-#define TARGET_FMT_lx "%016" PRIx64
-#define TARGET_FMT_ld "%" PRId64
-#define TARGET_FMT_lu "%" PRIu64
-#else
-#error TARGET_LONG_SIZE undefined
-#endif
+#include "exec/target-long.h"
 
 /* Only the bottom TB_JMP_PAGE_BITS of the jump cache hash bits vary for
    addresses on the same page.  The top bits are the same.  This allows
diff --git a/include/exec/target-long.h b/include/exec/target-long.h
new file mode 100644
index 0000000..478e8f0
--- /dev/null
+++ b/include/exec/target-long.h
@@ -0,0 +1,52 @@ 
+/*
+ * definition for the target_long type and friends.
+ *
+ * Copyright (c) 2003 Fabrice Bellard
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+/* No multiple included guard intended. Multi-arch setups may require multiple
+ * cpu.h's included which means this can be and should be reached twice.
+ */
+
+#include <stdint.h>
+
+#ifndef TARGET_LONG_BITS
+#error TARGET_LONG_BITS must be defined before including this header
+#endif
+
+#undef TARGET_LONG_SIZE
+#define TARGET_LONG_SIZE (TARGET_LONG_BITS / 8)
+
+#undef TARGET_FMT_lx
+#undef TARGET_FMT_ld
+#undef TARGET_FMT_lu
+
+/* target_ulong is the type of a virtual address */
+#if TARGET_LONG_SIZE == 4
+typedef int32_t target_long;
+typedef uint32_t target_ulong;
+#define TARGET_FMT_lx "%08x"
+#define TARGET_FMT_ld "%d"
+#define TARGET_FMT_lu "%u"
+#elif TARGET_LONG_SIZE == 8
+typedef int64_t target_long;
+typedef uint64_t target_ulong;
+#define TARGET_FMT_lx "%016" PRIx64
+#define TARGET_FMT_ld "%" PRId64
+#define TARGET_FMT_lu "%" PRIu64
+#else
+#error TARGET_LONG_SIZE undefined
+#endif