Re: [Qemu-devel] [PATCH 1/4] SPARC64: Implement ldfa/lddfa/ldqfa instructions properly

[Blue Swirl]

On Wed, Jul 13, 2011 at 6:30 AM, Tsuneo Saito <address@hidden> wrote:
> This patch implements sparcv9 ldfa/lddfa/ldqfa instructions
> with non block-load ASIs.
>
> Signed-off-by: Tsuneo Saito <address@hidden>
> ---
>  target-sparc/op_helper.c |   16 +++++++++++-----
>  1 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/target-sparc/op_helper.c b/target-sparc/op_helper.c
> index fd0cfbd..a75ac4f 100644
> --- a/target-sparc/op_helper.c
> +++ b/target-sparc/op_helper.c
> @@ -3331,7 +3331,7 @@ void helper_ldda_asi(target_ulong addr, int asi, int rd)
>  void helper_ldf_asi(target_ulong addr, int asi, int size, int rd)
>  {
>     unsigned int i;
> -    target_ulong val;
> +    CPU_DoubleU u;
>
>     helper_check_align(addr, 3);
>     addr = asi_address_mask(env, asi, addr);
> @@ -3371,17 +3371,23 @@ void helper_ldf_asi(target_ulong addr, int asi, int 
> size, int rd)
>         break;
>     }
>
> -    val = helper_ld_asi(addr, asi, size, 0);
>     switch(size) {
>     default:
>     case 4:
> -        *((uint32_t *)&env->fpr[rd]) = val;
> +        *((uint32_t *)&env->fpr[rd]) = helper_ld_asi(addr, asi, size, 0);
>         break;
>     case 8:
> -        *((int64_t *)&DT0) = val;
> +        u.ll = helper_ld_asi(addr, asi, size, 0);
> +        *((uint32_t *)&env->fpr[rd++]) = u.l.upper;
> +        *((uint32_t *)&env->fpr[rd++]) = u.l.lower;
>         break;
>     case 16:
> -        // XXX
> +        u.ll = helper_ld_asi(addr, asi, 8, 0);
> +        *((uint32_t *)&env->fpr[rd++]) = u.l.upper;
> +        *((uint32_t *)&env->fpr[rd++]) = u.l.lower;
> +        u.ll = helper_ld_asi(addr + 8, asi, 8, 0);

In general the patch is an improvement, however the ASIs are passed as
is to helper_ld_asi() and there the block ASIs would trigger
unassigned access faults. So you should perform some arithmetic with
the ASI numbers to make them match non-block ASIs, for example asi &
~0x6. This only works in the specific block ASIs, so I'd move this
inside the switch block for cases 0x16, 0x17, 0x1e, 0x1f etc. By the
way, aren't those the ASIs in question? The manual (UltraSPARC
Architecture 2007) is a bit confusing.

> +        *((uint32_t *)&env->fpr[rd++]) = u.l.upper;
> +        *((uint32_t *)&env->fpr[rd++]) = u.l.lower;
>         break;
>     }
>  }
> --
> 1.7.5.4
>
>
>