diff mbox

[v2,for-2.10,3/3] target/arm: Require alignment for load exclusive

Message ID 20170815145714.17635-4-richard.henderson@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Richard Henderson Aug. 15, 2017, 2:57 p.m. UTC 
From: Alistair Francis <alistair.francis@xilinx.com>

Acording to the ARM ARM exclusive loads require the same allignment as
exclusive stores. Let's update the memops used for the load to match
that of the store. This adds the alignment requirement to the memops.

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
[rth: Require 16-byte alignment for 64-bit LDXP.]
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/translate-a64.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Comments

Eric Blake Aug. 15, 2017, 3:27 p.m. UTC | #1 
On 08/15/2017 09:57 AM, Richard Henderson wrote:
> From: Alistair Francis <alistair.francis@xilinx.com>
> 
> Acording to the ARM ARM exclusive loads require the same allignment as

s/Acording/According/
s/allignmnet/alignment/

> exclusive stores. Let's update the memops used for the load to match
> that of the store. This adds the alignment requirement to the memops.
> 
> Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> [rth: Require 16-byte alignment for 64-bit LDXP.]
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
Philippe Mathieu-Daudé Aug. 15, 2017, 3:56 p.m. UTC | #2 
Hi Richard,

On 08/15/2017 11:57 AM, Richard Henderson wrote:
> From: Alistair Francis <alistair.francis@xilinx.com>
> 
> Acording to the ARM ARM exclusive loads require the same allignment as
> exclusive stores. Let's update the memops used for the load to match
> that of the store. This adds the alignment requirement to the memops.
> 
> Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> [rth: Require 16-byte alignment for 64-bit LDXP.]
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/arm/translate-a64.c | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index eac545e4f2..2200e25be0 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -1861,7 +1861,7 @@ static void gen_load_exclusive(DisasContext *s, int rt, int rt2,
>           g_assert(size >= 2);
>           if (size == 2) {
>               /* The pair must be single-copy atomic for the doubleword.  */
> -            memop |= MO_64;
> +            memop |= MO_64 | MO_ALIGN;

isn't MO_ALIGN_8 enough?

>               tcg_gen_qemu_ld_i64(cpu_exclusive_val, addr, idx, memop);
>               if (s->be_data == MO_LE) {
>                   tcg_gen_extract_i64(cpu_reg(s, rt), cpu_exclusive_val, 0, 32);
> @@ -1871,10 +1871,11 @@ static void gen_load_exclusive(DisasContext *s, int rt, int rt2,
>                   tcg_gen_extract_i64(cpu_reg(s, rt2), cpu_exclusive_val, 0, 32);
>               }
>           } else {
> -            /* The pair must be single-copy atomic for *each* doubleword,
> -               but not the entire quadword.  */
> +            /* The pair must be single-copy atomic for *each* doubleword, not
> +               the entire quadword, however it must be quadword aligned.  */
>               memop |= MO_64;
> -            tcg_gen_qemu_ld_i64(cpu_exclusive_val, addr, idx, memop);
> +            tcg_gen_qemu_ld_i64(cpu_exclusive_val, addr, idx,
> +                                memop | MO_ALIGN_16);

ok

>   
>               TCGv_i64 addr2 = tcg_temp_new_i64();
>               tcg_gen_addi_i64(addr2, addr, 8);
> @@ -1885,7 +1886,7 @@ static void gen_load_exclusive(DisasContext *s, int rt, int rt2,
>               tcg_gen_mov_i64(cpu_reg(s, rt2), cpu_exclusive_high);
>           }
>       } else {
> -        memop |= size;
> +        memop |= size | MO_ALIGN;

MO_ALIGN_8 here too?

>           tcg_gen_qemu_ld_i64(cpu_exclusive_val, addr, idx, memop);
>           tcg_gen_mov_i64(cpu_reg(s, rt), cpu_exclusive_val);
>       }
> 

Regards,

Phil.
Peter Maydell Aug. 15, 2017, 4:14 p.m. UTC | #3 
On 15 August 2017 at 16:56, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Richard,
>
> On 08/15/2017 11:57 AM, Richard Henderson wrote:
>>
>> From: Alistair Francis <alistair.francis@xilinx.com>
>>
>> Acording to the ARM ARM exclusive loads require the same allignment as
>> exclusive stores. Let's update the memops used for the load to match
>> that of the store. This adds the alignment requirement to the memops.
>>
>> Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> [rth: Require 16-byte alignment for 64-bit LDXP.]
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   target/arm/translate-a64.c | 11 ++++++-----
>>   1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
>> index eac545e4f2..2200e25be0 100644
>> --- a/target/arm/translate-a64.c
>> +++ b/target/arm/translate-a64.c
>> @@ -1861,7 +1861,7 @@ static void gen_load_exclusive(DisasContext *s, int
>> rt, int rt2,
>>           g_assert(size >= 2);
>>           if (size == 2) {
>>               /* The pair must be single-copy atomic for the doubleword.
>> */
>> -            memop |= MO_64;
>> +            memop |= MO_64 | MO_ALIGN;
>
>
> isn't MO_ALIGN_8 enough?

MO_ALIGN is "align to size of access", ie to 8 bytes in this case.
MO_ALIGN_8 is "align to a specified size (8 bytes) which isn't the
 size of the access".
In this case the access size is 8 bytes so we don't need MO_ALIGN_8.

Alignments to something other than the access size are the oddball
case, so I think it makes sense to stick with MO_ALIGN for the
common case of "just aligned to the access size" so you can
spot the odd cases when you're reading the source.

thanks
-- PMM
Richard Henderson Aug. 15, 2017, 4:32 p.m. UTC | #4 
On 08/15/2017 08:56 AM, Philippe Mathieu-Daudé wrote:
>> @@ -1885,7 +1886,7 @@ static void gen_load_exclusive(DisasContext *s, int rt,
>> int rt2,
>>               tcg_gen_mov_i64(cpu_reg(s, rt2), cpu_exclusive_high);
>>           }
>>       } else {
>> -        memop |= size;
>> +        memop |= size | MO_ALIGN;
> 
> MO_ALIGN_8 here too?
> 
>>           tcg_gen_qemu_ld_i64(cpu_exclusive_val, addr, idx, memop);
>>           tcg_gen_mov_i64(cpu_reg(s, rt), cpu_exclusive_val);

Peter already pointed out that MO_ALIGN_N should be reserved for those cases
where an explicit size really needed.  You should note that using MO_ALIGN_8
would be actively wrong here -- it would incorrectly require 8 byte alignment
for the single byte access of LDXRB.


r~
Philippe Mathieu-Daudé Aug. 15, 2017, 5:48 p.m. UTC | #5 
On 08/15/2017 01:14 PM, Peter Maydell wrote:
> On 15 August 2017 at 16:56, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Hi Richard,
>>
>> On 08/15/2017 11:57 AM, Richard Henderson wrote:
>>>
>>> From: Alistair Francis <alistair.francis@xilinx.com>
>>>
>>> Acording to the ARM ARM exclusive loads require the same allignment as
>>> exclusive stores. Let's update the memops used for the load to match
>>> that of the store. This adds the alignment requirement to the memops.
>>>
>>> Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>> [rth: Require 16-byte alignment for 64-bit LDXP.]
>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>> ---
>>>    target/arm/translate-a64.c | 11 ++++++-----
>>>    1 file changed, 6 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
>>> index eac545e4f2..2200e25be0 100644
>>> --- a/target/arm/translate-a64.c
>>> +++ b/target/arm/translate-a64.c
>>> @@ -1861,7 +1861,7 @@ static void gen_load_exclusive(DisasContext *s, int
>>> rt, int rt2,
>>>            g_assert(size >= 2);
>>>            if (size == 2) {
>>>                /* The pair must be single-copy atomic for the doubleword.
>>> */
>>> -            memop |= MO_64;
>>> +            memop |= MO_64 | MO_ALIGN;
>>
>>
>> isn't MO_ALIGN_8 enough?
> 
> MO_ALIGN is "align to size of access", ie to 8 bytes in this case.
> MO_ALIGN_8 is "align to a specified size (8 bytes) which isn't the
>   size of the access".
> In this case the access size is 8 bytes so we don't need MO_ALIGN_8.
> 
> Alignments to something other than the access size are the oddball
> case, so I think it makes sense to stick with MO_ALIGN for the
> common case of "just aligned to the access size" so you can
> spot the odd cases when you're reading the source.

Ok, I misunderstood "MO_ALIGN supposes the alignment size is the size of 
a memory access."

thanks!

> 
> thanks
> -- PMM
>
Philippe Mathieu-Daudé Aug. 15, 2017, 5:49 p.m. UTC | #6 
On 08/15/2017 01:32 PM, Richard Henderson wrote:
> On 08/15/2017 08:56 AM, Philippe Mathieu-Daudé wrote:
>>> @@ -1885,7 +1886,7 @@ static void gen_load_exclusive(DisasContext *s, int rt,
>>> int rt2,
>>>                tcg_gen_mov_i64(cpu_reg(s, rt2), cpu_exclusive_high);
>>>            }
>>>        } else {
>>> -        memop |= size;
>>> +        memop |= size | MO_ALIGN;
>>
>> MO_ALIGN_8 here too?
>>
>>>            tcg_gen_qemu_ld_i64(cpu_exclusive_val, addr, idx, memop);
>>>            tcg_gen_mov_i64(cpu_reg(s, rt), cpu_exclusive_val);
> 
> Peter already pointed out that MO_ALIGN_N should be reserved for those cases
> where an explicit size really needed.  You should note that using MO_ALIGN_8
> would be actively wrong here -- it would incorrectly require 8 byte alignment
> for the single byte access of LDXRB.

Indeed I didn't think of that, thanks!

> 
> 
> r~
>
diff mbox

Patch

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index eac545e4f2..2200e25be0 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -1861,7 +1861,7 @@  static void gen_load_exclusive(DisasContext *s, int rt, int rt2,
         g_assert(size >= 2);
         if (size == 2) {
             /* The pair must be single-copy atomic for the doubleword.  */
-            memop |= MO_64;
+            memop |= MO_64 | MO_ALIGN;
             tcg_gen_qemu_ld_i64(cpu_exclusive_val, addr, idx, memop);
             if (s->be_data == MO_LE) {
                 tcg_gen_extract_i64(cpu_reg(s, rt), cpu_exclusive_val, 0, 32);
@@ -1871,10 +1871,11 @@  static void gen_load_exclusive(DisasContext *s, int rt, int rt2,
                 tcg_gen_extract_i64(cpu_reg(s, rt2), cpu_exclusive_val, 0, 32);
             }
         } else {
-            /* The pair must be single-copy atomic for *each* doubleword,
-               but not the entire quadword.  */
+            /* The pair must be single-copy atomic for *each* doubleword, not
+               the entire quadword, however it must be quadword aligned.  */
             memop |= MO_64;
-            tcg_gen_qemu_ld_i64(cpu_exclusive_val, addr, idx, memop);
+            tcg_gen_qemu_ld_i64(cpu_exclusive_val, addr, idx,
+                                memop | MO_ALIGN_16);
 
             TCGv_i64 addr2 = tcg_temp_new_i64();
             tcg_gen_addi_i64(addr2, addr, 8);
@@ -1885,7 +1886,7 @@  static void gen_load_exclusive(DisasContext *s, int rt, int rt2,
             tcg_gen_mov_i64(cpu_reg(s, rt2), cpu_exclusive_high);
         }
     } else {
-        memop |= size;
+        memop |= size | MO_ALIGN;
         tcg_gen_qemu_ld_i64(cpu_exclusive_val, addr, idx, memop);
         tcg_gen_mov_i64(cpu_reg(s, rt), cpu_exclusive_val);
     }