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

[Blue Swirl]

On Wed, Jul 13, 2011 at 9:02 PM, Artyom Tarasenko <address@hidden> wrote:
> On Wed, Jul 13, 2011 at 6:27 PM, Blue Swirl <address@hidden> wrote:
>> 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.
>
> 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/
>