diff mbox

[v2,01/12] ARM: Export cpu_env

Message ID 1367303766-9526-1-git-send-email-john.rigby@linaro.org
State New
Headers show

Commit Message

John Rigby April 30, 2013, 6:36 a.m. UTC 
From: Alexander Graf <agraf@suse.de>

The cpu_env tcg variable will be used by both the AArch32 and AArch64
handling code. Unstaticify it, so that both sides can make use of it.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 target-arm/translate.c |    2 +-
 target-arm/translate.h |    2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

Comments

Andreas Färber April 30, 2013, 11:42 a.m. UTC | #1 
Am 30.04.2013 08:36, schrieb John Rigby:
> From: Alexander Graf <agraf@suse.de>
> 
> The cpu_env tcg variable will be used by both the AArch32 and AArch64
> handling code. Unstaticify it, so that both sides can make use of it.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>

John, when you submit patches, you need to add your Signed-off-by!

http://wiki.qemu.org/Contribute/SubmitAPatch

This patch is not reviewable in context because the messages are not
threaded together - git-send-email does that by default for you.

> ---
>  target-arm/translate.c |    2 +-
>  target-arm/translate.h |    2 ++
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 675773a..36537bd 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -59,7 +59,7 @@ static uint32_t gen_opc_condexec_bits[OPC_BUF_SIZE];
>  #define DISAS_WFI 4
>  #define DISAS_SWI 5
>  
> -static TCGv_ptr cpu_env;
> +TCGv_ptr cpu_env;
>  /* We reuse the same 64-bit temporaries for efficiency.  */
>  static TCGv_i64 cpu_V0, cpu_V1, cpu_M0;
>  static TCGv_i32 cpu_R[16];
> diff --git a/target-arm/translate.h b/target-arm/translate.h
> index e727bc6..8ba1433 100644
> --- a/target-arm/translate.h
> +++ b/target-arm/translate.h
> @@ -24,4 +24,6 @@ typedef struct DisasContext {
>      int vec_stride;
>  } DisasContext;
>  
> +extern TCGv_ptr cpu_env;
> +
>  #endif /* TARGET_ARM_TRANSLATE_H */

Alex, have you checked whether the variable can be placed in qom/cpu.c
instead once for all targets? I'd hope that would be possible with an
appropriate typedef (since target_long size etc. are unknown there).

Andreas
Alexander Graf April 30, 2013, 11:54 a.m. UTC | #2 
Am 30.04.2013 um 13:42 schrieb Andreas Färber <afaerber@suse.de>:

> Am 30.04.2013 08:36, schrieb John Rigby:
>> From: Alexander Graf <agraf@suse.de>
>> 
>> The cpu_env tcg variable will be used by both the AArch32 and AArch64
>> handling code. Unstaticify it, so that both sides can make use of it.
>> 
>> Signed-off-by: Alexander Graf <agraf@suse.de>
> 
> John, when you submit patches, you need to add your Signed-off-by!
> 
> http://wiki.qemu.org/Contribute/SubmitAPatch
> 
> This patch is not reviewable in context because the messages are not
> threaded together - git-send-email does that by default for you.
> 
>> ---
>> target-arm/translate.c |    2 +-
>> target-arm/translate.h |    2 ++
>> 2 files changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/target-arm/translate.c b/target-arm/translate.c
>> index 675773a..36537bd 100644
>> --- a/target-arm/translate.c
>> +++ b/target-arm/translate.c
>> @@ -59,7 +59,7 @@ static uint32_t gen_opc_condexec_bits[OPC_BUF_SIZE];
>> #define DISAS_WFI 4
>> #define DISAS_SWI 5
>> 
>> -static TCGv_ptr cpu_env;
>> +TCGv_ptr cpu_env;
>> /* We reuse the same 64-bit temporaries for efficiency.  */
>> static TCGv_i64 cpu_V0, cpu_V1, cpu_M0;
>> static TCGv_i32 cpu_R[16];
>> diff --git a/target-arm/translate.h b/target-arm/translate.h
>> index e727bc6..8ba1433 100644
>> --- a/target-arm/translate.h
>> +++ b/target-arm/translate.h
>> @@ -24,4 +24,6 @@ typedef struct DisasContext {
>>     int vec_stride;
>> } DisasContext;
>> 
>> +extern TCGv_ptr cpu_env;
>> +
>> #endif /* TARGET_ARM_TRANSLATE_H */
> 
> Alex, have you checked whether the variable can be placed in qom/cpu.c
> instead once for all targets? I'd hope that would be possible with an
> appropriate typedef (since target_long size etc. are unknown there).

I would prefer to keep the translation context separate from the execution context.

Alex

> 
> Andreas
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Andreas Färber April 30, 2013, noon UTC | #3 
Am 30.04.2013 13:54, schrieb Alexander Graf:
> 
> Am 30.04.2013 um 13:42 schrieb Andreas Färber <afaerber@suse.de>:
> 
>> Am 30.04.2013 08:36, schrieb John Rigby:
>>> From: Alexander Graf <agraf@suse.de>
>>>
>>> The cpu_env tcg variable will be used by both the AArch32 and AArch64
>>> handling code. Unstaticify it, so that both sides can make use of it.
>>>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
[...]
>>> ---
>>> target-arm/translate.c |    2 +-
>>> target-arm/translate.h |    2 ++
>>> 2 files changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/target-arm/translate.c b/target-arm/translate.c
>>> index 675773a..36537bd 100644
>>> --- a/target-arm/translate.c
>>> +++ b/target-arm/translate.c
>>> @@ -59,7 +59,7 @@ static uint32_t gen_opc_condexec_bits[OPC_BUF_SIZE];
>>> #define DISAS_WFI 4
>>> #define DISAS_SWI 5
>>>
>>> -static TCGv_ptr cpu_env;
>>> +TCGv_ptr cpu_env;
>>> /* We reuse the same 64-bit temporaries for efficiency.  */
>>> static TCGv_i64 cpu_V0, cpu_V1, cpu_M0;
>>> static TCGv_i32 cpu_R[16];
>>> diff --git a/target-arm/translate.h b/target-arm/translate.h
>>> index e727bc6..8ba1433 100644
>>> --- a/target-arm/translate.h
>>> +++ b/target-arm/translate.h
>>> @@ -24,4 +24,6 @@ typedef struct DisasContext {
>>>     int vec_stride;
>>> } DisasContext;
>>>
>>> +extern TCGv_ptr cpu_env;
>>> +
>>> #endif /* TARGET_ARM_TRANSLATE_H */
>>
>> Alex, have you checked whether the variable can be placed in qom/cpu.c
>> instead once for all targets? I'd hope that would be possible with an
>> appropriate typedef (since target_long size etc. are unknown there).
> 
> I would prefer to keep the translation context separate from the execution context.

Not sure if you understood my point? Exposing cpu_env from target-arm
would seem to torpedo our efforts to link target-arm and, e.g.,
target-microblaze together, since they all have cpu_env. I don't really
care if it's qom/cpu.c or tcg/shared.c or renaming to arm_cpu_env. :)

Andreas
Alexander Graf April 30, 2013, 12:33 p.m. UTC | #4 
On 04/30/2013 02:00 PM, Andreas Färber wrote:
> Am 30.04.2013 13:54, schrieb Alexander Graf:
>> Am 30.04.2013 um 13:42 schrieb Andreas Färber<afaerber@suse.de>:
>>
>>> Am 30.04.2013 08:36, schrieb John Rigby:
>>>> From: Alexander Graf<agraf@suse.de>
>>>>
>>>> The cpu_env tcg variable will be used by both the AArch32 and AArch64
>>>> handling code. Unstaticify it, so that both sides can make use of it.
>>>>
>>>> Signed-off-by: Alexander Graf<agraf@suse.de>
> [...]
>>>> ---
>>>> target-arm/translate.c |    2 +-
>>>> target-arm/translate.h |    2 ++
>>>> 2 files changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/target-arm/translate.c b/target-arm/translate.c
>>>> index 675773a..36537bd 100644
>>>> --- a/target-arm/translate.c
>>>> +++ b/target-arm/translate.c
>>>> @@ -59,7 +59,7 @@ static uint32_t gen_opc_condexec_bits[OPC_BUF_SIZE];
>>>> #define DISAS_WFI 4
>>>> #define DISAS_SWI 5
>>>>
>>>> -static TCGv_ptr cpu_env;
>>>> +TCGv_ptr cpu_env;
>>>> /* We reuse the same 64-bit temporaries for efficiency.  */
>>>> static TCGv_i64 cpu_V0, cpu_V1, cpu_M0;
>>>> static TCGv_i32 cpu_R[16];
>>>> diff --git a/target-arm/translate.h b/target-arm/translate.h
>>>> index e727bc6..8ba1433 100644
>>>> --- a/target-arm/translate.h
>>>> +++ b/target-arm/translate.h
>>>> @@ -24,4 +24,6 @@ typedef struct DisasContext {
>>>>      int vec_stride;
>>>> } DisasContext;
>>>>
>>>> +extern TCGv_ptr cpu_env;
>>>> +
>>>> #endif /* TARGET_ARM_TRANSLATE_H */
>>> Alex, have you checked whether the variable can be placed in qom/cpu.c
>>> instead once for all targets? I'd hope that would be possible with an
>>> appropriate typedef (since target_long size etc. are unknown there).
>> I would prefer to keep the translation context separate from the execution context.
> Not sure if you understood my point? Exposing cpu_env from target-arm
> would seem to torpedo our efforts to link target-arm and, e.g.,
> target-microblaze together, since they all have cpu_env. I don't really
> care if it's qom/cpu.c or tcg/shared.c or renaming to arm_cpu_env. :)

I see :). I was more coming from the angle of "we should be able to 
conditionally not compile TCG code". So anything that is TCG translation 
code and respective defines around it should be contained inside of itself.


Alex
Richard Henderson May 1, 2013, 9:29 a.m. UTC | #5 
On 2013-04-30 13:00, Andreas Färber wrote:
> Not sure if you understood my point? Exposing cpu_env from target-arm
> would seem to torpedo our efforts to link target-arm and, e.g.,
> target-microblaze together, since they all have cpu_env. I don't really
> care if it's qom/cpu.c or tcg/shared.c or renaming to arm_cpu_env. :)

cpu_env is still static within target-microblaze/translate.c;
there is no conflict ... yet.


r~
Peter Maydell June 28, 2013, 2:20 p.m. UTC | #6 
On 30 April 2013 13:00, Andreas Färber <afaerber@suse.de> wrote:
> Am 30.04.2013 13:54, schrieb Alexander Graf:
>> Am 30.04.2013 um 13:42 schrieb Andreas Färber <afaerber@suse.de>:
>>
>>> Am 30.04.2013 08:36, schrieb John Rigby:
>>>> From: Alexander Graf <agraf@suse.de>
>>>> diff --git a/target-arm/translate.c b/target-arm/translate.c
>>>> index 675773a..36537bd 100644
>>>> --- a/target-arm/translate.c
>>>> +++ b/target-arm/translate.c
>>>> @@ -59,7 +59,7 @@ static uint32_t gen_opc_condexec_bits[OPC_BUF_SIZE];
>>>> #define DISAS_WFI 4
>>>> #define DISAS_SWI 5
>>>>
>>>> -static TCGv_ptr cpu_env;
>>>> +TCGv_ptr cpu_env;
>>>> /* We reuse the same 64-bit temporaries for efficiency.  */
>>>> static TCGv_i64 cpu_V0, cpu_V1, cpu_M0;
>>>> static TCGv_i32 cpu_R[16];
>>>> diff --git a/target-arm/translate.h b/target-arm/translate.h
>>>> index e727bc6..8ba1433 100644
>>>> --- a/target-arm/translate.h
>>>> +++ b/target-arm/translate.h
>>>> @@ -24,4 +24,6 @@ typedef struct DisasContext {
>>>>     int vec_stride;
>>>> } DisasContext;
>>>>
>>>> +extern TCGv_ptr cpu_env;
>>>> +
>>>> #endif /* TARGET_ARM_TRANSLATE_H */
>>>
>>> Alex, have you checked whether the variable can be placed in qom/cpu.c
>>> instead once for all targets? I'd hope that would be possible with an
>>> appropriate typedef (since target_long size etc. are unknown there).
>>
>> I would prefer to keep the translation context separate from the execution context.
>
> Not sure if you understood my point? Exposing cpu_env from target-arm
> would seem to torpedo our efforts to link target-arm and, e.g.,
> target-microblaze together, since they all have cpu_env. I don't really
> care if it's qom/cpu.c or tcg/shared.c or renaming to arm_cpu_env. :)

So we could deal with this by:
 translate-a64.c has its own 'static TCGv_ptr cpu_env;'
 translate.c:arm_translate_init() passes cpu_env to
   a64_translate_init()
 translate-a64.c:a64_translate_init() stashes the value it
   gets passed in its own cpu_env static.

That avoids having it be a non-static global and doesn't require us
to rename every usage of cpu_env in translate.c either.

Any objections?

thanks
-- PMM
Alexander Graf June 28, 2013, 2:25 p.m. UTC | #7 
On 28.06.2013, at 16:20, Peter Maydell wrote:

> On 30 April 2013 13:00, Andreas Färber <afaerber@suse.de> wrote:
>> Am 30.04.2013 13:54, schrieb Alexander Graf:
>>> Am 30.04.2013 um 13:42 schrieb Andreas Färber <afaerber@suse.de>:
>>> 
>>>> Am 30.04.2013 08:36, schrieb John Rigby:
>>>>> From: Alexander Graf <agraf@suse.de>
>>>>> diff --git a/target-arm/translate.c b/target-arm/translate.c
>>>>> index 675773a..36537bd 100644
>>>>> --- a/target-arm/translate.c
>>>>> +++ b/target-arm/translate.c
>>>>> @@ -59,7 +59,7 @@ static uint32_t gen_opc_condexec_bits[OPC_BUF_SIZE];
>>>>> #define DISAS_WFI 4
>>>>> #define DISAS_SWI 5
>>>>> 
>>>>> -static TCGv_ptr cpu_env;
>>>>> +TCGv_ptr cpu_env;
>>>>> /* We reuse the same 64-bit temporaries for efficiency.  */
>>>>> static TCGv_i64 cpu_V0, cpu_V1, cpu_M0;
>>>>> static TCGv_i32 cpu_R[16];
>>>>> diff --git a/target-arm/translate.h b/target-arm/translate.h
>>>>> index e727bc6..8ba1433 100644
>>>>> --- a/target-arm/translate.h
>>>>> +++ b/target-arm/translate.h
>>>>> @@ -24,4 +24,6 @@ typedef struct DisasContext {
>>>>>    int vec_stride;
>>>>> } DisasContext;
>>>>> 
>>>>> +extern TCGv_ptr cpu_env;
>>>>> +
>>>>> #endif /* TARGET_ARM_TRANSLATE_H */
>>>> 
>>>> Alex, have you checked whether the variable can be placed in qom/cpu.c
>>>> instead once for all targets? I'd hope that would be possible with an
>>>> appropriate typedef (since target_long size etc. are unknown there).
>>> 
>>> I would prefer to keep the translation context separate from the execution context.
>> 
>> Not sure if you understood my point? Exposing cpu_env from target-arm
>> would seem to torpedo our efforts to link target-arm and, e.g.,
>> target-microblaze together, since they all have cpu_env. I don't really
>> care if it's qom/cpu.c or tcg/shared.c or renaming to arm_cpu_env. :)
> 
> So we could deal with this by:
> translate-a64.c has its own 'static TCGv_ptr cpu_env;'
> translate.c:arm_translate_init() passes cpu_env to
>   a64_translate_init()
> translate-a64.c:a64_translate_init() stashes the value it
>   gets passed in its own cpu_env static.
> 
> That avoids having it be a non-static global and doesn't require us
> to rename every usage of cpu_env in translate.c either.
> 
> Any objections?

Works for me :).


Alex
Andreas Färber June 28, 2013, 2:28 p.m. UTC | #8 
Am 28.06.2013 16:20, schrieb Peter Maydell:
> On 30 April 2013 13:00, Andreas Färber <afaerber@suse.de> wrote:
>> Am 30.04.2013 13:54, schrieb Alexander Graf:
>>> Am 30.04.2013 um 13:42 schrieb Andreas Färber <afaerber@suse.de>:
>>>
>>>> Am 30.04.2013 08:36, schrieb John Rigby:
>>>>> From: Alexander Graf <agraf@suse.de>
>>>>> diff --git a/target-arm/translate.c b/target-arm/translate.c
>>>>> index 675773a..36537bd 100644
>>>>> --- a/target-arm/translate.c
>>>>> +++ b/target-arm/translate.c
>>>>> @@ -59,7 +59,7 @@ static uint32_t gen_opc_condexec_bits[OPC_BUF_SIZE];
>>>>> #define DISAS_WFI 4
>>>>> #define DISAS_SWI 5
>>>>>
>>>>> -static TCGv_ptr cpu_env;
>>>>> +TCGv_ptr cpu_env;
>>>>> /* We reuse the same 64-bit temporaries for efficiency.  */
>>>>> static TCGv_i64 cpu_V0, cpu_V1, cpu_M0;
>>>>> static TCGv_i32 cpu_R[16];
>>>>> diff --git a/target-arm/translate.h b/target-arm/translate.h
>>>>> index e727bc6..8ba1433 100644
>>>>> --- a/target-arm/translate.h
>>>>> +++ b/target-arm/translate.h
>>>>> @@ -24,4 +24,6 @@ typedef struct DisasContext {
>>>>>     int vec_stride;
>>>>> } DisasContext;
>>>>>
>>>>> +extern TCGv_ptr cpu_env;
>>>>> +
>>>>> #endif /* TARGET_ARM_TRANSLATE_H */
>>>>
>>>> Alex, have you checked whether the variable can be placed in qom/cpu.c
>>>> instead once for all targets? I'd hope that would be possible with an
>>>> appropriate typedef (since target_long size etc. are unknown there).
>>>
>>> I would prefer to keep the translation context separate from the execution context.
>>
>> Not sure if you understood my point? Exposing cpu_env from target-arm
>> would seem to torpedo our efforts to link target-arm and, e.g.,
>> target-microblaze together, since they all have cpu_env. I don't really
>> care if it's qom/cpu.c or tcg/shared.c or renaming to arm_cpu_env. :)
> 
> So we could deal with this by:
>  translate-a64.c has its own 'static TCGv_ptr cpu_env;'
>  translate.c:arm_translate_init() passes cpu_env to
>    a64_translate_init()
>  translate-a64.c:a64_translate_init() stashes the value it
>    gets passed in its own cpu_env static.
> 
> That avoids having it be a non-static global and doesn't require us
> to rename every usage of cpu_env in translate.c either.
> 
> Any objections?

I don't mind that cpu_env change getting committed as interim solution,
so far I did not come up with a better patch - we'd need to split out
host parts from tcg/tcg.h first, for which I did not find time yet.
Just please fix up the subject so that things are consistent.

Andreas
Peter Maydell June 28, 2013, 2:35 p.m. UTC | #9 
On 28 June 2013 15:28, Andreas Färber <afaerber@suse.de> wrote:
> I don't mind that cpu_env change getting committed as interim solution,
> so far I did not come up with a better patch - we'd need to split out
> host parts from tcg/tcg.h first, for which I did not find time yet.

Interim solution on the path to where? I'm not convinced the
cpu_env variables should be visible outside each individual
decoder (any more than, for instance, the ARM cpu_V0, cpu_V1
variables are). Admittedly the environment pointer is a bit
of a special case, but perhaps we should deal with it by making
it less of one?

thanks
-- PMM
Andreas Färber June 28, 2013, 2:42 p.m. UTC | #10 
Am 28.06.2013 16:35, schrieb Peter Maydell:
> On 28 June 2013 15:28, Andreas Färber <afaerber@suse.de> wrote:
>> I don't mind that cpu_env change getting committed as interim solution,
>> so far I did not come up with a better patch - we'd need to split out
>> host parts from tcg/tcg.h first, for which I did not find time yet.
> 
> Interim solution on the path to where? I'm not convinced the
> cpu_env variables should be visible outside each individual
> decoder (any more than, for instance, the ARM cpu_V0, cpu_V1
> variables are). Admittedly the environment pointer is a bit
> of a special case, but perhaps we should deal with it by making
> it less of one?

Well, I'm not so fond of the idea of having two static variables for the
same thing. If that is just to shield the global symbols, we could
rename the exported variable to arm_cpu_env and do #define cpu_env
arm_cpu_env to avoid a large-scale renaming.

I was thinking that the only thing this duplicated TCGv_ptr cpu_env
depends on is host pointer size, TCG debug configuration (struct or
integer) and host environment pointer register. So why duplicate it
longterm rather than having one that can be shared by multiple targets
to derive the target-specific register TCGv* variables?

Cheers,
Andreas
diff mbox

Patch

diff --git a/target-arm/translate.c b/target-arm/translate.c
index 675773a..36537bd 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -59,7 +59,7 @@  static uint32_t gen_opc_condexec_bits[OPC_BUF_SIZE];
 #define DISAS_WFI 4
 #define DISAS_SWI 5
 
-static TCGv_ptr cpu_env;
+TCGv_ptr cpu_env;
 /* We reuse the same 64-bit temporaries for efficiency.  */
 static TCGv_i64 cpu_V0, cpu_V1, cpu_M0;
 static TCGv_i32 cpu_R[16];
diff --git a/target-arm/translate.h b/target-arm/translate.h
index e727bc6..8ba1433 100644
--- a/target-arm/translate.h
+++ b/target-arm/translate.h
@@ -24,4 +24,6 @@  typedef struct DisasContext {
     int vec_stride;
 } DisasContext;
 
+extern TCGv_ptr cpu_env;
+
 #endif /* TARGET_ARM_TRANSLATE_H */