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

[Edgar E. Iglesias]

On Wed, Apr 01, 2009 at 12:58:06AM +0400, Vladimir Prus wrote:
> On Wednesday 14 January 2009 23:45:11 Edgar E. Iglesias wrote:
> > On Thu, Dec 11, 2008 at 09:34:30PM +0300, Vladimir Prus wrote:
> > > 
> > > This patch fixes the emulation of movca.l and ocbi
> > > instructions. movca.l is documented to allocate a cache
> > > like, and write into it. ocbi invalidates a cache line.
> > > So, given code like:
> > > 
> > >                   asm volatile("movca.l r0, @%0\n\t"
> > >                        "movca.l r0, @%1\n\t"
> > >                        "ocbi @%0\n\t"
> > >                        "ocbi @%1" : :
> > >                        "r" (a0), "r" (a1));
> > > 
> > > Nothing is actually written to memory. Code like this can be
> > > found in arch/sh/mm/cache-sh4.c and is used to flush the cache.
> > > 
> > > Current QEMU implements movca.l the same way as ordinary move,
> > > and the code above just corrupts memory. Doing full cache emulation
> > > is out of question, so this patch implements a hack. Stores
> > > done by movca.l are delayed. If we execute an instructions that is
> > > neither movca.l nor ocbi, we flush all pending stores. If we execute
> > > ocbi, we look at pending stores and delete a store to the invalidated
> > > address. This appears to work fairly well in practice.
> > > 
> > > - Volodya
> > 
> > Hello and thanks for the patch.
> > 
> > If you wonder why the late sudden interest in this, see the following
> > thread :)
> > http://lists.gnu.org/archive/html/qemu-devel/2009-01/msg00460.html
> 
> Hi Edgar,
> 
> apologies for belated reply.

Hi, Don't worry about it..


> 
> > I've got a few comments on your patch. Lets start with the general ones.
> > 
> > It would be good if the patch handled when the data cache gets disabled or
> > put into writethrough mode. There are also memory areas that are not
> > cache:able (TLB feature). Supporting the latter can get messy for very
> > little win, IMO not needed.
> 
> 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.

[cut]
> I am attaching a revised patch -- what do you think?

Thanks it looks good, at least I think it will handle more of the
realistic use cases.

A few inlined minor comments on the cosmetics though:


> 
> - Volodya
> 

> commit 1902e15049a9179ced0c924c5ce7c43f69e74001
> Author: Vladimir Prus <address@hidden>
> Date:   Tue Nov 11 12:29:02 2008 +0300
> 
>     Fix movcal.l/ocbi emulation.
>     
>       * target-sh4/cpu.h (memory_content_t): New.
>       (CPUSH4State): New fields movcal_backup and movcal_backup_tail.
>       * target-sh4/helper.h (helper_movcal)
>       (helper_discard_movcal_backup, helper_ocbi): New.
>       * target-sh4/op_helper.c (helper_movcal)
>       (helper_discard_movcal_backup, helper_ocbi): New.
>       * target-sh4/translate.c (DisasContext): New field has_movcal.
>       (sh4_defs): Update CVS for SH7785.
>       (cpu_sh4_init): Initialize env->movcal_backup_tail.
>       (_decode_opc): Discard movca.l-backup.
>       Make use of helper_movcal and helper_ocbi.
>       (gen_intermediate_code_internal): Initialize has_movcal to 1.
> 
> 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.


>      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).


>  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.



>  } CPUSH4State;
>  
>  CPUSH4State *cpu_sh4_init(const char *cpu_model);
> @@ -294,16 +302,19 @@ static inline void cpu_pc_from_tb(CPUState *env, 
> TranslationBlock *tb)
>      env->flags = tb->flags;
>  }
>  
> +#define TB_FLAG_PENDING_MOVCA  (1 << 4)
> +
>  static inline void cpu_get_tb_cpu_state(CPUState *env, target_ulong *pc,
>                                          target_ulong *cs_base, int *flags)
>  {
>      *pc = env->pc;
>      *cs_base = 0;
>      *flags = (env->flags & (DELAY_SLOT | DELAY_SLOT_CONDITIONAL
> -                    | DELAY_SLOT_TRUE | DELAY_SLOT_CLEARME))   /* Bits  0- 3 
> */
> -            | (env->fpscr & (FPSCR_FR | FPSCR_SZ | FPSCR_PR))  /* Bits 19-21 
> */
> +                    | DELAY_SLOT_TRUE | DELAY_SLOT_CLEARME))     /* Bits  0- 
> 3 */
> +            | (env->fpscr & (FPSCR_FR | FPSCR_SZ | FPSCR_PR))    /* Bits 
> 19-21 */
>              | (env->sr & (SR_MD | SR_RB))                      /* Bits 29-30 
> */
> -            | (env->sr & SR_FD);                               /* Bit 15 */
> +            | (env->sr & SR_FD)                                /* Bit 15 */
> +            | (env->movcal_backup ? TB_FLAG_PENDING_MOVCA : 0); /* Bit 4 */
>  }
>  
>  #endif                               /* _CPU_SH4_H */
> diff --git a/target-sh4/helper.h b/target-sh4/helper.h
> index e665185..4b2fcdd 100644
> --- a/target-sh4/helper.h
> +++ b/target-sh4/helper.h
> @@ -9,6 +9,10 @@ DEF_HELPER_0(debug, void)
>  DEF_HELPER_1(sleep, void, i32)
>  DEF_HELPER_1(trapa, void, i32)
>  
> +DEF_HELPER_2(movcal, void, i32, i32)
> +DEF_HELPER_0(discard_movcal_backup, void)
> +DEF_HELPER_1(ocbi, void, i32)
> +
>  DEF_HELPER_2(addv, i32, i32, i32)
>  DEF_HELPER_2(addc, i32, i32, i32)
>  DEF_HELPER_2(subv, i32, i32, i32)
> diff --git a/target-sh4/op_helper.c b/target-sh4/op_helper.c
> index 84e1ad3..e6b71a0 100644
> --- a/target-sh4/op_helper.c
> +++ b/target-sh4/op_helper.c
> @@ -18,6 +18,7 @@
>   * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston MA  02110-1301 
> USA
>   */
>  #include <assert.h>
> +#include <stdlib.h>
>  #include "exec.h"
>  #include "helper.h"
>  
> @@ -122,6 +123,55 @@ void helper_trapa(uint32_t tra)
>      cpu_loop_exit();
>  }
>  
> +void helper_movcal(uint32_t address, uint32_t value)
> +{
> +    memory_content_t *r = (memory_content_t *)malloc 
> (sizeof(memory_content_t));
> +    r->address = address;
> +    r->value = value;
> +    r->next = NULL;
> +
> +    *(env->movcal_backup_tail) = r;
> +    env->movcal_backup_tail = &(r->next);
> +}
> +
> +void helper_discard_movcal_backup(void)
> +{
> +    memory_content_t *current = env->movcal_backup;
> +
> +    while(current)
> +    {
> +        memory_content_t *next = current->next;
> +        free (current);
> +        env->movcal_backup = current = next;
> +        if (current == 0)
> +            env->movcal_backup_tail = &(env->movcal_backup);
> +    } 
> +}
> +
> +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.

Cheers