Message ID | 1367303766-9526-1-git-send-email-john.rigby@linaro.org |
---|---|
State | New |
Headers | show |
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
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
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
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
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~
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
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
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
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
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 --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 */