diff mbox

[2/8] sparc64: fix missing address masking

Message ID 20100601201227.5908.12931.stgit@skyserv
State New
Headers show

Commit Message

Igor V. Kovalenko June 1, 2010, 8:12 p.m. UTC 
From: Igor V. Kovalenko <igor.v.kovalenko@gmail.com>

- address masking for ldqf and stqf insns
- address masking for lddf and stdf insns
- address masking for translating ASI (Ultrasparc IIi)

Signed-off-by: Igor V. Kovalenko <igor.v.kovalenko@gmail.com>
---
 target-sparc/op_helper.c |   47 ++++++++++++++++++++++++++++++++++++++++++++++
 target-sparc/translate.c |    4 ++++
 2 files changed, 51 insertions(+), 0 deletions(-)

Comments

Richard Henderson June 1, 2010, 8:44 p.m. UTC | #1 
On 06/01/2010 01:12 PM, Igor V. Kovalenko wrote:
> +    if ((env->pstate & PS_AM) && is_translating_asi(asi)) {
> +        addr &= 0xffffffffULL;
> +    }

I suggest that these be written instead as

  if (is_translating_asi(asi)) {
    addr = address_mask(addr);
  }

That should allow you to remove some of the ifdefs.


r~
Igor V. Kovalenko June 2, 2010, 4:29 a.m. UTC | #2 
On Wed, Jun 2, 2010 at 12:44 AM, Richard Henderson <rth@twiddle.net> wrote:
> On 06/01/2010 01:12 PM, Igor V. Kovalenko wrote:
>> +    if ((env->pstate & PS_AM) && is_translating_asi(asi)) {
>> +        addr &= 0xffffffffULL;
>> +    }
>
> I suggest that these be written instead as
>
>  if (is_translating_asi(asi)) {
>    addr = address_mask(addr);
>  }
>
> That should allow you to remove some of the ifdefs.

All address masking is done for sparc64 target only, sparc32 does not
have the notion of translating asi.
I think it's better to do debug printf macro trick then but I see no
real benefit at the moment.
Richard Henderson June 2, 2010, 1:47 p.m. UTC | #3 
On 06/01/2010 09:29 PM, Igor Kovalenko wrote:
> On Wed, Jun 2, 2010 at 12:44 AM, Richard Henderson <rth@twiddle.net> wrote:
>> On 06/01/2010 01:12 PM, Igor V. Kovalenko wrote:
>>> +    if ((env->pstate & PS_AM) && is_translating_asi(asi)) {
>>> +        addr &= 0xffffffffULL;
>>> +    }
>>
>> I suggest that these be written instead as
>>
>>  if (is_translating_asi(asi)) {
>>    addr = address_mask(addr);
>>  }
>>
>> That should allow you to remove some of the ifdefs.
> 
> All address masking is done for sparc64 target only, sparc32 does not
> have the notion of translating asi.

Of course I know that.

> I think it's better to do debug printf macro trick ...

... with no evidence.  The compiler is happy to optimize away
the entire if statement without having to resort to macros.

> ... then but I see no real benefit at the moment.

Avoiding ifdefs isn't a benefit?


r~
Blue Swirl June 2, 2010, 4:10 p.m. UTC | #4 
On Wed, Jun 2, 2010 at 1:47 PM, Richard Henderson <rth@twiddle.net> wrote:
> On 06/01/2010 09:29 PM, Igor Kovalenko wrote:
>> On Wed, Jun 2, 2010 at 12:44 AM, Richard Henderson <rth@twiddle.net> wrote:
>>> On 06/01/2010 01:12 PM, Igor V. Kovalenko wrote:
>>>> +    if ((env->pstate & PS_AM) && is_translating_asi(asi)) {
>>>> +        addr &= 0xffffffffULL;
>>>> +    }
>>>
>>> I suggest that these be written instead as
>>>
>>>  if (is_translating_asi(asi)) {
>>>    addr = address_mask(addr);
>>>  }
>>>
>>> That should allow you to remove some of the ifdefs.
>>
>> All address masking is done for sparc64 target only, sparc32 does not
>> have the notion of translating asi.
>
> Of course I know that.
>
>> I think it's better to do debug printf macro trick ...
>
> ... with no evidence.  The compiler is happy to optimize away
> the entire if statement without having to resort to macros.
>
>> ... then but I see no real benefit at the moment.
>
> Avoiding ifdefs isn't a benefit?

I agree macros would make the code more tidy, perhaps it could swallow
both the check and the masking. The macro can be empty for Sparc32.
Andreas Färber June 2, 2010, 4:46 p.m. UTC | #5 
Am 02.06.2010 um 18:10 schrieb Blue Swirl:

> On Wed, Jun 2, 2010 at 1:47 PM, Richard Henderson <rth@twiddle.net>  
> wrote:
>> On 06/01/2010 09:29 PM, Igor Kovalenko wrote:
>>> On Wed, Jun 2, 2010 at 12:44 AM, Richard Henderson  
>>> <rth@twiddle.net> wrote:
>>>> On 06/01/2010 01:12 PM, Igor V. Kovalenko wrote:
>>>>> +    if ((env->pstate & PS_AM) && is_translating_asi(asi)) {
>>>>> +        addr &= 0xffffffffULL;
>>>>> +    }
>>>>
>>>> I suggest that these be written instead as
>>>>
>>>>  if (is_translating_asi(asi)) {
>>>>    addr = address_mask(addr);
>>>>  }
>>>>
>>>> That should allow you to remove some of the ifdefs.
>>
>>> I think it's better to do debug printf macro trick ...
>>
>> ... with no evidence.  The compiler is happy to optimize away
>> the entire if statement without having to resort to macros.
>>
>>> ... then but I see no real benefit at the moment.
>>
>> Avoiding ifdefs isn't a benefit?
>
> I agree macros would make the code more tidy, perhaps it could swallow
> both the check and the masking. The macro can be empty for Sparc32.

I usually prefer static inline functions over multi-line macros.  
Probably a matter of taste.
Igor V. Kovalenko June 2, 2010, 6:21 p.m. UTC | #6 
On Wed, Jun 2, 2010 at 8:46 PM, Andreas Färber <andreas.faerber@web.de> wrote:
> Am 02.06.2010 um 18:10 schrieb Blue Swirl:
>
>> On Wed, Jun 2, 2010 at 1:47 PM, Richard Henderson <rth@twiddle.net> wrote:
>>>
>>> On 06/01/2010 09:29 PM, Igor Kovalenko wrote:
>>>>
>>>> On Wed, Jun 2, 2010 at 12:44 AM, Richard Henderson <rth@twiddle.net>
>>>> wrote:
>>>>>
>>>>> On 06/01/2010 01:12 PM, Igor V. Kovalenko wrote:
>>>>>>
>>>>>> +    if ((env->pstate & PS_AM) && is_translating_asi(asi)) {
>>>>>> +        addr &= 0xffffffffULL;
>>>>>> +    }
>>>>>
>>>>> I suggest that these be written instead as
>>>>>
>>>>>  if (is_translating_asi(asi)) {
>>>>>   addr = address_mask(addr);
>>>>>  }
>>>>>
>>>>> That should allow you to remove some of the ifdefs.
>>>
>>>> I think it's better to do debug printf macro trick ...
>>>
>>> ... with no evidence.  The compiler is happy to optimize away
>>> the entire if statement without having to resort to macros.
>>>
>>>> ... then but I see no real benefit at the moment.
>>>
>>> Avoiding ifdefs isn't a benefit?
>>
>> I agree macros would make the code more tidy, perhaps it could swallow
>> both the check and the masking. The macro can be empty for Sparc32.
>
> I usually prefer static inline functions over multi-line macros. Probably a
> matter of taste.

I'll resend this one updated to have less ifdefs.
Igor V. Kovalenko June 2, 2010, 7:20 p.m. UTC | #7 
On Wed, Jun 2, 2010 at 5:47 PM, Richard Henderson <rth@twiddle.net> wrote:
> On 06/01/2010 09:29 PM, Igor Kovalenko wrote:
>> On Wed, Jun 2, 2010 at 12:44 AM, Richard Henderson <rth@twiddle.net> wrote:
>>> On 06/01/2010 01:12 PM, Igor V. Kovalenko wrote:
>>>> +    if ((env->pstate & PS_AM) && is_translating_asi(asi)) {
>>>> +        addr &= 0xffffffffULL;
>>>> +    }
>>>
>>> I suggest that these be written instead as
>>>
>>>  if (is_translating_asi(asi)) {
>>>    addr = address_mask(addr);
>>>  }
>>>
>>> That should allow you to remove some of the ifdefs.
>>
>> All address masking is done for sparc64 target only, sparc32 does not
>> have the notion of translating asi.
>
> Of course I know that.
>
>> I think it's better to do debug printf macro trick ...
>
> ... with no evidence.  The compiler is happy to optimize away
> the entire if statement without having to resort to macros.
>
>> ... then but I see no real benefit at the moment.
>
> Avoiding ifdefs isn't a benefit?

Reading through the code you will have false evidence of possible
extra steps taken by hardware we emulate.
Looking at sparcv8 docs it is clear that similar steps are done there
as well so I'll drop ifdefs at call sites and redo the patch.
diff mbox

Patch

diff --git a/target-sparc/op_helper.c b/target-sparc/op_helper.c
index ef3504f..f5e153d 100644
--- a/target-sparc/op_helper.c
+++ b/target-sparc/op_helper.c
@@ -2315,6 +2315,25 @@  void helper_st_asi(target_ulong addr, target_ulong val, int asi, int size)
 
 #else /* CONFIG_USER_ONLY */
 
+/* Ultrasparc IIi translating asi
+   - note this list is defined by cpu implementation
+ */
+static inline int is_translating_asi(int asi)
+{
+    switch (asi) {
+    case 0x04 ... 0x11:
+    case 0x18 ... 0x19:
+    case 0x24 ... 0x2C:
+    case 0x70 ... 0x73:
+    case 0x78 ... 0x79:
+    case 0x80 ... 0xFF:
+        return 1;
+
+    default:
+        return 0;
+    }
+}
+
 uint64_t helper_ld_asi(target_ulong addr, int asi, int size, int sign)
 {
     uint64_t ret = 0;
@@ -2330,7 +2349,12 @@  uint64_t helper_ld_asi(target_ulong addr, int asi, int size, int sign)
             && !(env->hpstate & HS_PRIV)))
         raise_exception(TT_PRIV_ACT);
 
+    if ((env->pstate & PS_AM) && is_translating_asi(asi)) {
+        addr &= 0xffffffffULL;
+    }
+
     helper_check_align(addr, size - 1);
+
     switch (asi) {
     case 0x82: // Primary no-fault
     case 0x8a: // Primary no-fault LE
@@ -2681,7 +2705,12 @@  void helper_st_asi(target_ulong addr, target_ulong val, int asi, int size)
             && !(env->hpstate & HS_PRIV)))
         raise_exception(TT_PRIV_ACT);
 
+    if ((env->pstate & PS_AM) && is_translating_asi(asi)) {
+        addr &= 0xffffffffULL;
+    }
+
     helper_check_align(addr, size - 1);
+
     /* Convert to little endian */
     switch (asi) {
     case 0x0c: // Nucleus Little Endian (LE)
@@ -3056,6 +3085,12 @@  void helper_ldda_asi(target_ulong addr, int asi, int rd)
             && !(env->hpstate & HS_PRIV)))
         raise_exception(TT_PRIV_ACT);
 
+#if defined (CONFIG_SPARC64)
+    if ((env->pstate & PS_AM) && is_translating_asi(asi)) {
+        addr &= 0xffffffffULL;
+    }
+#endif
+
     switch (asi) {
 #if !defined(CONFIG_USER_ONLY)
     case 0x24: // Nucleus quad LDD 128 bit atomic
@@ -3102,6 +3137,12 @@  void helper_ldf_asi(target_ulong addr, int asi, int size, int rd)
     unsigned int i;
     target_ulong val;
 
+#if defined (CONFIG_SPARC64)
+    if ((env->pstate & PS_AM) && is_translating_asi(asi)) {
+        addr &= 0xffffffffULL;
+    }
+#endif
+
     helper_check_align(addr, 3);
     switch (asi) {
     case 0xf0: // Block load primary
@@ -3144,6 +3185,12 @@  void helper_stf_asi(target_ulong addr, int asi, int size, int rd)
     unsigned int i;
     target_ulong val = 0;
 
+#if defined (CONFIG_SPARC64)
+    if ((env->pstate & PS_AM) && is_translating_asi(asi)) {
+        addr &= 0xffffffffULL;
+    }
+#endif
+
     helper_check_align(addr, 3);
     switch (asi) {
     case 0xe0: // UA2007 Block commit store primary (cache flush)
diff --git a/target-sparc/translate.c b/target-sparc/translate.c
index 72ca0b4..eff64d4 100644
--- a/target-sparc/translate.c
+++ b/target-sparc/translate.c
@@ -4490,6 +4490,7 @@  static void disas_sparc_insn(DisasContext * dc)
 
                         CHECK_FPU_FEATURE(dc, FLOAT128);
                         r_const = tcg_const_i32(dc->mem_idx);
+                        gen_address_mask(dc, cpu_addr);
                         gen_helper_ldqf(cpu_addr, r_const);
                         tcg_temp_free_i32(r_const);
                         gen_op_store_QT0_fpr(QFPREG(rd));
@@ -4500,6 +4501,7 @@  static void disas_sparc_insn(DisasContext * dc)
                         TCGv_i32 r_const;
 
                         r_const = tcg_const_i32(dc->mem_idx);
+                        gen_address_mask(dc, cpu_addr);
                         gen_helper_lddf(cpu_addr, r_const);
                         tcg_temp_free_i32(r_const);
                         gen_op_store_DT0_fpr(DFPREG(rd));
@@ -4635,6 +4637,7 @@  static void disas_sparc_insn(DisasContext * dc)
                         CHECK_FPU_FEATURE(dc, FLOAT128);
                         gen_op_load_fpr_QT0(QFPREG(rd));
                         r_const = tcg_const_i32(dc->mem_idx);
+                        gen_address_mask(dc, cpu_addr);
                         gen_helper_stqf(cpu_addr, r_const);
                         tcg_temp_free_i32(r_const);
                     }
@@ -4657,6 +4660,7 @@  static void disas_sparc_insn(DisasContext * dc)
 
                         gen_op_load_fpr_DT0(DFPREG(rd));
                         r_const = tcg_const_i32(dc->mem_idx);
+                        gen_address_mask(dc, cpu_addr);
                         gen_helper_stdf(cpu_addr, r_const);
                         tcg_temp_free_i32(r_const);
                     }