* [Qemu-devel] Re: [PATCH 4/4] require #define NEED_GLOBAL_ENV for files that need the global register variable [not found] <5692464.1280361277887491249.JavaMail.root@zmail07.collab.prod.int.phx2.redhat.com> @ 2010-06-30 8:56 ` Paolo Bonzini 2010-07-01 19:42 ` Blue Swirl 0 siblings, 1 reply; 13+ messages in thread From: Paolo Bonzini @ 2010-06-30 8:56 UTC (permalink / raw) To: Blue Swirl; +Cc: Paul Brook, qemu-devel >>> 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. It doesn't matter what's the name of the global. What matters is whether it's defined at all. For this reason it's bad to bury it in dyngen-exec.h which is included only indirectly. It's better to leave it in all */exec.h files as Paul explained---and I agree with him. I also gave reason why unpoisoning env globally is not a problem at all. For target-dependent files, they did not (and do not) poison anything, so my first patch series didn't change anything WRT current QEMU sources. exec.h always includes cpu.h, so there's no way exec.h can be included by mistake in a target-independent file. I can make exec.h error out if NEED_CPU_H is not defined, but I think it's a worthless complication. So, can someone please apply patches 1 to 3 of this series so that we can move on? Paolo ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] Re: [PATCH 4/4] require #define NEED_GLOBAL_ENV for files that need the global register variable 2010-06-30 8:56 ` [Qemu-devel] Re: [PATCH 4/4] require #define NEED_GLOBAL_ENV for files that need the global register variable Paolo Bonzini @ 2010-07-01 19:42 ` Blue Swirl 2010-07-02 9:50 ` Paolo Bonzini 0 siblings, 1 reply; 13+ messages in thread From: Blue Swirl @ 2010-07-01 19:42 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Paul Brook, qemu-devel On Wed, Jun 30, 2010 at 8:56 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: >>>> 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. > > It doesn't matter what's the name of the global. Yes it does. Unfortunately, 'env' is a name which is used for other purposes than the global register variable pointing to CPUState. Any of these uses could accidentally refer to global 'env'. Also one bug was where the function had parameter 'env1', but some code inside still used 'env'. These kind of bugs can be avoided by simply renaming 'env' to less collision prone name. > What matters is > whether it's defined at all. For this reason it's bad to bury it > in dyngen-exec.h which is included only indirectly. It's better to > leave it in all */exec.h files as Paul explained---and I agree with > him. Fine by me too. > I also gave reason why unpoisoning env globally is not a problem at > all. For target-dependent files, they did not (and do not) poison > anything, so my first patch series didn't change anything WRT current > QEMU sources. exec.h always includes cpu.h, so there's no way exec.h > can be included by mistake in a target-independent file. I can make > exec.h error out if NEED_CPU_H is not defined, but I think it's a > worthless complication. I still maintain that 'env' may not be unpoisoned until the name is less likely to invite accidents. > > So, can someone please apply patches 1 to 3 of this series so that > we can move on? I think they are nice cleanups regardless of the approach for 'env'. If nobody has any objections, I'll apply them on Saturday. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] Re: [PATCH 4/4] require #define NEED_GLOBAL_ENV for files that need the global register variable 2010-07-01 19:42 ` Blue Swirl @ 2010-07-02 9:50 ` Paolo Bonzini 2010-07-03 7:23 ` Blue Swirl 0 siblings, 1 reply; 13+ messages in thread From: Paolo Bonzini @ 2010-07-02 9:50 UTC (permalink / raw) To: Blue Swirl; +Cc: Paul Brook, qemu-devel On 07/01/2010 09:42 PM, Blue Swirl wrote: > I still maintain that 'env' may not be unpoisoned until the name is > less likely to invite accidents. The *global* env is still unavailable (i.e. no difference WRT poisoning), by virtue of being defined in exec.h which is not available unless -DNEED_CPU_H is defined. So: | before | after ------------+---------------------------------+-------------------------- NEED_CPU_H | env not poisoned, global env | same | available iff exec.h included | ------------+---------------------------------+-------------------------- !NEED_CPU_H | env poisoned; CPUState | env not poisoned; | not available, so exec.h cannot | exec.h requires cpu.h | be included | so it cannot be included Paolo ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] Re: [PATCH 4/4] require #define NEED_GLOBAL_ENV for files that need the global register variable 2010-07-02 9:50 ` Paolo Bonzini @ 2010-07-03 7:23 ` Blue Swirl 2010-07-08 16:39 ` Paolo Bonzini 0 siblings, 1 reply; 13+ messages in thread From: Blue Swirl @ 2010-07-03 7:23 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Paul Brook, qemu-devel On Fri, Jul 2, 2010 at 9:50 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 07/01/2010 09:42 PM, Blue Swirl wrote: >> >> I still maintain that 'env' may not be unpoisoned until the name is >> less likely to invite accidents. > > The *global* env is still unavailable (i.e. no difference WRT poisoning), by > virtue of being defined in exec.h which is not available unless -DNEED_CPU_H > is defined. > > So: > > | before | after > ------------+---------------------------------+-------------------------- > NEED_CPU_H | env not poisoned, global env | same > | available iff exec.h included | > ------------+---------------------------------+-------------------------- > !NEED_CPU_H | env poisoned; CPUState | env not poisoned; > | not available, so exec.h cannot | exec.h requires cpu.h > | be included | so it cannot be included But why would it be necessary to unpoison 'env' for the lower right case? Poisoning protects against accidents (in addition to exec.h thing) and there are no downsides. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] Re: [PATCH 4/4] require #define NEED_GLOBAL_ENV for files that need the global register variable 2010-07-03 7:23 ` Blue Swirl @ 2010-07-08 16:39 ` Paolo Bonzini 0 siblings, 0 replies; 13+ messages in thread From: Paolo Bonzini @ 2010-07-08 16:39 UTC (permalink / raw) To: Blue Swirl; +Cc: Paul Brook, qemu-devel On 07/03/2010 09:23 AM, Blue Swirl wrote: > On Fri, Jul 2, 2010 at 9:50 AM, Paolo Bonzini<pbonzini@redhat.com> wrote: >> The *global* env is still unavailable (i.e. no difference WRT poisoning), by >> virtue of being defined in exec.h which is not available unless -DNEED_CPU_H >> is defined. >> >> So: >> >> | before | after >> ------------+---------------------------------+-------------------------- >> NEED_CPU_H | env not poisoned, global env | same >> | available iff exec.h included | >> ------------+---------------------------------+-------------------------- >> !NEED_CPU_H | env poisoned; CPUState | env not poisoned; >> | not available, so exec.h cannot | exec.h requires cpu.h >> | be included | so it cannot be included > > But why would it be necessary to unpoison 'env' for the lower right > case? Poisoning protects against accidents (in addition to exec.h > thing) and there are no downsides. Because double protection is useless. Two warnings may be better than one, but we're talking about compilation errors and two errors do not achieve anything more than one error. Plus, the policy for code using the global `env' is clear and is not going to change, and including exec.h outside cpu-exec.c/op_helper.c would be spotted instantly during code review. And the downside, of course, is that you're holding up a patch to improve QEMU's type-safety. If you want to poison env on all files that do not include exec.h that's another story. I think it's a good idea, but it's not what the current code does. So my pending patches would not make anything worse compared to the status quo, and it's something that can be done after my existing patches go in. Besides, I'm on vacation and I try to enjoy nature more than hacking for these three weeks. :) So if you want me to rebase/resend, that's half hour work and I'll do it gladly, otherwise I'll pass the ball. Paolo ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH 0/4] introduce NEED_GLOBAL_ENV
@ 2010-06-28 17:17 Paolo Bonzini
2010-06-28 17:17 ` [Qemu-devel] [PATCH 4/4] require #define NEED_GLOBAL_ENV for files that need the global register variable Paolo Bonzini
0 siblings, 1 reply; 13+ messages in thread
From: Paolo Bonzini @ 2010-06-28 17:17 UTC (permalink / raw)
To: qemu-devel; +Cc: blauwirbel
Let's start the cleanups from the feature required by Blue Swirl.
I also include here a baby step towards removing eminently TCG-related
stuff from cpu.h.
After this series, only a bunch of files will include exec-all.h,
instead of getting it indirectly from cpu.h.
Note that (as sworn in the previous submission) exec.h is only included
by files that need the global register variable (i.e. cpu-exec.c and
target-*/op_helper.c), and this is the same subset that gets
NEED_GLOBAL_ENV in this patchset.
i386 and sparc have functions declared in cpu.h that are in op_helper.c.
I checked that these do not need the global variable, but it would be
nice to cleanup those too.
Paolo Bonzini (4):
remove unused stuff from */exec.h
move cpu_pc_from_tb to target-*/exec.h
remove exec-all.h inclusion from cpu.h
require #define NEED_GLOBAL_ENV for files that need the global
register variable
cpu-exec.c | 2 ++
exec-all.h | 4 ++++
gdbstub.c | 1 +
hw/xen_domainbuild.c | 1 +
kvm-stub.c | 1 +
monitor.c | 1 +
target-alpha/cpu.h | 6 ------
target-alpha/exec.h | 9 +++++----
target-alpha/op_helper.c | 1 +
target-arm/cpu.h | 6 ------
target-arm/exec.h | 8 ++++++--
target-arm/op_helper.c | 1 +
target-cris/cpu.h | 6 ------
target-cris/exec.h | 11 ++++++-----
target-cris/op_helper.c | 1 +
target-i386/cpu.h | 7 -------
target-i386/exec.h | 17 ++++++-----------
target-i386/op_helper.c | 3 ++-
target-m68k/cpu.h | 6 ------
target-m68k/exec.h | 8 ++++++--
target-m68k/op_helper.c | 2 ++
target-microblaze/cpu.h | 6 ------
target-microblaze/exec.h | 10 ++++++----
target-microblaze/op_helper.c | 1 +
target-mips/cpu.h | 8 --------
target-mips/exec.h | 17 +++++++----------
target-mips/op_helper.c | 7 +++++++
target-ppc/cpu.h | 6 ------
target-ppc/exec.h | 7 +++++--
target-ppc/op_helper.c | 2 ++
target-s390x/cpu.h | 6 ------
target-s390x/exec.h | 8 ++++++--
target-s390x/op_helper.c | 1 +
target-sh4/cpu.h | 7 -------
target-sh4/exec.h | 8 ++++++--
target-sh4/op_helper.c | 2 ++
target-sparc/cpu.h | 7 -------
target-sparc/exec.h | 8 ++++++--
target-sparc/op_helper.c | 1 +
39 files changed, 96 insertions(+), 118 deletions(-)
^ permalink raw reply [flat|nested] 13+ messages in thread* [Qemu-devel] [PATCH 4/4] require #define NEED_GLOBAL_ENV for files that need the global register variable 2010-06-28 17:17 [Qemu-devel] [PATCH 0/4] introduce NEED_GLOBAL_ENV Paolo Bonzini @ 2010-06-28 17:17 ` Paolo Bonzini 2010-06-28 20:29 ` Paul Brook 0 siblings, 1 reply; 13+ messages in thread From: Paolo Bonzini @ 2010-06-28 17:17 UTC (permalink / raw) To: qemu-devel; +Cc: blauwirbel 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(-) 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" -- 1.7.0.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 4/4] require #define NEED_GLOBAL_ENV for files that need the global register variable 2010-06-28 17:17 ` [Qemu-devel] [PATCH 4/4] require #define NEED_GLOBAL_ENV for files that need the global register variable Paolo Bonzini @ 2010-06-28 20:29 ` Paul Brook 2010-06-29 7:44 ` [Qemu-devel] " Paolo Bonzini 0 siblings, 1 reply; 13+ messages in thread From: Paul Brook @ 2010-06-28 20:29 UTC (permalink / raw) To: qemu-devel; +Cc: blauwirbel, Paolo Bonzini > 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] Re: [PATCH 4/4] require #define NEED_GLOBAL_ENV for files that need the global register variable 2010-06-28 20:29 ` Paul Brook @ 2010-06-29 7:44 ` Paolo Bonzini 2010-06-29 11:30 ` Paul Brook 2010-06-29 18:22 ` Blue Swirl 0 siblings, 2 replies; 13+ messages in thread From: Paolo Bonzini @ 2010-06-29 7:44 UTC (permalink / raw) To: Paul Brook; +Cc: blauwirbel, qemu-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] Re: [PATCH 4/4] require #define NEED_GLOBAL_ENV for files that need the global register variable 2010-06-29 7:44 ` [Qemu-devel] " Paolo Bonzini @ 2010-06-29 11:30 ` Paul Brook 2010-06-29 13:51 ` Paolo Bonzini 2010-06-29 18:22 ` Blue Swirl 1 sibling, 1 reply; 13+ messages in thread From: Paul Brook @ 2010-06-29 11:30 UTC (permalink / raw) To: qemu-devel; +Cc: blauwirbel, Paolo Bonzini > 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] Re: [PATCH 4/4] require #define NEED_GLOBAL_ENV for files that need the global register variable 2010-06-29 11:30 ` Paul Brook @ 2010-06-29 13:51 ` Paolo Bonzini 2010-06-29 14:24 ` Paolo Bonzini 0 siblings, 1 reply; 13+ messages in thread From: Paolo Bonzini @ 2010-06-29 13:51 UTC (permalink / raw) To: Paul Brook; +Cc: blauwirbel, qemu-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] Re: [PATCH 4/4] require #define NEED_GLOBAL_ENV for files that need the global register variable 2010-06-29 13:51 ` Paolo Bonzini @ 2010-06-29 14:24 ` Paolo Bonzini 2010-06-29 15:28 ` Paul Brook 0 siblings, 1 reply; 13+ messages in thread From: Paolo Bonzini @ 2010-06-29 14:24 UTC (permalink / raw) Cc: blauwirbel, Paul Brook, qemu-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] Re: [PATCH 4/4] require #define NEED_GLOBAL_ENV for files that need the global register variable 2010-06-29 14:24 ` Paolo Bonzini @ 2010-06-29 15:28 ` Paul Brook 2010-06-29 15:40 ` Paolo Bonzini 0 siblings, 1 reply; 13+ messages in thread From: Paul Brook @ 2010-06-29 15:28 UTC (permalink / raw) To: Paolo Bonzini; +Cc: blauwirbel, qemu-devel > 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] Re: [PATCH 4/4] require #define NEED_GLOBAL_ENV for files that need the global register variable 2010-06-29 15:28 ` Paul Brook @ 2010-06-29 15:40 ` Paolo Bonzini 2010-06-30 0:34 ` Paul Brook 0 siblings, 1 reply; 13+ messages in thread From: Paolo Bonzini @ 2010-06-29 15:40 UTC (permalink / raw) To: Paul Brook; +Cc: blauwirbel, qemu-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] Re: [PATCH 4/4] require #define NEED_GLOBAL_ENV for files that need the global register variable 2010-06-29 15:40 ` Paolo Bonzini @ 2010-06-30 0:34 ` Paul Brook 0 siblings, 0 replies; 13+ messages in thread From: Paul Brook @ 2010-06-30 0:34 UTC (permalink / raw) To: qemu-devel; +Cc: blauwirbel, Paolo Bonzini > >> 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] Re: [PATCH 4/4] require #define NEED_GLOBAL_ENV for files that need the global register variable 2010-06-29 7:44 ` [Qemu-devel] " Paolo Bonzini 2010-06-29 11:30 ` Paul Brook @ 2010-06-29 18:22 ` Blue Swirl 1 sibling, 0 replies; 13+ messages in thread From: Blue Swirl @ 2010-06-29 18:22 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Paul Brook, qemu-devel 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2010-07-08 16:39 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <5692464.1280361277887491249.JavaMail.root@zmail07.collab.prod.int.phx2.redhat.com>
2010-06-30 8:56 ` [Qemu-devel] Re: [PATCH 4/4] require #define NEED_GLOBAL_ENV for files that need the global register variable Paolo Bonzini
2010-07-01 19:42 ` Blue Swirl
2010-07-02 9:50 ` Paolo Bonzini
2010-07-03 7:23 ` Blue Swirl
2010-07-08 16:39 ` Paolo Bonzini
2010-06-28 17:17 [Qemu-devel] [PATCH 0/4] introduce NEED_GLOBAL_ENV Paolo Bonzini
2010-06-28 17:17 ` [Qemu-devel] [PATCH 4/4] require #define NEED_GLOBAL_ENV for files that need the global register variable Paolo Bonzini
2010-06-28 20:29 ` Paul Brook
2010-06-29 7:44 ` [Qemu-devel] " Paolo Bonzini
2010-06-29 11:30 ` Paul Brook
2010-06-29 13:51 ` Paolo Bonzini
2010-06-29 14:24 ` Paolo Bonzini
2010-06-29 15:28 ` Paul Brook
2010-06-29 15:40 ` Paolo Bonzini
2010-06-30 0:34 ` Paul Brook
2010-06-29 18:22 ` Blue Swirl
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).