| Message ID | 1334936646-13807-1-git-send-email-peter.portante@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 20.04.2012 17:44, schrieb Peter Portante: > Signed-off-by: Peter Portante <peter.portante@redhat.com> Fix itself looks okay, but author, maintainer and qemu-ppc were missing in CC, and a better commit message would be: ---8<--- pseries: Fix use of global CPU state Commit ed120055c7f9b26b5707d3ceabbe5a3f06aaf937 (Implement PAPR VPA functions for pSeries shared processor partitions) introduced the register_dtl() function and typo "emv" as name of its argument. This went unnoticed because the code in that function can access the global variable "env" so that no build failure resulted. Fix the argument to read "env". Resolves LP#986241. ---8<--- Andreas > --- > hw/spapr_hcall.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/hw/spapr_hcall.c b/hw/spapr_hcall.c > index 634763e..94bb504 100644 > --- a/hw/spapr_hcall.c > +++ b/hw/spapr_hcall.c > @@ -482,7 +482,7 @@ static target_ulong register_dtl(CPUPPCState *env, target_ulong addr) > return H_SUCCESS; > } > > -static target_ulong deregister_dtl(CPUPPCState *emv, target_ulong addr) > +static target_ulong deregister_dtl(CPUPPCState *env, target_ulong addr) > { > env->dispatch_trace_log = 0; > env->dtl_size = 0;
On 22 April 2012 20:56, Andreas Färber <afaerber@suse.de> wrote: > Commit ed120055c7f9b26b5707d3ceabbe5a3f06aaf937 (Implement PAPR VPA > functions for pSeries shared processor partitions) introduced the > register_dtl() function and typo "emv" as name of its argument. > This went unnoticed because the code in that function can access the > global variable "env" so that no build failure resulted. > > Fix the argument to read "env". Resolves LP#986241. Incidentally, I had a quick look through that file and couldn't see any uses of 'env' outside of functions which pass it in -- is it possible to drop the dyngen-exec.h include altogether? -- PMM
Am 22.04.2012 22:06, schrieb Peter Maydell: > On 22 April 2012 20:56, Andreas Färber <afaerber@suse.de> wrote: >> Commit ed120055c7f9b26b5707d3ceabbe5a3f06aaf937 (Implement PAPR VPA >> functions for pSeries shared processor partitions) introduced the >> register_dtl() function and typo "emv" as name of its argument. >> This went unnoticed because the code in that function can access the >> global variable "env" so that no build failure resulted. >> >> Fix the argument to read "env". Resolves LP#986241. > > Incidentally, I had a quick look through that file and couldn't > see any uses of 'env' outside of functions which pass it in -- > is it possible to drop the dyngen-exec.h include altogether? A quick build test confirms that, yes. Blue's AREG0 series does not seem to touch this file, so we could do it independently as a follow-up to this one. Andreas
On Sun, Apr 22, 2012 at 09:56:09PM +0200, Andreas Färber wrote: > Am 20.04.2012 17:44, schrieb Peter Portante: > > Signed-off-by: Peter Portante <peter.portante@redhat.com> > > Fix itself looks okay, but author, maintainer and qemu-ppc were missing > in CC, and a better commit message would be: > > ---8<--- > pseries: Fix use of global CPU state > > Commit ed120055c7f9b26b5707d3ceabbe5a3f06aaf937 (Implement PAPR VPA > functions for pSeries shared processor partitions) introduced the > register_dtl() function and typo "emv" as name of its argument. > This went unnoticed because the code in that function can access the > global variable "env" so that no build failure resulted. > > Fix the argument to read "env". Resolves LP#986241. Ouch. Good catch.
On Sun, Apr 22, 2012 at 3:56 PM, Andreas Färber <afaerber@suse.de> wrote: > Am 20.04.2012 17:44, schrieb Peter Portante: > > Signed-off-by: Peter Portante <peter.portante@redhat.com> > > Fix itself looks okay, but author, maintainer and qemu-ppc were missing > in CC, and a better commit message would be: > > ---8<--- > pseries: Fix use of global CPU state > > Commit ed120055c7f9b26b5707d3ceabbe5a3f06aaf937 (Implement PAPR VPA > functions for pSeries shared processor partitions) introduced the > register_dtl() function and typo "emv" as name of its argument. > This went unnoticed because the code in that function can access the > global variable "env" so that no build failure resulted. > > Fix the argument to read "env". Resolves LP#986241. > ---8<--- > Thanks Andreas. I'll resubmit the patch tomorrow. -peter > Andreas > > > --- > > hw/spapr_hcall.c | 2 +- > > 1 files changed, 1 insertions(+), 1 deletions(-) > > > > diff --git a/hw/spapr_hcall.c b/hw/spapr_hcall.c > > index 634763e..94bb504 100644 > > --- a/hw/spapr_hcall.c > > +++ b/hw/spapr_hcall.c > > @@ -482,7 +482,7 @@ static target_ulong register_dtl(CPUPPCState *env, > target_ulong addr) > > return H_SUCCESS; > > } > > > > -static target_ulong deregister_dtl(CPUPPCState *emv, target_ulong addr) > > +static target_ulong deregister_dtl(CPUPPCState *env, target_ulong addr) > > { > > env->dispatch_trace_log = 0; > > env->dtl_size = 0; > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg > >
On Sun, Apr 22, 2012 at 9:40 PM, Peter Portante <peter.a.portante@gmail.com>wrote: > On Sun, Apr 22, 2012 at 3:56 PM, Andreas Färber <afaerber@suse.de> wrote: > >> Am 20.04.2012 17:44, schrieb Peter Portante: >> > Signed-off-by: Peter Portante <peter.portante@redhat.com> >> >> Fix itself looks okay, but author, maintainer and qemu-ppc were missing >> in CC, and a better commit message would be: >> >> ---8<--- >> pseries: Fix use of global CPU state >> >> Commit ed120055c7f9b26b5707d3ceabbe5a3f06aaf937 (Implement PAPR VPA >> functions for pSeries shared processor partitions) introduced the >> register_dtl() function and typo "emv" as name of its argument. >> This went unnoticed because the code in that function can access the >> global variable "env" so that no build failure resulted. >> >> Fix the argument to read "env". Resolves LP#986241. >> ---8<--- >> > > Thanks Andreas. I'll resubmit the patch tomorrow. -peter > > >> Andreas >> >> > --- >> > hw/spapr_hcall.c | 2 +- >> > 1 files changed, 1 insertions(+), 1 deletions(-) >> > >> > diff --git a/hw/spapr_hcall.c b/hw/spapr_hcall.c >> > index 634763e..94bb504 100644 >> > --- a/hw/spapr_hcall.c >> > +++ b/hw/spapr_hcall.c >> > @@ -482,7 +482,7 @@ static target_ulong register_dtl(CPUPPCState *env, >> target_ulong addr) >> > return H_SUCCESS; >> > } >> > >> > -static target_ulong deregister_dtl(CPUPPCState *emv, target_ulong addr) >> > +static target_ulong deregister_dtl(CPUPPCState *env, target_ulong addr) >> > { >> > env->dispatch_trace_log = 0; >> > env->dtl_size = 0; >> >> -- >> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany >> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg >> >> > And I planned on submitting the other changes to remove the unused includes of dyngen-exec.h in a separate patch.
On 04/22/2012 09:22 PM, David Gibson wrote: > On Sun, Apr 22, 2012 at 09:56:09PM +0200, Andreas Färber wrote: >> Am 20.04.2012 17:44, schrieb Peter Portante: >>> Signed-off-by: Peter Portante<peter.portante@redhat.com> >> Fix itself looks okay, but author, maintainer and qemu-ppc were missing >> in CC, and a better commit message would be: >> >> ---8<--- >> pseries: Fix use of global CPU state >> >> Commit ed120055c7f9b26b5707d3ceabbe5a3f06aaf937 (Implement PAPR VPA >> functions for pSeries shared processor partitions) introduced the >> register_dtl() function and typo "emv" as name of its argument. >> This went unnoticed because the code in that function can access the >> global variable "env" so that no build failure resulted. >> >> Fix the argument to read "env". Resolves LP#986241. > Ouch. Good catch. Found by playing around with -flto with gcc. Ran into problems with link time common warnings due to register declaration in dyngen-exec.h being included in multiple .c files where they referenced env. Rebuilt with --enabled-tcg-interpretter (which causes dyngen-exec.h to only emit an extern declaration), and ifdef'd out the storage declarations in other .c files, and voila, undefined reference to env from within hw/spapr_hcall.c. Do folks seen this as a maintenance headache to have a global variable name used as a parameter name in function arguments as well? Can somebody speak to the history of using a global variable for env state instead of passing it around in function calls? Is this because we could not trust the compiler to keep it in a register between function calls? If so, then do you think it might be worth a look at what the compilers can do to help simplify somewhat complicated code and still maintain the performance? Or was it that env had to be added to so many functions that it became too tedious to maintain? Regards, -peter
On 23 April 2012 03:25, Peter Portante <pportant@redhat.com> wrote: > Can somebody speak to the history of using a global variable for env state > instead of passing it around in function calls? Is this because we could not > trust the compiler to keep it in a register between function calls? If so, > then do you think it might be worth a look at what the compilers can do to > help simplify somewhat complicated code and still maintain the performance? > Or was it that env had to be added to so many functions that it became too > tedious to maintain? It's historical legacy, really. env is in a fixed register when running in TCG code, and so some functions called directly from TCG code have also been compiled to assume env to be in that fixed register, to avoid the overhead of having TCG code marshall it in as a function parameter. Blue Swirl has been working on patchsets that move towards always passing it in a register; unfortunately there is apparently a performance hit associated with this, though. -- PMM
On Sun, Apr 22, 2012 at 3:56 PM, Andreas Färber <afaerber@suse.de> wrote: > Am 20.04.2012 17:44, schrieb Peter Portante: > > Signed-off-by: Peter Portante <peter.portante@redhat.com> > > Fix itself looks okay, but author, maintainer and qemu-ppc were missing > in CC, and a better commit message would be: > > ---8<--- > pseries: Fix use of global CPU state > > Commit ed120055c7f9b26b5707d3ceabbe5a3f06aaf937 (Implement PAPR VPA > functions for pSeries shared processor partitions) introduced the > register_dtl() function and typo "emv" as name of its argument. > This went unnoticed because the code in that function can access the > global variable "env" so that no build failure resulted. > > Fix the argument to read "env". Resolves LP#986241. > ---8<--- > > Resubmitted this patch, subject: .[Patch v2] pseries: Fix use of global CPU state Thanks, -peter > Andreas > > > --- > > hw/spapr_hcall.c | 2 +- > > 1 files changed, 1 insertions(+), 1 deletions(-) > > > > diff --git a/hw/spapr_hcall.c b/hw/spapr_hcall.c > > index 634763e..94bb504 100644 > > --- a/hw/spapr_hcall.c > > +++ b/hw/spapr_hcall.c > > @@ -482,7 +482,7 @@ static target_ulong register_dtl(CPUPPCState *env, > target_ulong addr) > > return H_SUCCESS; > > } > > > > -static target_ulong deregister_dtl(CPUPPCState *emv, target_ulong addr) > > +static target_ulong deregister_dtl(CPUPPCState *env, target_ulong addr) > > { > > env->dispatch_trace_log = 0; > > env->dtl_size = 0; > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg > >
diff --git a/hw/spapr_hcall.c b/hw/spapr_hcall.c index 634763e..94bb504 100644 --- a/hw/spapr_hcall.c +++ b/hw/spapr_hcall.c @@ -482,7 +482,7 @@ static target_ulong register_dtl(CPUPPCState *env, target_ulong addr) return H_SUCCESS; } -static target_ulong deregister_dtl(CPUPPCState *emv, target_ulong addr) +static target_ulong deregister_dtl(CPUPPCState *env, target_ulong addr) { env->dispatch_trace_log = 0; env->dtl_size = 0;
Signed-off-by: Peter Portante <peter.portante@redhat.com> --- hw/spapr_hcall.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)