| Message ID | 1277745445-30560-5-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
> diff --git a/exec-all.h b/exec-all.h > index a775582..ebe88ad 100644 > --- a/exec-all.h > +++ b/exec-all.h > @@ -353,4 +353,8 @@ extern int singlestep; > /* cpu-exec.c */ > extern volatile sig_atomic_t exit_request; > > +#ifdef NEED_GLOBAL_ENV > +register CPUState *env asm(AREG0); > +#endif Wouldn't it be better to just put this in dyngen-exec.h ? AFAICT there's a direct correlation between NEED_GLOBAL_ENV and #include "exec.h". Paul
On 06/28/2010 10:29 PM, Paul Brook wrote: >> diff --git a/exec-all.h b/exec-all.h >> index a775582..ebe88ad 100644 >> --- a/exec-all.h >> +++ b/exec-all.h >> @@ -353,4 +353,8 @@ extern int singlestep; >> /* cpu-exec.c */ >> extern volatile sig_atomic_t exit_request; >> >> +#ifdef NEED_GLOBAL_ENV >> +register CPUState *env asm(AREG0); >> +#endif > > Wouldn't it be better to just put this in dyngen-exec.h ? > AFAICT there's a direct correlation between NEED_GLOBAL_ENV and #include > "exec.h". True, see cover letter in 0/4. I was told to make each file request explicitly the global variable though. So I'd have to leave the #ifdef even if I moved it into dyngen-exec.h. Paolo
> On 06/28/2010 10:29 PM, Paul Brook wrote: > >> diff --git a/exec-all.h b/exec-all.h > >> index a775582..ebe88ad 100644 > >> --- a/exec-all.h > >> +++ b/exec-all.h > >> @@ -353,4 +353,8 @@ extern int singlestep; > >> > >> /* cpu-exec.c */ > >> extern volatile sig_atomic_t exit_request; > >> > >> +#ifdef NEED_GLOBAL_ENV > >> +register CPUState *env asm(AREG0); > >> +#endif > > > > Wouldn't it be better to just put this in dyngen-exec.h ? > > AFAICT there's a direct correlation between NEED_GLOBAL_ENV and #include > > "exec.h". > > True, see cover letter in 0/4. I was told to make each file request > explicitly the global variable though. So I'd have to leave the #ifdef > even if I moved it into dyngen-exec.h. I don't understand what this is supposed to achieve. The inclusion of exec.h is what defines whether this global variable is available. Just as importantly, it also prevents code clobbering this value. Having one without the other makes no sense. Making "env" available without including exec.h would be a different problem, but we never do that and would probably indicate we're doing something else wrong. Paul
On 06/29/2010 01:30 PM, Paul Brook wrote: > I don't understand what this is supposed to achieve. > The inclusion of exec.h is what defines whether this global variable is > available. Just as importantly, it also prevents code clobbering this value. > Having one without the other makes no sense. > > Making "env" available without including exec.h would be a different problem, > but we never do that and would probably indicate we're doing something else > wrong. Paul, I agree but I was told to do something different. Paolo
On 06/29/2010 03:51 PM, Paolo Bonzini wrote: > On 06/29/2010 01:30 PM, Paul Brook wrote: >> I don't understand what this is supposed to achieve. The inclusion >> of exec.h is what defines whether this global variable is >> available. Just as importantly, it also prevents code clobbering >> this value. Having one without the other makes no sense. >> >> Making "env" available without including exec.h would be a >> different problem, but we never do that and would probably indicate >> we're doing something else wrong. > > Paul, I agree but I was told to do something different. BTW, this may be useful for one thing: being able to include exec.h without bringing in the global variable. I didn't really see a use right now for that, but cpu_get_tb_cpu_state could be something that may belong in target-*/exec.h more than in target-*/cpu.h (no, I'm not going to move it); yet it cannot be moved right now because it is called in exec.c. That said, I think everybody at least agrees on the first 3 patches, is it possible to get at least (v2 of) thos applied? Paolo
> On 06/29/2010 03:51 PM, Paolo Bonzini wrote: > > On 06/29/2010 01:30 PM, Paul Brook wrote: > >> I don't understand what this is supposed to achieve. The inclusion > >> of exec.h is what defines whether this global variable is > >> available. Just as importantly, it also prevents code clobbering > >> this value. Having one without the other makes no sense. > >> > >> Making "env" available without including exec.h would be a > >> different problem, but we never do that and would probably indicate > >> we're doing something else wrong. > > > > Paul, I agree but I was told to do something different. > > BTW, this may be useful for one thing: being able to include exec.h > without bringing in the global variable. I'd argue that's not a desirable feature. I'd much rather have exec.h be the controlling factor than have all files include the same set of headers and have semantics determined by some combination of preprocessor macros. I realise we already have this with NEED_CPU_H, however I don't think that's a precedent we want to encourage. > I didn't really see a use right now for that, but cpu_get_tb_cpu_state > could be something that may belong in target-*/exec.h more than in > target-*/cpu.h (no, I'm not going to move it); yet it cannot be moved > right now because it is called in exec.c. How is putting it in exec.h better? Paul
On 06/29/2010 05:28 PM, Paul Brook wrote: >> On 06/29/2010 03:51 PM, Paolo Bonzini wrote: >>> On 06/29/2010 01:30 PM, Paul Brook wrote: >>>> I don't understand what this is supposed to achieve. The inclusion >>>> of exec.h is what defines whether this global variable is >>>> available. Just as importantly, it also prevents code clobbering >>>> this value. Having one without the other makes no sense. >>>> >>>> Making "env" available without including exec.h would be a >>>> different problem, but we never do that and would probably indicate >>>> we're doing something else wrong. >>> >>> Paul, I agree but I was told to do something different. >> >> BTW, this may be useful for one thing: being able to include exec.h >> without bringing in the global variable. > > I'd argue that's not a desirable feature. I'd much rather have exec.h be the > controlling factor than have all files include the same set of headers and > have semantics determined by some combination of preprocessor macros. > > I realise we already have this with NEED_CPU_H, however I don't think that's a > precedent we want to encourage. > >> I didn't really see a use right now for that, but cpu_get_tb_cpu_state >> could be something that may belong in target-*/exec.h more than in >> target-*/cpu.h (no, I'm not going to move it); yet it cannot be moved >> right now because it is called in exec.c. > > How is putting it in exec.h better? I see cpu.h as holding things related to the CPU as a hardware device, and exec.h as holding things related to emulation of the CPU. In theory, a hypothetical KVM-only/no-TCG version of QEMU would not need to include neither exec-all.h nor target-*/exec.h. Paolo
On Tue, Jun 29, 2010 at 7:44 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 06/28/2010 10:29 PM, Paul Brook wrote: >>> >>> diff --git a/exec-all.h b/exec-all.h >>> index a775582..ebe88ad 100644 >>> --- a/exec-all.h >>> +++ b/exec-all.h >>> @@ -353,4 +353,8 @@ extern int singlestep; >>> /* cpu-exec.c */ >>> extern volatile sig_atomic_t exit_request; >>> >>> +#ifdef NEED_GLOBAL_ENV >>> +register CPUState *env asm(AREG0); >>> +#endif >> >> Wouldn't it be better to just put this in dyngen-exec.h ? >> AFAICT there's a direct correlation between NEED_GLOBAL_ENV and #include >> "exec.h". > > True, see cover letter in 0/4. I was told to make each file request > explicitly the global variable though. So I'd have to leave the #ifdef even > if I moved it into dyngen-exec.h. Well, I only said I'd rename global 'env' to 'global_reg_env', not something about each file requesting it. But NEED_GLOBAL_ENV wasn't so bad idea in my opinion.
> >> BTW, this may be useful for one thing: being able to include exec.h > >> without bringing in the global variable. > > > > I'd argue that's not a desirable feature. I'd much rather have exec.h be > > the controlling factor than have all files include the same set of > > headers and have semantics determined by some combination of > > preprocessor macros. > > > > I realise we already have this with NEED_CPU_H, however I don't think > > that's a precedent we want to encourage. > > > >> I didn't really see a use right now for that, but cpu_get_tb_cpu_state > >> could be something that may belong in target-*/exec.h more than in > >> target-*/cpu.h (no, I'm not going to move it); yet it cannot be moved > >> right now because it is called in exec.c. > > > > How is putting it in exec.h better? > > I see cpu.h as holding things related to the CPU as a hardware device, > and exec.h as holding things related to emulation of the CPU. If you really want NEED_GLOBAL_ENV then your implementation is nowhere near sufficient. One of the main reasons we have exec.h is that you may not call GLOBAL_ENV code from regular code[1]. Doing so will result in qemu crashing. The current system is simple to understand and audit: All prototypes for code that includes exec.h must go in exec.h. Once you introduce NEED_GLOBAL_ENV you have both safe and unsafe code in exec.h. All unsafe code should be protected by proper #idefs. I have low confidence in being able to get this right. If you want to split out the TCG specific bits of cpu.h (including things like cpu_halted which are currently in exec.h but don't use global env) then I suggest creating a new header file for this. Paul [1] cpu_exec() is a special case, and jumps though hoops to make this work.
diff --git a/cpu-exec.c b/cpu-exec.c index 026980a..5d5170f 100644 --- a/cpu-exec.c +++ b/cpu-exec.c @@ -16,7 +16,9 @@ * 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/>. */ + #include "config.h" +#define NEED_GLOBAL_ENV #include "exec.h" #include "disas.h" #include "tcg.h" diff --git a/exec-all.h b/exec-all.h index a775582..ebe88ad 100644 --- a/exec-all.h +++ b/exec-all.h @@ -353,4 +353,8 @@ extern int singlestep; /* cpu-exec.c */ extern volatile sig_atomic_t exit_request; +#ifdef NEED_GLOBAL_ENV +register CPUState *env asm(AREG0); +#endif + #endif diff --git a/target-alpha/exec.h b/target-alpha/exec.h index a8a38d2..1950f83 100644 --- a/target-alpha/exec.h +++ b/target-alpha/exec.h @@ -26,8 +26,6 @@ #define TARGET_LONG_BITS 64 -register struct CPUAlphaState *env asm(AREG0); - #define FP_STATUS (env->fp_status) #include "cpu.h" diff --git a/target-alpha/op_helper.c b/target-alpha/op_helper.c index ff5ae26..9870d97 100644 --- a/target-alpha/op_helper.c +++ b/target-alpha/op_helper.c @@ -17,6 +17,7 @@ * License along with this library; if not, see <http://www.gnu.org/licenses/>. */ +#define NEED_GLOBAL_ENV #include "exec.h" #include "host-utils.h" #include "softfloat.h" diff --git a/target-arm/exec.h b/target-arm/exec.h index e4c35a3..eda5632 100644 --- a/target-arm/exec.h +++ b/target-arm/exec.h @@ -19,8 +19,6 @@ #include "config.h" #include "dyngen-exec.h" -register struct CPUARMState *env asm(AREG0); - #define M0 env->iwmmxt.val #include "cpu.h" diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c index 9b1a014..235cffc 100644 --- a/target-arm/op_helper.c +++ b/target-arm/op_helper.c @@ -16,6 +16,7 @@ * 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/>. */ +#define NEED_GLOBAL_ENV #include "exec.h" #include "helpers.h" diff --git a/target-cris/exec.h b/target-cris/exec.h index 93ce768..360f45a 100644 --- a/target-cris/exec.h +++ b/target-cris/exec.h @@ -19,8 +19,6 @@ */ #include "dyngen-exec.h" -register struct CPUCRISState *env asm(AREG0); - #include "cpu.h" #include "exec-all.h" diff --git a/target-cris/op_helper.c b/target-cris/op_helper.c index a60da94..8bb3876 100644 --- a/target-cris/op_helper.c +++ b/target-cris/op_helper.c @@ -18,6 +18,7 @@ * License along with this library; if not, see <http://www.gnu.org/licenses/>. */ +#define NEED_GLOBAL_ENV #include "exec.h" #include "mmu.h" #include "helper.h" diff --git a/target-i386/exec.h b/target-i386/exec.h index 04a566f..63f2363 100644 --- a/target-i386/exec.h +++ b/target-i386/exec.h @@ -28,8 +28,6 @@ #include "cpu-defs.h" -register struct CPUX86State *env asm(AREG0); - #include "qemu-common.h" #include "qemu-log.h" diff --git a/target-i386/op_helper.c b/target-i386/op_helper.c index 00fc671..05b6340 100644 --- a/target-i386/op_helper.c +++ b/target-i386/op_helper.c @@ -17,6 +17,7 @@ * License along with this library; if not, see <http://www.gnu.org/licenses/>. */ +#define NEED_GLOBAL_ENV #include "exec.h" #include "exec-all.h" #include "host-utils.h" diff --git a/target-m68k/exec.h b/target-m68k/exec.h index f31e06e..ac1cf12 100644 --- a/target-m68k/exec.h +++ b/target-m68k/exec.h @@ -19,8 +19,6 @@ */ #include "dyngen-exec.h" -register struct CPUM68KState *env asm(AREG0); - #include "cpu.h" #include "exec-all.h" diff --git a/target-m68k/op_helper.c b/target-m68k/op_helper.c index 0711107..df4489f 100644 --- a/target-m68k/op_helper.c +++ b/target-m68k/op_helper.c @@ -16,6 +16,8 @@ * 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/>. */ + +#define NEED_GLOBAL_ENV #include "exec.h" #include "helpers.h" diff --git a/target-microblaze/exec.h b/target-microblaze/exec.h index 87b2494..31a14fe 100644 --- a/target-microblaze/exec.h +++ b/target-microblaze/exec.h @@ -18,8 +18,6 @@ */ #include "dyngen-exec.h" -register struct CPUMBState *env asm(AREG0); - #include "cpu.h" #include "exec-all.h" diff --git a/target-microblaze/op_helper.c b/target-microblaze/op_helper.c index be0c829..07992b7 100644 --- a/target-microblaze/op_helper.c +++ b/target-microblaze/op_helper.c @@ -17,6 +17,7 @@ * License along with this library; if not, see <http://www.gnu.org/licenses/>. */ +#define NEED_GLOBAL_ENV #include <assert.h> #include "exec.h" #include "helper.h" diff --git a/target-mips/exec.h b/target-mips/exec.h index af61b54..3cb3201 100644 --- a/target-mips/exec.h +++ b/target-mips/exec.h @@ -8,8 +8,6 @@ #include "dyngen-exec.h" #include "cpu-defs.h" -register struct CPUMIPSState *env asm(AREG0); - #include "cpu.h" #include "exec-all.h" diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c index 344e0bd..b06bb5d 100644 --- a/target-mips/op_helper.c +++ b/target-mips/op_helper.c @@ -16,6 +16,8 @@ * 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/>. */ + +#define NEED_GLOBAL_ENV #include <stdlib.h> #include "exec.h" diff --git a/target-ppc/exec.h b/target-ppc/exec.h index 44cc5e9..b835273 100644 --- a/target-ppc/exec.h +++ b/target-ppc/exec.h @@ -29,8 +29,6 @@ /* Precise emulation is needed to correctly emulate exception flags */ #define USE_PRECISE_EMULATION 1 -register struct CPUPPCState *env asm(AREG0); - #if !defined(CONFIG_USER_ONLY) #include "softmmu_exec.h" #endif /* !defined(CONFIG_USER_ONLY) */ diff --git a/target-ppc/op_helper.c b/target-ppc/op_helper.c index 3c3aa60..a10c233 100644 --- a/target-ppc/op_helper.c +++ b/target-ppc/op_helper.c @@ -16,6 +16,8 @@ * 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/>. */ + +#define NEED_GLOBAL_ENV #include <string.h> #include "exec.h" #include "host-utils.h" diff --git a/target-s390x/exec.h b/target-s390x/exec.h index bf3f264..e87622a 100644 --- a/target-s390x/exec.h +++ b/target-s390x/exec.h @@ -19,8 +19,6 @@ #include "dyngen-exec.h" -register struct CPUS390XState *env asm(AREG0); - #include "config.h" #include "cpu.h" #include "exec-all.h" diff --git a/target-s390x/op_helper.c b/target-s390x/op_helper.c index 402df2d..a3b66b8 100644 --- a/target-s390x/op_helper.c +++ b/target-s390x/op_helper.c @@ -17,6 +17,7 @@ * License along with this library; if not, see <http://www.gnu.org/licenses/>. */ +#define NEED_GLOBAL_ENV #include "exec.h" /*****************************************************************************/ diff --git a/target-sh4/exec.h b/target-sh4/exec.h index 2999c02..73c06ad 100644 --- a/target-sh4/exec.h +++ b/target-sh4/exec.h @@ -22,8 +22,6 @@ #include "config.h" #include "dyngen-exec.h" -register struct CPUSH4State *env asm(AREG0); - #include "cpu.h" #include "exec-all.h" diff --git a/target-sh4/op_helper.c b/target-sh4/op_helper.c index 2e5f555..d29d75e 100644 --- a/target-sh4/op_helper.c +++ b/target-sh4/op_helper.c @@ -18,6 +18,8 @@ */ #include <assert.h> #include <stdlib.h> + +#define NEED_GLOBAL_ENV #include "exec.h" #include "helper.h" diff --git a/target-sparc/exec.h b/target-sparc/exec.h index f811571..db6ed49 100644 --- a/target-sparc/exec.h +++ b/target-sparc/exec.h @@ -3,8 +3,6 @@ #include "config.h" #include "dyngen-exec.h" -register struct CPUSPARCState *env asm(AREG0); - #include "cpu.h" #include "exec-all.h" diff --git a/target-sparc/op_helper.c b/target-sparc/op_helper.c index be3c1e0..5ad1fac 100644 --- a/target-sparc/op_helper.c +++ b/target-sparc/op_helper.c @@ -1,3 +1,4 @@ +#define NEED_GLOBAL_ENV #include "exec.h" #include "host-utils.h" #include "helper.h"
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- cpu-exec.c | 2 ++ exec-all.h | 4 ++++ target-alpha/exec.h | 2 -- target-alpha/op_helper.c | 1 + target-arm/exec.h | 2 -- target-arm/op_helper.c | 1 + target-cris/exec.h | 2 -- target-cris/op_helper.c | 1 + target-i386/exec.h | 2 -- target-i386/op_helper.c | 1 + target-m68k/exec.h | 2 -- target-m68k/op_helper.c | 2 ++ target-microblaze/exec.h | 2 -- target-microblaze/op_helper.c | 1 + target-mips/exec.h | 2 -- target-mips/op_helper.c | 2 ++ target-ppc/exec.h | 2 -- target-ppc/op_helper.c | 2 ++ target-s390x/exec.h | 2 -- target-s390x/op_helper.c | 1 + target-sh4/exec.h | 2 -- target-sh4/op_helper.c | 2 ++ target-sparc/exec.h | 2 -- target-sparc/op_helper.c | 1 + 24 files changed, 21 insertions(+), 22 deletions(-)