Re: [Qemu-devel] SH: Implement MOVCO.L and MOVLI.L

[Aurelien Jarno]

On Thu, Feb 12, 2009 at 12:05:15AM +0300, Vladimir Prus wrote:
> On Saturday 07 February 2009 17:24:59 Aurelien Jarno wrote:
> > On Thu, Dec 11, 2008 at 09:48:00PM +0300, Vladimir Prus wrote:
> > > On Thursday 11 December 2008 21:36:13 Paul Brook wrote:
> > > > On Thursday 11 December 2008, Vladimir Prus wrote:
> > > > > This patch implements a couple more instructions
> > > > > specific to SH4A. 
> > > > 
> > > > > +          For now, do unconditional move. FIXME.
> > > > 
> > > > I can't make sense of this FIXME.
> > > 
> > > At it happens, it's stale. The entire line you quoted can be removed.
> > > I attach the revised patch.
> > > 
> > > - Volodya
> > > 
> > > 
> > 
> > > commit 9ba7a6294a809dc11a0348ccca501513dd90ae6e
> > > Author: Vladimir Prus <address@hidden>
> > > Date:   Sun Oct 12 19:36:22 2008 +0400
> > > 
> > >     SH: Implement MOVCO.L and MOVLI.L
> > >     
> > >           * target-sh4/cpu.h (struct CPUSH4State): New field ldst.
> > >           * target-sh4/helper.c (do_interrupt): Initialize ldst.
> > >           * target-sh4/translate.c (cpu_ldst): New.
> > >           (sh4_translate_init): Initialize cpu_ldst.
> > >           (_decode_opc): Support MOVCO.L and MOVLI.L.
> > > 
> > > diff --git a/target-sh4/cpu.h b/target-sh4/cpu.h
> > > index a0108b4..ae434d1 100644
> > > --- a/target-sh4/cpu.h
> > > +++ b/target-sh4/cpu.h
> > > @@ -135,6 +135,8 @@ typedef struct CPUSH4State {
> > >      uint32_t prr;                /* Processor Revision Register */
> > >      uint32_t cvr;                /* Cache Version Register */
> > >  
> > > +    uint32_t ldst;
> > > +
> > >       CPU_COMMON tlb_t utlb[UTLB_SIZE];   /* unified translation table */
> > >      tlb_t itlb[ITLB_SIZE];       /* instruction translation table */
> > >      void *intc_handle;
> > > diff --git a/target-sh4/helper.c b/target-sh4/helper.c
> > > index 882bc9c..61b668b 100644
> > > --- a/target-sh4/helper.c
> > > +++ b/target-sh4/helper.c
> > > @@ -160,6 +160,7 @@ void do_interrupt(CPUState * env)
> > >      env->spc = env->pc;
> > >      env->sgr = env->gregs[15];
> > >      env->sr |= SR_BL | SR_MD | SR_RB;
> > > +    env->ldst = 0;
> > >  
> > >      if (env->flags & (DELAY_SLOT | DELAY_SLOT_CONDITIONAL)) {
> > >          /* Branch instruction should be executed again before delay 
> > > slot. */
> > > diff --git a/target-sh4/translate.c b/target-sh4/translate.c
> > > index d38e95f..e4a1b28 100644
> > > --- a/target-sh4/translate.c
> > > +++ b/target-sh4/translate.c
> > > @@ -72,7 +72,7 @@ static TCGv_ptr cpu_env;
> > >  static TCGv cpu_gregs[24];
> > >  static TCGv cpu_pc, cpu_sr, cpu_ssr, cpu_spc, cpu_gbr;
> > >  static TCGv cpu_vbr, cpu_sgr, cpu_dbr, cpu_mach, cpu_macl;
> > > -static TCGv cpu_pr, cpu_fpscr, cpu_fpul;
> > > +static TCGv cpu_pr, cpu_fpscr, cpu_fpul, cpu_ldst;
> > >  static TCGv cpu_fregs[32];
> > >  
> > >  /* internal register indexes */
> > > @@ -144,6 +144,8 @@ static void sh4_translate_init(void)
> > >      cpu_delayed_pc = tcg_global_mem_new_i32(TCG_AREG0,
> > >                                       offsetof(CPUState, delayed_pc),
> > >                                       "_delayed_pc_");
> > > +    cpu_ldst = tcg_global_mem_new_i32(TCG_AREG0,
> > > +                               offsetof(CPUState, ldst), "_ldst_");
> > >  
> > >      for (i = 0; i < 32; i++)
> > >          cpu_fregs[i] = tcg_global_mem_new_i32(TCG_AREG0,
> > > @@ -1543,6 +1545,36 @@ static void _decode_opc(DisasContext * ctx)
> > >      case 0x0029:         /* movt Rn */
> > >   tcg_gen_andi_i32(REG(B11_8), cpu_sr, SR_T);
> > >   return;
> > > +    case 0x0073:
> > > +        /* MOVCO.L 
> > > +        LDST -> T
> > > +               If (T == 1) R0 -> (Rn)
> > > +               0 -> LDST
> > > +        */          
> > > +        if (ctx->features & SH_FEATURE_SH4A) {
> > > +     int label = gen_new_label();
> > > +     gen_clr_t();
> > > +     tcg_gen_or_i32(cpu_sr, cpu_sr, cpu_ldst);
> > > +     tcg_gen_brcondi_i32(TCG_COND_EQ, cpu_ldst, 0, label);
> > > +     tcg_gen_qemu_st32(REG(0), REG(B11_8), ctx->memidx);
> > > +     gen_set_label(label);
> > > +     tcg_gen_movi_i32(cpu_ldst, 0);
> > > +     return;
> > > + } else
> > > +     break;
> > > +    case 0x0063:
> > > +        /* MOVLI.L @Rm,R0 
> > > +               1 -> LDST
> > > +               (Rm) -> R0
> > > +               When interrupt/exception
> > > +               occurred 0 -> LDST
> > > +        */     
> > > + if (ctx->features & SH_FEATURE_SH4A) {
> > > +     tcg_gen_movi_i32(cpu_ldst, 1);
> > > +     tcg_gen_qemu_ld32s(REG(0), REG(B11_8), ctx->memidx);
> > > +     return;
> > 
> > This TCG code doesn't seems to match the comments. With the current
> > code, if a load exception occurs, LDST end with a value of 1 instead of
> > 0.
> 
> I might be missing something -- it was my intention that if any exception
> or interrupt happens, do_interrupt will set ldst to zero. And if the exception
> happens during load, there's nothing else that will set ldst back to 1. Did
> I miss something?
> 

In system mode, I am not sure that doing that in do_interrupt() is the
best way, especially given it looks like we can exit this subroutine
before actually setting the value to 0, though I haven't really analyzed
those cases. In user mode, it's surely wrong, given that do_interrupt()
does nothing.

In my opinion you should use the fact that the TCG code will stop it's
execution at the point were the exception occurs. Doing something like
this should work:

tcg_gen_movi_i32(cpu_ldst, 0);
tcg_gen_qemu_ld32s(REG(0), REG(B11_8), ctx->memidx);
tcg_gen_movi_i32(cpu_ldst, 1);

This way if everything works fine, cpu_ldst ends with 1. If an exception
occurs, it stays to zero.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
address@hidden                 http://www.aurel32.net