* Re: [Qemu-devel] [PATCH v2 01/12] ARM: Export cpu_env
2013-04-30 12:00 ` Andreas Färber
@ 2013-04-30 12:33 ` Alexander Graf
2013-05-01 9:29 ` Richard Henderson
2013-06-28 14:20 ` Peter Maydell
2 siblings, 0 replies; 11+ messages in thread
From: Alexander Graf @ 2013-04-30 12:33 UTC (permalink / raw)
To: Andreas Färber
Cc: Peter Crosthwaite, Peter Maydell, John Rigby, qemu-devel,
Edgar E. Iglesias
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [Qemu-devel] [PATCH v2 01/12] ARM: Export cpu_env
2013-04-30 12:00 ` Andreas Färber
2013-04-30 12:33 ` Alexander Graf
@ 2013-05-01 9:29 ` Richard Henderson
2013-06-28 14:20 ` Peter Maydell
2 siblings, 0 replies; 11+ messages in thread
From: Richard Henderson @ 2013-05-01 9:29 UTC (permalink / raw)
To: Andreas Färber
Cc: Peter Maydell, John Rigby, qemu-devel, Peter Crosthwaite,
Alexander Graf, Edgar E. Iglesias
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~
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2 01/12] ARM: Export cpu_env
2013-04-30 12:00 ` Andreas Färber
2013-04-30 12:33 ` Alexander Graf
2013-05-01 9:29 ` Richard Henderson
@ 2013-06-28 14:20 ` Peter Maydell
2013-06-28 14:25 ` Alexander Graf
2013-06-28 14:28 ` Andreas Färber
2 siblings, 2 replies; 11+ messages in thread
From: Peter Maydell @ 2013-06-28 14:20 UTC (permalink / raw)
To: Andreas Färber
Cc: Peter Crosthwaite, Edgar E. Iglesias, John Rigby, Alexander Graf,
qemu-devel
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [Qemu-devel] [PATCH v2 01/12] ARM: Export cpu_env
2013-06-28 14:20 ` Peter Maydell
@ 2013-06-28 14:25 ` Alexander Graf
2013-06-28 14:28 ` Andreas Färber
1 sibling, 0 replies; 11+ messages in thread
From: Alexander Graf @ 2013-06-28 14:25 UTC (permalink / raw)
To: Peter Maydell
Cc: Peter Crosthwaite, Edgar E. Iglesias, John Rigby,
Andreas Färber, qemu-devel
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [Qemu-devel] [PATCH v2 01/12] ARM: Export cpu_env
2013-06-28 14:20 ` Peter Maydell
2013-06-28 14:25 ` Alexander Graf
@ 2013-06-28 14:28 ` Andreas Färber
2013-06-28 14:35 ` Peter Maydell
1 sibling, 1 reply; 11+ messages in thread
From: Andreas Färber @ 2013-06-28 14:28 UTC (permalink / raw)
To: Peter Maydell
Cc: Peter Crosthwaite, Edgar E. Iglesias, John Rigby, Alexander Graf,
qemu-devel
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
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [Qemu-devel] [PATCH v2 01/12] ARM: Export cpu_env
2013-06-28 14:28 ` Andreas Färber
@ 2013-06-28 14:35 ` Peter Maydell
2013-06-28 14:42 ` Andreas Färber
0 siblings, 1 reply; 11+ messages in thread
From: Peter Maydell @ 2013-06-28 14:35 UTC (permalink / raw)
To: Andreas Färber
Cc: Peter Crosthwaite, Edgar E. Iglesias, John Rigby, Alexander Graf,
qemu-devel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2 01/12] ARM: Export cpu_env
2013-06-28 14:35 ` Peter Maydell
@ 2013-06-28 14:42 ` Andreas Färber
0 siblings, 0 replies; 11+ messages in thread
From: Andreas Färber @ 2013-06-28 14:42 UTC (permalink / raw)
To: Peter Maydell
Cc: Peter Crosthwaite, Edgar E. Iglesias, John Rigby, Alexander Graf,
qemu-devel
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
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 11+ messages in thread