Patchwork [1/4] SPARC64: Implement ldfa/lddfa/ldqfa instructions properly

login
register
mail settings
Submitter Tsuneo Saito
Date July 13, 2011, 3:30 a.m.
Message ID <1310527849-784-2-git-send-email-tsnsaito@gmail.com>
Download mbox | patch
Permalink /patch/104472/
State New
Headers show

Comments

Tsuneo Saito - July 13, 2011, 3:30 a.m.
This patch implements sparcv9 ldfa/lddfa/ldqfa instructions
with non block-load ASIs.

Signed-off-by: Tsuneo Saito <tsnsaito@gmail.com>
---
 target-sparc/op_helper.c |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)
Blue Swirl - July 13, 2011, 4:27 p.m.
On Wed, Jul 13, 2011 at 6:30 AM, Tsuneo Saito <tsnsaito@gmail.com> wrote:
> This patch implements sparcv9 ldfa/lddfa/ldqfa instructions
> with non block-load ASIs.
>
> Signed-off-by: Tsuneo Saito <tsnsaito@gmail.com>
> ---
>  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
>
>
>
Artyom Tarasenko - July 13, 2011, 6:02 p.m.
On Wed, Jul 13, 2011 at 6:27 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Wed, Jul 13, 2011 at 6:30 AM, Tsuneo Saito <tsnsaito@gmail.com> wrote:
>> This patch implements sparcv9 ldfa/lddfa/ldqfa instructions
>> with non block-load ASIs.
>>
>> Signed-off-by: Tsuneo Saito <tsnsaito@gmail.com>
>> ---
>>  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.

You mean the not [yet] implemented block ASIs? The implemented ones
return earlier
and for the not implemented ones unassigned access fault appears to be
a right thing to do.
Do you have an example where we would have an unassigned access fault
for an implemented ASI ?

>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
>>
Blue Swirl - July 13, 2011, 6:19 p.m.
On Wed, Jul 13, 2011 at 9:02 PM, Artyom Tarasenko <atar4qemu@gmail.com> wrote:
> On Wed, Jul 13, 2011 at 6:27 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
>> On Wed, Jul 13, 2011 at 6:30 AM, Tsuneo Saito <tsnsaito@gmail.com> wrote:
>>> This patch implements sparcv9 ldfa/lddfa/ldqfa instructions
>>> with non block-load ASIs.
>>>
>>> Signed-off-by: Tsuneo Saito <tsnsaito@gmail.com>
>>> ---
>>>  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.
>
> You mean the not [yet] implemented block ASIs? The implemented ones
> return earlier
> and for the not implemented ones unassigned access fault appears to be
> a right thing to do.
> Do you have an example where we would have an unassigned access fault
> for an implemented ASI ?

This patch would implement the unimplemented ones by calling
helper_ld_asi(), but the ASIs do not match. Instead of 0x1f (as if
user little endian secondary address space block ASI), 0x19
(corresponding non-block ASI) should be used. Also helper_ld_asi()
should not be changed to accept block ASIs, because then they would be
accepted also for non-block accesses.

>>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
>>>
>
>
>
> --
> Regards,
> Artyom Tarasenko
>
> solaris/sparc under qemu blog: http://tyom.blogspot.com/
>
Tsuneo Saito - July 13, 2011, 10:56 p.m.
Hi,

At Wed, 13 Jul 2011 21:19:16 +0300,
Blue Swirl wrote:
> On Wed, Jul 13, 2011 at 9:02 PM, Artyom Tarasenko <atar4qemu@gmail.com> wrote:
> > On Wed, Jul 13, 2011 at 6:27 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
> >> On Wed, Jul 13, 2011 at 6:30 AM, Tsuneo Saito <tsnsaito@gmail.com> wrote:
> >>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.

Thanks for the review!
I didn't know these ASIs (0x16, 0x17, 0x1e, 0x1f) as I was looking
at the JPS 1.0.4 specification.  I'll add codes for these ASIs.

---- 
Tsuneo Saito <tsnsaito@gmail.com>

Patch

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);
+        *((uint32_t *)&env->fpr[rd++]) = u.l.upper;
+        *((uint32_t *)&env->fpr[rd++]) = u.l.lower;
         break;
     }
 }