Re: [Qemu-devel] [PATCH] PPC: Add wrapper for target long DCR operations

[Alexander Graf]

Am 06.02.2010 um 17:15 schrieb Aurelien Jarno <address@hidden>:

> On Thu, Jan 14, 2010 at 06:04:25PM +0100, Alexander Graf wrote:
> > On 14.01.2010, at 18:02, Aurelien Jarno wrote:
> >
> > > On Thu, Jan 14, 2010 at 04:19:31PM +0100, Alexander Graf wrote:
> > > > On 14.01.2010, at 16:13, Aurelien Jarno wrote:
> > > >
> > > > > On Fri, Jan 01, 2010 at 04:41:06PM +0100, Alexander Graf wrote:
> > > > > > The recent transition to always have the DCR helper functions  
> > > > > > take 32 bit
> > > > > > values broke the PPC64 target, as tlong became 64 bits there.
> > > > > >
> > > > > > This patch moves all translate.c callers to a _tl function that  
> > > > > > simply
> > > > > > calls the uint32_t functions. That way we don't need to mess  
> > > > > > with TCG
> > > > > > trying to pass registers as uint32_t variables to functions.
> > > > > >
> > > > > > Fixes PPC64 build with --enable-debug-tcg
> > > > > >
> > > > > > Signed-off-by: Alexander Graf <address@hidden>
> > > > > > Reported-by: Stefan Weil <address@hidden>
> > > > > > ---
> > > > > > target-ppc/cpu.h       |    2 ++
> > > > > > target-ppc/helper.h    |    4 ++--
> > > > > > target-ppc/op_helper.c |   10 ++++++++++
> > > > > > target-ppc/translate.c |   12 ++++++------
> > > > > > 4 files changed, 20 insertions(+), 8 deletions(-)
> > > > > >
> > > > > > diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> > > > > > index d15bba1..60a8b68 100644
> > > > > > --- a/target-ppc/cpu.h
> > > > > > +++ b/target-ppc/cpu.h
> > > > > > @@ -733,6 +733,8 @@ void ppc_store_slb (CPUPPCState *env,  
> > > > > > target_ulong rb, target_ulong rs);
> > > > > > void ppc_store_sr (CPUPPCState *env, int srnum, target_ulong  
> > > > > > value);
> > > > > > #endif /* !defined(CONFIG_USER_ONLY) */
> > > > > > void ppc_store_msr (CPUPPCState *env, target_ulong value);
> > > > > > +void helper_store_dcr (uint32_t dcrn, uint32_t val);
> > > > > > +uint32_t helper_load_dcr (uint32_t dcrn);
> > > > > >
> > > > > > void ppc_cpu_list (FILE *f, int (*cpu_fprintf)(FILE *f, const  
> > > > > > char *fmt, ...));
> > > > > >
> > > > > > diff --git a/target-ppc/helper.h b/target-ppc/helper.h
> > > > > > index 40d4ced..86f0af7 100644
> > > > > > --- a/target-ppc/helper.h
> > > > > > +++ b/target-ppc/helper.h
> > > > > > @@ -359,8 +359,8 @@ DEF_HELPER_2(divo, tl, tl, tl)
> > > > > > DEF_HELPER_2(divs, tl, tl, tl)
> > > > > > DEF_HELPER_2(divso, tl, tl, tl)
> > > > > >
> > > > > > -DEF_HELPER_1(load_dcr, i32, i32);
> > > > > > -DEF_HELPER_2(store_dcr, void, i32, i32)
> > > > > > +DEF_HELPER_1(load_dcr_tl, tl, tl);
> > > > > > +DEF_HELPER_2(store_dcr_tl, void, tl, tl)
> > > > > >
> > > > > > DEF_HELPER_1(load_dump_spr, void, i32)
> > > > > > DEF_HELPER_1(store_dump_spr, void, i32)
> > > > > > diff --git a/target-ppc/op_helper.c b/target-ppc/op_helper.c
> > > > > > index cea27f2..6c375d3 100644
> > > > > > --- a/target-ppc/op_helper.c
> > > > > > +++ b/target-ppc/op_helper.c
> > > > > > @@ -1844,6 +1844,11 @@ uint32_t helper_load_dcr (uint32_t dcrn)
> > > > > >   return val;
> > > > > > }
> > > > > >
> > > > > > +target_ulong helper_load_dcr_tl (target_ulong dcrn)
> > > > > > +{
> > > > > > +    return (uint32_t)helper_load_dcr((uint32_t)dcrn);
> > > > > > +}
> > > > > > +
> > > > > > void helper_store_dcr (uint32_t dcrn, uint32_t val)
> > > > > > {
> > > > > >   if (unlikely(env->dcr_env == NULL)) {
> > > > > > @@ -1857,6 +1862,11 @@ void helper_store_dcr (uint32_t dcrn,  
> > > > > > uint32_t val)
> > > > > >   }
> > > > > > }
> > > > > >
> > > > > > +void helper_store_dcr_tl (target_ulong dcrn, target_ulong val)
> > > > > > +{
> > > > > > +    helper_store_dcr((uint32_t)dcrn, (uint32_t)val);
> > > > > > +}
> > > > > > +
> > > > >
> > > > > I do wonder why we need to keep the old helper_load_dcr() and
> > > > > helper_store_dcr() instead of modifying them directly. They  
> > > > > doesn't seems
> > > > > to be used elsewhere.
> > > >
> > > > Last time I checked they were used in hw/*ppc*.c. Maybe mangled  
> > > > through funny preprocessor or function callback logic. But maybe  
> > > > I'm wrong :).
> > >
> > > I am not able to find them. I have tried to remove helper_load_dcr 
> > > () and
> > > helper_store_dcr() from target-ppc/cpu.h and the code still  
> > > compiles.
> >
> > Hm, right. The only references are to ppc_dcr_read and  
> > ppc_dcr_write. I guess the other helpers are superfluous then.
>
> I have applied a patch to fix the problem.

Thanks a lot! Sorry I didn't get to it :(.

Alex