Re: [Qemu-devel] [PATCH v4 6/8] target/mips: Amend CP0 WatchHi register implementation

[Aleksandar Markovic]

> > +++ b/target/mips/op_helper.c
> > @@ -893,7 +893,12 @@ target_ulong helper_mfc0_watchlo(CPUMIPSState *env, > 
> > uint32_t sel)
> >
> >  target_ulong helper_mfc0_watchhi(CPUMIPSState *env, uint32_t sel)
> >  {
> > -    return env->CP0_WatchHi[sel];
> > +    return (int32_t) env->CP0_WatchHi[sel];
> > +}
> > +
> > +target_ulong helper_mfhc0_watchhi(CPUMIPSState *env, uint32_t sel)
> > +{
> > +    return env->CP0_WatchHi[sel] >> 32;
> >  }
>
> Did you in fact want the high-part sign-extended as well?
> It might be more obvious to write
>
>     return sextract64(env->CP0_WatchHi[sel], 32, 32);
>
> in that case.
>
> > +void helper_mthc0_watchhi(CPUMIPSState *env, target_ulong arg1, uint32_t 
> > sel)
> > +{
> > +    env->CP0_WatchHi[sel] = ((uint64_t) (arg1) << 32) |
> > +                            (env->CP0_WatchHi[sel] & 
> > 0x00000000ffffffffULL);
>
> Cleaner as
>
>     env->CP0_WatchHi[sel] = deposit64(env->CP0_WatchHi[sel], 32, 32, arg1);
>
>
> For future cleanup, there is nothing in this (or several other) that requires
> writing helper code.  This could just as easily be expanded inline with one
> single load or store operation.
>
>
> r~>

If this all is the case, I am going to remove this patch from this series. 
There is no hurry. This patch just needs to be thought over a little bit more. 
It will be sent in some future series.

Regards,
Aleksandar