target/hppa: Allow, but diagnose, LDCW aligned only mod 4
diff mbox series

Message ID 20200117015322.12953-1-richard.henderson@linaro.org
State New
Headers show
Series
  • target/hppa: Allow, but diagnose, LDCW aligned only mod 4
Related show

Commit Message

Richard Henderson Jan. 17, 2020, 1:53 a.m. UTC
The PA-RISC 1.1 specification says that LDCW must be aligned mod 16
or the operation is undefined.  However, real hardware only generates
an unaligned access trap for unaligned mod 4.

Match real hardware, but diagnose with GUEST_ERROR a violation of the
specification.

Reported-by: Helge Deller <deller@gmx.de>
Suggested-by: John David Anglin <dave.anglin@bell.net>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---

Helge, can you please test this against your failing kernel?
You will of course want to add -D logfile -d guest_errors to
you qemu command-line.


r~

---
 target/hppa/helper.h    | 2 ++
 target/hppa/op_helper.c | 9 +++++++++
 target/hppa/translate.c | 6 +++++-
 3 files changed, 16 insertions(+), 1 deletion(-)

Comments

Helge Deller Jan. 17, 2020, 3:49 p.m. UTC | #1
On 17.01.20 02:53, Richard Henderson wrote:
> The PA-RISC 1.1 specification says that LDCW must be aligned mod 16
> or the operation is undefined.  However, real hardware only generates
> an unaligned access trap for unaligned mod 4.
>
> Match real hardware, but diagnose with GUEST_ERROR a violation of the
> specification.
>
> Reported-by: Helge Deller <deller@gmx.de>
> Suggested-by: John David Anglin <dave.anglin@bell.net>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>
> Helge, can you please test this against your failing kernel?
> You will of course want to add -D logfile -d guest_errors to
> you qemu command-line.

Yes, works as expected.
Thanks!

Please add:
Tested-by: Helge Deller <deller@gmx.de>


[deller]$ tail -f logfile
Undefined ldc to address unaligned mod 16: 00000504fa6c7848
Undefined ldc to address unaligned mod 16: 00000504fa6c7a48
Undefined ldc to address unaligned mod 16: 00000506f9434848
Undefined ldc to address unaligned mod 16: 00000506f9434a48
Undefined ldc to address unaligned mod 16: 00000508fa036848
Undefined ldc to address unaligned mod 16: 00000508fa036a48
Undefined ldc to address unaligned mod 16: 0000050afa8c4848
Undefined ldc to address unaligned mod 16: 0000050afa8c4a48
Undefined ldc to address unaligned mod 16: 0000050cf94d1848
Undefined ldc to address unaligned mod 16: 0000050cf94d1a48
....



>
>
> r~
>
> ---
>  target/hppa/helper.h    | 2 ++
>  target/hppa/op_helper.c | 9 +++++++++
>  target/hppa/translate.c | 6 +++++-
>  3 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/target/hppa/helper.h b/target/hppa/helper.h
> index 38d834ef6b..2d483aab58 100644
> --- a/target/hppa/helper.h
> +++ b/target/hppa/helper.h
> @@ -17,6 +17,8 @@ DEF_HELPER_FLAGS_3(stby_b_parallel, TCG_CALL_NO_WG, void, env, tl, tr)
>  DEF_HELPER_FLAGS_3(stby_e, TCG_CALL_NO_WG, void, env, tl, tr)
>  DEF_HELPER_FLAGS_3(stby_e_parallel, TCG_CALL_NO_WG, void, env, tl, tr)
>
> +DEF_HELPER_FLAGS_1(ldc_check, TCG_CALL_NO_RWG, void, tl)
> +
>  DEF_HELPER_FLAGS_4(probe, TCG_CALL_NO_WG, tr, env, tl, i32, i32)
>
>  DEF_HELPER_FLAGS_1(loaded_fr0, TCG_CALL_NO_RWG, void, env)
> diff --git a/target/hppa/op_helper.c b/target/hppa/op_helper.c
> index f0516e81f1..345cef2c08 100644
> --- a/target/hppa/op_helper.c
> +++ b/target/hppa/op_helper.c
> @@ -153,6 +153,15 @@ void HELPER(stby_e_parallel)(CPUHPPAState *env, target_ulong addr,
>      do_stby_e(env, addr, val, true, GETPC());
>  }
>
> +void HELPER(ldc_check)(target_ulong addr)
> +{
> +    if (unlikely(addr & 0xf)) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "Undefined ldc to address unaligned mod 16: "
> +                      TARGET_FMT_lx "\n", addr);
> +    }
> +}
> +
>  target_ureg HELPER(probe)(CPUHPPAState *env, target_ulong addr,
>                            uint32_t level, uint32_t want)
>  {
> diff --git a/target/hppa/translate.c b/target/hppa/translate.c
> index 2f8d407a82..669381dc1d 100644
> --- a/target/hppa/translate.c
> +++ b/target/hppa/translate.c
> @@ -2942,7 +2942,7 @@ static bool trans_st(DisasContext *ctx, arg_ldst *a)
>
>  static bool trans_ldc(DisasContext *ctx, arg_ldst *a)
>  {
> -    MemOp mop = MO_TEUL | MO_ALIGN_16 | a->size;
> +    MemOp mop = MO_TE | MO_ALIGN | a->size;
>      TCGv_reg zero, dest, ofs;
>      TCGv_tl addr;
>
> @@ -2958,8 +2958,12 @@ static bool trans_ldc(DisasContext *ctx, arg_ldst *a)
>
>      form_gva(ctx, &addr, &ofs, a->b, a->x, a->scale ? a->size : 0,
>               a->disp, a->sp, a->m, ctx->mmu_idx == MMU_PHYS_IDX);
> +
> +    gen_helper_ldc_check(addr);
>      zero = tcg_const_reg(0);
>      tcg_gen_atomic_xchg_reg(dest, addr, zero, ctx->mmu_idx, mop);
> +    tcg_temp_free(zero);
> +
>      if (a->m) {
>          save_gpr(ctx, a->b, ofs);
>      }
>
Philippe Mathieu-Daudé Jan. 17, 2020, 4:13 p.m. UTC | #2
On 1/17/20 4:49 PM, Helge Deller wrote:
> On 17.01.20 02:53, Richard Henderson wrote:
>> The PA-RISC 1.1 specification says that LDCW must be aligned mod 16
>> or the operation is undefined.  However, real hardware only generates
>> an unaligned access trap for unaligned mod 4.
>>
>> Match real hardware, but diagnose with GUEST_ERROR a violation of the
>> specification.
>>
>> Reported-by: Helge Deller <deller@gmx.de>
>> Suggested-by: John David Anglin <dave.anglin@bell.net>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>
>> Helge, can you please test this against your failing kernel?
>> You will of course want to add -D logfile -d guest_errors to
>> you qemu command-line.
> 
> Yes, works as expected.
> Thanks!
> 
> Please add:
> Tested-by: Helge Deller <deller@gmx.de>
> 
> 
> [deller]$ tail -f logfile
> Undefined ldc to address unaligned mod 16: 00000504fa6c7848
> Undefined ldc to address unaligned mod 16: 00000504fa6c7a48
> Undefined ldc to address unaligned mod 16: 00000506f9434848
> Undefined ldc to address unaligned mod 16: 00000506f9434a48
> Undefined ldc to address unaligned mod 16: 00000508fa036848
> Undefined ldc to address unaligned mod 16: 00000508fa036a48
> Undefined ldc to address unaligned mod 16: 0000050afa8c4848
> Undefined ldc to address unaligned mod 16: 0000050afa8c4a48
> Undefined ldc to address unaligned mod 16: 0000050cf94d1848
> Undefined ldc to address unaligned mod 16: 0000050cf94d1a48
> ....
> 
> 
> 
>>
>>
>> r~
>>
>> ---
>>   target/hppa/helper.h    | 2 ++
>>   target/hppa/op_helper.c | 9 +++++++++
>>   target/hppa/translate.c | 6 +++++-
>>   3 files changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/hppa/helper.h b/target/hppa/helper.h
>> index 38d834ef6b..2d483aab58 100644
>> --- a/target/hppa/helper.h
>> +++ b/target/hppa/helper.h
>> @@ -17,6 +17,8 @@ DEF_HELPER_FLAGS_3(stby_b_parallel, TCG_CALL_NO_WG, void, env, tl, tr)
>>   DEF_HELPER_FLAGS_3(stby_e, TCG_CALL_NO_WG, void, env, tl, tr)
>>   DEF_HELPER_FLAGS_3(stby_e_parallel, TCG_CALL_NO_WG, void, env, tl, tr)
>>
>> +DEF_HELPER_FLAGS_1(ldc_check, TCG_CALL_NO_RWG, void, tl)
>> +
>>   DEF_HELPER_FLAGS_4(probe, TCG_CALL_NO_WG, tr, env, tl, i32, i32)
>>
>>   DEF_HELPER_FLAGS_1(loaded_fr0, TCG_CALL_NO_RWG, void, env)
>> diff --git a/target/hppa/op_helper.c b/target/hppa/op_helper.c
>> index f0516e81f1..345cef2c08 100644
>> --- a/target/hppa/op_helper.c
>> +++ b/target/hppa/op_helper.c
>> @@ -153,6 +153,15 @@ void HELPER(stby_e_parallel)(CPUHPPAState *env, target_ulong addr,
>>       do_stby_e(env, addr, val, true, GETPC());
>>   }
>>
>> +void HELPER(ldc_check)(target_ulong addr)
>> +{
>> +    if (unlikely(addr & 0xf)) {
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "Undefined ldc to address unaligned mod 16: "

"to unaligned address mod 16"?

>> +                      TARGET_FMT_lx "\n", addr);
>> +    }
>> +}
>> +
>>   target_ureg HELPER(probe)(CPUHPPAState *env, target_ulong addr,
>>                             uint32_t level, uint32_t want)
>>   {
>> diff --git a/target/hppa/translate.c b/target/hppa/translate.c
>> index 2f8d407a82..669381dc1d 100644
>> --- a/target/hppa/translate.c
>> +++ b/target/hppa/translate.c
>> @@ -2942,7 +2942,7 @@ static bool trans_st(DisasContext *ctx, arg_ldst *a)
>>
>>   static bool trans_ldc(DisasContext *ctx, arg_ldst *a)
>>   {
>> -    MemOp mop = MO_TEUL | MO_ALIGN_16 | a->size;
>> +    MemOp mop = MO_TE | MO_ALIGN | a->size;


Hmmm you changed MO_TEUL -> MO_TE, so from MO_32 to MO_8.

Per your description, shouldn't this be MO_TEUL | MO_ALIGN_4?

>>       TCGv_reg zero, dest, ofs;
>>       TCGv_tl addr;
>>
>> @@ -2958,8 +2958,12 @@ static bool trans_ldc(DisasContext *ctx, arg_ldst *a)
>>
>>       form_gva(ctx, &addr, &ofs, a->b, a->x, a->scale ? a->size : 0,
>>                a->disp, a->sp, a->m, ctx->mmu_idx == MMU_PHYS_IDX);
>> +
>> +    gen_helper_ldc_check(addr);
>>       zero = tcg_const_reg(0);
>>       tcg_gen_atomic_xchg_reg(dest, addr, zero, ctx->mmu_idx, mop);
>> +    tcg_temp_free(zero);
>> +
>>       if (a->m) {
>>           save_gpr(ctx, a->b, ofs);
>>       }
>>
> 
>
Helge Deller Jan. 17, 2020, 5:01 p.m. UTC | #3
On 17.01.20 17:13, Philippe Mathieu-Daudé wrote:
> On 1/17/20 4:49 PM, Helge Deller wrote:
>> On 17.01.20 02:53, Richard Henderson wrote:
>>> The PA-RISC 1.1 specification says that LDCW must be aligned mod 16
>>> or the operation is undefined.  However, real hardware only generates
>>> an unaligned access trap for unaligned mod 4.
>>>
>>> Match real hardware, but diagnose with GUEST_ERROR a violation of the
>>> specification.
>>>
>>> Reported-by: Helge Deller <deller@gmx.de>
>>> Suggested-by: John David Anglin <dave.anglin@bell.net>
>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>>
>>> Helge, can you please test this against your failing kernel?
>>> You will of course want to add -D logfile -d guest_errors to
>>> you qemu command-line.
>>
>> Yes, works as expected.
>> Thanks!
>>
>> Please add:
>> Tested-by: Helge Deller <deller@gmx.de>
>>
>>
>> [deller]$ tail -f logfile
>> Undefined ldc to address unaligned mod 16: 00000504fa6c7848
>> Undefined ldc to address unaligned mod 16: 00000504fa6c7a48
>> Undefined ldc to address unaligned mod 16: 00000506f9434848
>> Undefined ldc to address unaligned mod 16: 00000506f9434a48
>> Undefined ldc to address unaligned mod 16: 00000508fa036848
>> Undefined ldc to address unaligned mod 16: 00000508fa036a48
>> Undefined ldc to address unaligned mod 16: 0000050afa8c4848
>> Undefined ldc to address unaligned mod 16: 0000050afa8c4a48
>> Undefined ldc to address unaligned mod 16: 0000050cf94d1848
>> Undefined ldc to address unaligned mod 16: 0000050cf94d1a48
>> ....
>>
>>
>>
>>>
>>>
>>> r~
>>>
>>> ---
>>>   target/hppa/helper.h    | 2 ++
>>>   target/hppa/op_helper.c | 9 +++++++++
>>>   target/hppa/translate.c | 6 +++++-
>>>   3 files changed, 16 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/target/hppa/helper.h b/target/hppa/helper.h
>>> index 38d834ef6b..2d483aab58 100644
>>> --- a/target/hppa/helper.h
>>> +++ b/target/hppa/helper.h
>>> @@ -17,6 +17,8 @@ DEF_HELPER_FLAGS_3(stby_b_parallel, TCG_CALL_NO_WG, void, env, tl, tr)
>>>   DEF_HELPER_FLAGS_3(stby_e, TCG_CALL_NO_WG, void, env, tl, tr)
>>>   DEF_HELPER_FLAGS_3(stby_e_parallel, TCG_CALL_NO_WG, void, env, tl, tr)
>>>
>>> +DEF_HELPER_FLAGS_1(ldc_check, TCG_CALL_NO_RWG, void, tl)
>>> +
>>>   DEF_HELPER_FLAGS_4(probe, TCG_CALL_NO_WG, tr, env, tl, i32, i32)
>>>
>>>   DEF_HELPER_FLAGS_1(loaded_fr0, TCG_CALL_NO_RWG, void, env)
>>> diff --git a/target/hppa/op_helper.c b/target/hppa/op_helper.c
>>> index f0516e81f1..345cef2c08 100644
>>> --- a/target/hppa/op_helper.c
>>> +++ b/target/hppa/op_helper.c
>>> @@ -153,6 +153,15 @@ void HELPER(stby_e_parallel)(CPUHPPAState *env, target_ulong addr,
>>>       do_stby_e(env, addr, val, true, GETPC());
>>>   }
>>>
>>> +void HELPER(ldc_check)(target_ulong addr)
>>> +{
>>> +    if (unlikely(addr & 0xf)) {
>>> +        qemu_log_mask(LOG_GUEST_ERROR,
>>> +                      "Undefined ldc to address unaligned mod 16: "
>
> "to unaligned address mod 16"?
>
>>> +                      TARGET_FMT_lx "\n", addr);
>>> +    }
>>> +}
>>> +
>>>   target_ureg HELPER(probe)(CPUHPPAState *env, target_ulong addr,
>>>                             uint32_t level, uint32_t want)
>>>   {
>>> diff --git a/target/hppa/translate.c b/target/hppa/translate.c
>>> index 2f8d407a82..669381dc1d 100644
>>> --- a/target/hppa/translate.c
>>> +++ b/target/hppa/translate.c
>>> @@ -2942,7 +2942,7 @@ static bool trans_st(DisasContext *ctx, arg_ldst *a)
>>>
>>>   static bool trans_ldc(DisasContext *ctx, arg_ldst *a)
>>>   {
>>> -    MemOp mop = MO_TEUL | MO_ALIGN_16 | a->size;
>>> +    MemOp mop = MO_TE | MO_ALIGN | a->size;
>
>
> Hmmm you changed MO_TEUL -> MO_TE, so from MO_32 to MO_8.
>
> Per your description, shouldn't this be MO_TEUL | MO_ALIGN_4?

>>>       TCGv_reg zero, dest, ofs;
>>>       TCGv_tl addr;
>>>
>>> @@ -2958,8 +2958,12 @@ static bool trans_ldc(DisasContext *ctx, arg_ldst *a)
>>>
>>>       form_gva(ctx, &addr, &ofs, a->b, a->x, a->scale ? a->size : 0,
>>>                a->disp, a->sp, a->m, ctx->mmu_idx == MMU_PHYS_IDX);
>>> +
>>> +    gen_helper_ldc_check(addr);

Actually, for 64-bit the address is allowed to be 4-byte aligned, as long as the
"co" completer is given, e.g. this would be OK:  "ldcw,co  0(addr),%reg".
Maybe adding something like (if TARGET_32BIT...) now would make sense, so we don't get it
wrong when 64bit gets added?

Helge



>>>       zero = tcg_const_reg(0);
>>>       tcg_gen_atomic_xchg_reg(dest, addr, zero, ctx->mmu_idx, mop);
>>> +    tcg_temp_free(zero);
>>> +
>>>       if (a->m) {
>>>           save_gpr(ctx, a->b, ofs);
>>>       }
>>>
>>
>>
>
Richard Henderson Jan. 17, 2020, 5:30 p.m. UTC | #4
On 1/17/20 6:13 AM, Philippe Mathieu-Daudé wrote:
>>> -    MemOp mop = MO_TEUL | MO_ALIGN_16 | a->size;
>>> +    MemOp mop = MO_TE | MO_ALIGN | a->size;
> 
> 
> Hmmm you changed MO_TEUL -> MO_TE, so from MO_32 to MO_8.
> 
> Per your description, shouldn't this be MO_TEUL | MO_ALIGN_4?

The "UL" part is also being added by a->size.  This code was written this way
in preparation for the 64-bit ldc, and the bug was not noticable because we
don't have that yet.


r~
John David Anglin Jan. 17, 2020, 5:33 p.m. UTC | #5
This is not related to the patch but there is one other corner issue with the load and clear instructions.

When the target register is GR0, the instruction may be implemented as a normal load and clear
which clears memory, or it may be aliased to the equivalent-sized load instruction, in which case
it behaves exactly like that prefetch instruction and does not clear the data in memory.

See page 6-12 on PA 2.0 architecture.

Don't know what was actually done.

Regards,
Dave
Philippe Mathieu-Daudé Jan. 17, 2020, 6:20 p.m. UTC | #6
On 1/17/20 6:30 PM, Richard Henderson wrote:
> On 1/17/20 6:13 AM, Philippe Mathieu-Daudé wrote:
>>>> -    MemOp mop = MO_TEUL | MO_ALIGN_16 | a->size;
>>>> +    MemOp mop = MO_TE | MO_ALIGN | a->size;
>>
>>
>> Hmmm you changed MO_TEUL -> MO_TE, so from MO_32 to MO_8.
>>
>> Per your description, shouldn't this be MO_TEUL | MO_ALIGN_4?
> 
> The "UL" part is also being added by a->size.  This code was written this way
> in preparation for the 64-bit ldc, and the bug was not noticable because we
> don't have that yet.

Ah I missed the a->size.

So on 32-bit the hw trap doesn't trap on unaligned 16, but traps on 
unaligned 4.

On 64-bit we don't know yet, but IIUC we expect to not trap on unaligned 8.

Looking at target/hppa/insns.decode:

&ldst           t b x disp sp m scale size

@ldstx          ...... b:5 x:5 sp:2 scale:1 ....... m:1 t:5     &ldst disp=0
@ldim5          ...... b:5 ..... sp:2 ......... t:5     \
                 &ldst disp=%im5_16 x=0 scale=0 m=%ma_to_m

ldc             000011 ..... ..... .. . 1 -- 0111      ......   @ldim5 
size=2
ldc             000011 ..... ..... .. . 0 -- 0111      ......   @ldstx 
size=2

We have a->size = 2 = MO_32 = MO_UL.

So do you plan to add LDCD from PA2.0 using size=3, OK.

 From "exec/memop.h":

      * MO_ALIGN supposes the alignment size is the size of a memory access.
      *
      * There are three options:
      * - unaligned access permitted (MO_UNALN).
      * - an alignment to the size of an access (MO_ALIGN);

Ah so with LDCW we already access a word, so MO_ALIGN -> MO_ALIGN_4.

With LDLD we'll have MO_ALIGN -> MO_ALIGN_8.

Now "MemOp mop = MO_TE | MO_ALIGN | a->size;" makes sense!

The power of your decodetree script amazed me again :)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Richard Henderson Jan. 17, 2020, 6:34 p.m. UTC | #7
On 1/17/20 7:01 AM, Helge Deller wrote:
> Maybe adding something like (if TARGET_32BIT...) now would make sense, so we don't get it
> wrong when 64bit gets added?

I'll add a "TODO: HPPA64" comment.


r~
Philippe Mathieu-Daudé Jan. 17, 2020, 6:50 p.m. UTC | #8
On Fri, Jan 17, 2020 at 2:53 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> The PA-RISC 1.1 specification says that LDCW must be aligned mod 16
> or the operation is undefined.  However, real hardware only generates
> an unaligned access trap for unaligned mod 4.

This Linux kernel commit seems relevant:

https://github.com/torvalds/linux/commit/14e256c107304#diff-e85862c7227599cb24e36494f75948d5R159-R164

  /* From: "Jim Hull" <jim.hull of hp.com>
  I've attached a summary of the change, but basically, for PA 2.0, as
  long as the ",CO" (coherent operation) completer is specified, then the
  16-byte alignment requirement for ldcw and ldcd is relaxed, and instead
  they only require "natural" alignment (4-byte for ldcw, 8-byte for
  ldcd). */

(I couldn't find the original post on
http://lists.parisc-linux.org/pipermail/parisc-linux/)

Maybe you can point to it in the description.

> Match real hardware, but diagnose with GUEST_ERROR a violation of the
> specification.
>
> Reported-by: Helge Deller <deller@gmx.de>
> Suggested-by: John David Anglin <dave.anglin@bell.net>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>
> Helge, can you please test this against your failing kernel?
> You will of course want to add -D logfile -d guest_errors to
> you qemu command-line.
>
>
> r~
>
> ---
>  target/hppa/helper.h    | 2 ++
>  target/hppa/op_helper.c | 9 +++++++++
>  target/hppa/translate.c | 6 +++++-
>  3 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/target/hppa/helper.h b/target/hppa/helper.h
> index 38d834ef6b..2d483aab58 100644
> --- a/target/hppa/helper.h
> +++ b/target/hppa/helper.h
> @@ -17,6 +17,8 @@ DEF_HELPER_FLAGS_3(stby_b_parallel, TCG_CALL_NO_WG, void, env, tl, tr)
>  DEF_HELPER_FLAGS_3(stby_e, TCG_CALL_NO_WG, void, env, tl, tr)
>  DEF_HELPER_FLAGS_3(stby_e_parallel, TCG_CALL_NO_WG, void, env, tl, tr)
>
> +DEF_HELPER_FLAGS_1(ldc_check, TCG_CALL_NO_RWG, void, tl)
> +
>  DEF_HELPER_FLAGS_4(probe, TCG_CALL_NO_WG, tr, env, tl, i32, i32)
>
>  DEF_HELPER_FLAGS_1(loaded_fr0, TCG_CALL_NO_RWG, void, env)
> diff --git a/target/hppa/op_helper.c b/target/hppa/op_helper.c
> index f0516e81f1..345cef2c08 100644
> --- a/target/hppa/op_helper.c
> +++ b/target/hppa/op_helper.c
> @@ -153,6 +153,15 @@ void HELPER(stby_e_parallel)(CPUHPPAState *env, target_ulong addr,
>      do_stby_e(env, addr, val, true, GETPC());
>  }
>
> +void HELPER(ldc_check)(target_ulong addr)
> +{
> +    if (unlikely(addr & 0xf)) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "Undefined ldc to address unaligned mod 16: "
> +                      TARGET_FMT_lx "\n", addr);
> +    }
> +}
> +
>  target_ureg HELPER(probe)(CPUHPPAState *env, target_ulong addr,
>                            uint32_t level, uint32_t want)
>  {
> diff --git a/target/hppa/translate.c b/target/hppa/translate.c
> index 2f8d407a82..669381dc1d 100644
> --- a/target/hppa/translate.c
> +++ b/target/hppa/translate.c
> @@ -2942,7 +2942,7 @@ static bool trans_st(DisasContext *ctx, arg_ldst *a)
>
>  static bool trans_ldc(DisasContext *ctx, arg_ldst *a)
>  {
> -    MemOp mop = MO_TEUL | MO_ALIGN_16 | a->size;
> +    MemOp mop = MO_TE | MO_ALIGN | a->size;
>      TCGv_reg zero, dest, ofs;
>      TCGv_tl addr;
>
> @@ -2958,8 +2958,12 @@ static bool trans_ldc(DisasContext *ctx, arg_ldst *a)
>
>      form_gva(ctx, &addr, &ofs, a->b, a->x, a->scale ? a->size : 0,
>               a->disp, a->sp, a->m, ctx->mmu_idx == MMU_PHYS_IDX);
> +
> +    gen_helper_ldc_check(addr);
>      zero = tcg_const_reg(0);
>      tcg_gen_atomic_xchg_reg(dest, addr, zero, ctx->mmu_idx, mop);
> +    tcg_temp_free(zero);
> +
>      if (a->m) {
>          save_gpr(ctx, a->b, ofs);
>      }
> --
> 2.20.1
>
>
Richard Henderson Jan. 17, 2020, 7:23 p.m. UTC | #9
On 1/17/20 8:50 AM, Philippe Mathieu-Daudé wrote:
> On Fri, Jan 17, 2020 at 2:53 AM Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> The PA-RISC 1.1 specification says that LDCW must be aligned mod 16
>> or the operation is undefined.  However, real hardware only generates
>> an unaligned access trap for unaligned mod 4.
> 
> This Linux kernel commit seems relevant:
> 
> https://github.com/torvalds/linux/commit/14e256c107304#diff-e85862c7227599cb24e36494f75948d5R159-R164
> 
>   /* From: "Jim Hull" <jim.hull of hp.com>
>   I've attached a summary of the change, but basically, for PA 2.0, as
>   long as the ",CO" (coherent operation) completer is specified, then the
>   16-byte alignment requirement for ldcw and ldcd is relaxed, and instead
>   they only require "natural" alignment (4-byte for ldcw, 8-byte for
>   ldcd). */

It isn't completely relevant.  We don't implement hppa 2.0.

I added a TODO comment for HPPA64, as I said in reply to Dave Anglin elsewhere
in this thread.


r~
Helge Deller Jan. 17, 2020, 7:57 p.m. UTC | #10
On 17.01.20 20:23, Richard Henderson wrote:
> On 1/17/20 8:50 AM, Philippe Mathieu-Daudé wrote:
>> On Fri, Jan 17, 2020 at 2:53 AM Richard Henderson
>> <richard.henderson@linaro.org> wrote:
>>>
>>> The PA-RISC 1.1 specification says that LDCW must be aligned mod 16
>>> or the operation is undefined.  However, real hardware only generates
>>> an unaligned access trap for unaligned mod 4.
>>
>> This Linux kernel commit seems relevant:
>>
>> https://github.com/torvalds/linux/commit/14e256c107304#diff-e85862c7227599cb24e36494f75948d5R159-R164
>>
>>   /* From: "Jim Hull" <jim.hull of hp.com>
>>   I've attached a summary of the change, but basically, for PA 2.0, as
>>   long as the ",CO" (coherent operation) completer is specified, then the
>>   16-byte alignment requirement for ldcw and ldcd is relaxed, and instead
>>   they only require "natural" alignment (4-byte for ldcw, 8-byte for
>>   ldcd). */
>
> It isn't completely relevant.  We don't implement hppa 2.0.
>
> I added a TODO comment for HPPA64, as I said in reply to Dave Anglin elsewhere
> in this thread.

I agree. A TODO comment should be sufficient for now.

Thanks!
Helge
Philippe Mathieu-Daudé Jan. 18, 2020, 6:10 a.m. UTC | #11
On 1/17/20 8:23 PM, Richard Henderson wrote:
> On 1/17/20 8:50 AM, Philippe Mathieu-Daudé wrote:
>> On Fri, Jan 17, 2020 at 2:53 AM Richard Henderson
>> <richard.henderson@linaro.org> wrote:
>>>
>>> The PA-RISC 1.1 specification says that LDCW must be aligned mod 16
>>> or the operation is undefined.  However, real hardware only generates
>>> an unaligned access trap for unaligned mod 4.
>>
>> This Linux kernel commit seems relevant:
>>
>> https://github.com/torvalds/linux/commit/14e256c107304#diff-e85862c7227599cb24e36494f75948d5R159-R164
>>
>>    /* From: "Jim Hull" <jim.hull of hp.com>
>>    I've attached a summary of the change, but basically, for PA 2.0, as
>>    long as the ",CO" (coherent operation) completer is specified, then the
>>    16-byte alignment requirement for ldcw and ldcd is relaxed, and instead
>>    they only require "natural" alignment (4-byte for ldcw, 8-byte for
>>    ldcd). */
> 
> It isn't completely relevant.  We don't implement hppa 2.0.

Oh... It was late and I misread it as:

   if (PA2)
     ...
   else
     ... "instead of PA 2.0 ... the 16-byte alignment ... is
     relaxed, and insted only require "natural" alignment ..."

   endif

> 
> I added a TODO comment for HPPA64, as I said in reply to Dave Anglin elsewhere
> in this thread.

Patch
diff mbox series

diff --git a/target/hppa/helper.h b/target/hppa/helper.h
index 38d834ef6b..2d483aab58 100644
--- a/target/hppa/helper.h
+++ b/target/hppa/helper.h
@@ -17,6 +17,8 @@  DEF_HELPER_FLAGS_3(stby_b_parallel, TCG_CALL_NO_WG, void, env, tl, tr)
 DEF_HELPER_FLAGS_3(stby_e, TCG_CALL_NO_WG, void, env, tl, tr)
 DEF_HELPER_FLAGS_3(stby_e_parallel, TCG_CALL_NO_WG, void, env, tl, tr)
 
+DEF_HELPER_FLAGS_1(ldc_check, TCG_CALL_NO_RWG, void, tl)
+
 DEF_HELPER_FLAGS_4(probe, TCG_CALL_NO_WG, tr, env, tl, i32, i32)
 
 DEF_HELPER_FLAGS_1(loaded_fr0, TCG_CALL_NO_RWG, void, env)
diff --git a/target/hppa/op_helper.c b/target/hppa/op_helper.c
index f0516e81f1..345cef2c08 100644
--- a/target/hppa/op_helper.c
+++ b/target/hppa/op_helper.c
@@ -153,6 +153,15 @@  void HELPER(stby_e_parallel)(CPUHPPAState *env, target_ulong addr,
     do_stby_e(env, addr, val, true, GETPC());
 }
 
+void HELPER(ldc_check)(target_ulong addr)
+{
+    if (unlikely(addr & 0xf)) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "Undefined ldc to address unaligned mod 16: "
+                      TARGET_FMT_lx "\n", addr);
+    }
+}
+
 target_ureg HELPER(probe)(CPUHPPAState *env, target_ulong addr,
                           uint32_t level, uint32_t want)
 {
diff --git a/target/hppa/translate.c b/target/hppa/translate.c
index 2f8d407a82..669381dc1d 100644
--- a/target/hppa/translate.c
+++ b/target/hppa/translate.c
@@ -2942,7 +2942,7 @@  static bool trans_st(DisasContext *ctx, arg_ldst *a)
 
 static bool trans_ldc(DisasContext *ctx, arg_ldst *a)
 {
-    MemOp mop = MO_TEUL | MO_ALIGN_16 | a->size;
+    MemOp mop = MO_TE | MO_ALIGN | a->size;
     TCGv_reg zero, dest, ofs;
     TCGv_tl addr;
 
@@ -2958,8 +2958,12 @@  static bool trans_ldc(DisasContext *ctx, arg_ldst *a)
 
     form_gva(ctx, &addr, &ofs, a->b, a->x, a->scale ? a->size : 0,
              a->disp, a->sp, a->m, ctx->mmu_idx == MMU_PHYS_IDX);
+
+    gen_helper_ldc_check(addr);
     zero = tcg_const_reg(0);
     tcg_gen_atomic_xchg_reg(dest, addr, zero, ctx->mmu_idx, mop);
+    tcg_temp_free(zero);
+
     if (a->m) {
         save_gpr(ctx, a->b, ofs);
     }