Re: [Qemu-devel] SH: Fix movca.l/ocbi emulation.

[Vladimir Prus]

On Wednesday 01 April 2009 16:44:49 Edgar E. Iglesias wrote:

Hi Edgar,


> > I am afraid I might not be the best person to accurately catch all cases
> > where cache might be enabled or disabled. KAWASAKI-san has posted a function
> > that can be used to check if caching is enabled, in:
> > 
> >     http://article.gmane.org/gmane.comp.emulators.qemu/36348
> > 
> > Does that one address your suggestion? If so, I can incorporate that 
> > function,
> > and checking for it.
> 
> I dont have the manuals here but yes, I think something like that will be
> needed. On other archs I've seen ppl do memcpy to IO areas, don't know
> about sh but to be on the safe side we better not do any funny stuff
> with uncached stores.

I have added the check, using the routine provided by Kawasaki-san.

> > diff --git a/cpu-exec.c b/cpu-exec.c
> > index cf7c1fb..b0f0b5e 100644
> > --- a/cpu-exec.c
> > +++ b/cpu-exec.c
> > @@ -174,7 +174,7 @@ static inline TranslationBlock *tb_find_fast(void)
> >      /* we record a subset of the CPU state. It will
> >         always be the same before a given translated block
> >         is executed. */
> > -    cpu_get_tb_cpu_state(env, &pc, &cs_base, &flags);
> > +    cpu_get_tb_cpu_state(env, &pc, &cs_base, &flags);            
>                                                           ^^^
> Stray spaces.

Doh -- I had unsaved emacs buffer with this issue fixed.

> >      tb = env->tb_jmp_cache[tb_jmp_cache_hash_func(pc)];
> >      if (unlikely(!tb || tb->pc != pc || tb->cs_base != cs_base ||
> >                   tb->flags != flags)) {
> > diff --git a/target-sh4/cpu.h b/target-sh4/cpu.h
> > index e9eee2c..67741f6 100644
> > --- a/target-sh4/cpu.h
> > +++ b/target-sh4/cpu.h
> > @@ -100,6 +100,12 @@ enum sh_features {
> >      SH_FEATURE_BCR3_AND_BCR4 = 2,
> >  };
> >  
> > +typedef struct memory_content_t {
> > +    uint32_t address;
> > +    uint32_t value;
> > +    struct memory_content_t *next;
> > +} memory_content_t;
> > +
> 
> Please drop the _t (reserved names).

Fixed.

> >  typedef struct CPUSH4State {
> >      int id;                        /* CPU model */
> >  
> > @@ -149,6 +155,8 @@ typedef struct CPUSH4State {
> >      tlb_t itlb[ITLB_SIZE]; /* instruction translation table */
> >      void *intc_handle;
> >      int intr_at_halt;              /* SR_BL ignored during sleep */
> > +    memory_content_t *movcal_backup;
> > +    memory_content_t **movcal_backup_tail;
> 
> Do you really need the _tail member?
> Seems to me like you're only maintaining it but not really using it.

See helper_movcal:

        *(env->movcal_backup_tail) = r;

I think having movcal backup list in-order is good. Say, if we have two
movcal to the same address and ocbi, we want to restore the saved when
the first movcal was executed, and given that helper_ocbi traversed
the list from the front we need to put elements in-order.


> > +void helper_ocbi(uint32_t address)
> > +{
> > +    memory_content_t **current = &(env->movcal_backup);
> > +    while (*current)
> > +    {
> > +        uint32_t a = (*current)->address;
> > +        if ((a & ~0x1F) == (address & ~0x1F))
> > +   {
> > +            memory_content_t *next = (*current)->next;
> > +
> > +            stl_data(a, (*current)->value);
> > +
> > +            if (next == 0)
> > +       {
> > +                env->movcal_backup_tail = current;
> > +       }
> > +
> > +            free (*current);
> > +            *current = next;
> > +            break;
> > +   }
> 
> The indentation got messed up here.

I have fixed it. Revised patch is attached.

Thanks,
Volodya

movcal-2.diff
Description: Text Data