[Qemu-devel] Re: [PATCH 4/4] require #define NEED_GLOBAL_ENV for files that need the global register variable

[Qemu-devel] Re: [PATCH 4/4] require #define NEED_GLOBAL_ENV for files that need the global register variable

[Paolo Bonzini]

>>> 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

[Qemu-devel] Re: [PATCH 4/4] require #define NEED_GLOBAL_ENV for files that need the global register variable

[Blue Swirl]

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.

[Qemu-devel] Re: [PATCH 4/4] require #define NEED_GLOBAL_ENV for files that need the global register variable

[Paolo Bonzini]

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

[Qemu-devel] Re: [PATCH 4/4] require #define NEED_GLOBAL_ENV for files that need the global register variable

[Blue Swirl]

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.

[Qemu-devel] Re: [PATCH 4/4] require #define NEED_GLOBAL_ENV for files that need the global register variable

[Paolo Bonzini]

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

Re: [Qemu-devel] Re: [PATCH 4/4] require #define NEED_GLOBAL_ENV for files that need the global register variable

[Paul Brook]

> >> 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.

[Qemu-devel] Re: [PATCH 4/4] require #define NEED_GLOBAL_ENV for files that need the global register variable

[Blue Swirl]

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.

[Qemu-devel] Re: [PATCH 4/4] require #define NEED_GLOBAL_ENV for files that need the global register variable

[Paolo Bonzini]

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

[Qemu-devel] Re: [PATCH 4/4] require #define NEED_GLOBAL_ENV for files that need the global register variable

[Paul Brook]

> 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

[Qemu-devel] Re: [PATCH 4/4] require #define NEED_GLOBAL_ENV for files that need the global register variable

[Paolo Bonzini]

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

Re: [Qemu-devel] Re: [PATCH 4/4] require #define NEED_GLOBAL_ENV for files that need the global register variable

[Paolo Bonzini]

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

Re: [Qemu-devel] Re: [PATCH 4/4] require #define NEED_GLOBAL_ENV for files that need the global register variable

[Paul Brook]

> 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

[Qemu-devel] Re: [PATCH 4/4] require #define NEED_GLOBAL_ENV for files that need the global register variable

[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.

Paolo