Re: [Qemu-devel] [PATCH v2 21/68] target/arm: Convert Synchronization primitives
On 8/27/19 4:10 AM, Peter Maydell wrote:
> On Tue, 27 Aug 2019 at 11:46, Peter Maydell wrote:
>> ...OK, not this specific function, as I just noticed it's the _a32
>> one, but trans_STREXB(), trans_STREXH(), etc are wrong.
>
> I did a quick grep through for places checking the 6K condition,
> and I think these are the only ones that need changing:
>
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index b4d53f3d37f..58e50f2d808 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -8874,7 +8874,7 @@ static bool trans_STREXD_t32(DisasContext *s,
> arg_STREX *a)
>
> static bool trans_STREXB(DisasContext *s, arg_STREX *a)
> {
> -if (!ENABLE_ARCH_6K) {
> +if (!ENABLE_ARCH_6K && !arm_dc_feature(s, ARM_FEATURE_M)) {
Looking again, I think the correct test is
if (s->thumb ? !ENABLE_ARCH_7 : !ENABLE_ARCH_6K)
for all of these.
r~
Re: [Qemu-devel] [PATCH v2 21/68] target/arm: Convert Synchronization primitives
On Tue, 27 Aug 2019 at 11:46, Peter Maydell wrote:
> ...OK, not this specific function, as I just noticed it's the _a32
> one, but trans_STREXB(), trans_STREXH(), etc are wrong.
I did a quick grep through for places checking the 6K condition,
and I think these are the only ones that need changing:
diff --git a/target/arm/translate.c b/target/arm/translate.c
index b4d53f3d37f..58e50f2d808 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -8874,7 +8874,7 @@ static bool trans_STREXD_t32(DisasContext *s,
arg_STREX *a)
static bool trans_STREXB(DisasContext *s, arg_STREX *a)
{
-if (!ENABLE_ARCH_6K) {
+if (!ENABLE_ARCH_6K && !arm_dc_feature(s, ARM_FEATURE_M)) {
return false;
}
return op_strex(s, a, MO_8, false);
@@ -8882,7 +8882,7 @@ static bool trans_STREXB(DisasContext *s, arg_STREX *a)
static bool trans_STREXH(DisasContext *s, arg_STREX *a)
{
-if (!ENABLE_ARCH_6K) {
+if (!ENABLE_ARCH_6K && !arm_dc_feature(s, ARM_FEATURE_M)) {
return false;
}
return op_strex(s, a, MO_16, false);
@@ -9004,7 +9004,7 @@ static bool trans_LDREXD_t32(DisasContext *s,
arg_LDREX *a)
static bool trans_LDREXB(DisasContext *s, arg_LDREX *a)
{
-if (!ENABLE_ARCH_6K) {
+if (!ENABLE_ARCH_6K && !arm_dc_feature(s, ARM_FEATURE_M)) {
return false;
}
return op_ldrex(s, a, MO_8, false);
@@ -9012,7 +9012,7 @@ static bool trans_LDREXB(DisasContext *s, arg_LDREX *a)
static bool trans_LDREXH(DisasContext *s, arg_LDREX *a)
{
-if (!ENABLE_ARCH_6K) {
+if (!ENABLE_ARCH_6K && !arm_dc_feature(s, ARM_FEATURE_M)) {
return false;
}
return op_ldrex(s, a, MO_16, false);
@@ -10236,7 +10236,7 @@ static bool trans_CPS_v6m(DisasContext *s,
arg_CPS_v6m *a)
static bool trans_CLREX(DisasContext *s, arg_CLREX *a)
{
-if (!ENABLE_ARCH_6K) {
+if (!ENABLE_ARCH_6K && !arm_dc_feature(s, ARM_FEATURE_M)) {
return false;
}
gen_clrex(s);
thanks
-- PMM
Re: [Qemu-devel] [PATCH v2 21/68] target/arm: Convert Synchronization primitives
On Tue, 27 Aug 2019 at 11:44, Peter Maydell wrote:
>
> On Mon, 19 Aug 2019 at 22:38, Richard Henderson
> wrote:
> >
> > Signed-off-by: Richard Henderson
> > ---
> > target/arm/translate.c | 560 ++---
> > target/arm/a32.decode | 48
> > target/arm/t32.decode | 46
> > 3 files changed, 396 insertions(+), 258 deletions(-)
>
> > +static bool trans_STREXD_a32(DisasContext *s, arg_STREX *a)
> > +{
> > +if (!ENABLE_ARCH_6K || (a->rt & 1)) {
> > +return false;
> > +}
> > +a->rt2 = a->rt + 1;
> > +return op_strex(s, a, MO_64, false);
> > +}
>
> I've just noticed that there's a bug in these checks -- the
> M-profile CPUs don't have the V6K feature, but they should
> still have STREXB/STREXH/STREXD, and with this test they'll
> incorrectly UNDEF them.
...OK, not this specific function, as I just noticed it's the _a32
one, but trans_STREXB(), trans_STREXH(), etc are wrong.
thanks
-- PMM
Re: [Qemu-devel] [PATCH v2 21/68] target/arm: Convert Synchronization primitives
On Mon, 19 Aug 2019 at 22:38, Richard Henderson
wrote:
>
> Signed-off-by: Richard Henderson
> ---
> target/arm/translate.c | 560 ++---
> target/arm/a32.decode | 48
> target/arm/t32.decode | 46
> 3 files changed, 396 insertions(+), 258 deletions(-)
> +static bool trans_STREXD_a32(DisasContext *s, arg_STREX *a)
> +{
> +if (!ENABLE_ARCH_6K || (a->rt & 1)) {
> +return false;
> +}
> +a->rt2 = a->rt + 1;
> +return op_strex(s, a, MO_64, false);
> +}
I've just noticed that there's a bug in these checks -- the
M-profile CPUs don't have the V6K feature, but they should
still have STREXB/STREXH/STREXD, and with this test they'll
incorrectly UNDEF them.
thanks
-- PMM
Re: [Qemu-devel] [PATCH v2 21/68] target/arm: Convert Synchronization primitives
On 8/23/19 8:28 AM, Peter Maydell wrote:
>> +gen_aa32_st_i32(s, tmp, addr, get_mem_index(s), mop | s->be_data);
>> +disas_set_da_iss(s, mop, a->rt | ISSIsAcqRel | ISSIsWrite);
...
>> -switch (op1) {
>> -case 0: /* stl */
>> -gen_aa32_st32_iss(s, tmp, addr,
>> - get_mem_index(s),
>> - rm | ISSIsAcqRel);
>> -break;
>> -case 2: /* stlb */
>> -gen_aa32_st8_iss(s, tmp, addr,
>> - get_mem_index(s),
>> - rm | ISSIsAcqRel);
>> -break;
>> -case 3: /* stlh */
>> -gen_aa32_st16_iss(s, tmp, addr,
>> - get_mem_index(s),
>> - rm | ISSIsAcqRel);
>> -break;
>
> Any particular reason for doing separate gen_aa32_st_i32()
> and disas_set_da_iss() calls rather than using the _gen_aa32_st32_iss()
> function that the old decoder did?
I think gen_aa32_st_i32 with the MemOp argument is preferable to a switch
statement that enumerates the different sizes.
We don't have a version of gen_aa32_st_i32 that also passes the ISS. While we
could add one, a separate call seems just as easy.
r~
Re: [Qemu-devel] [PATCH v2 21/68] target/arm: Convert Synchronization primitives
On Mon, 19 Aug 2019 at 22:38, Richard Henderson
wrote:
>
> Signed-off-by: Richard Henderson
> ---
> target/arm/translate.c | 560 ++---
> target/arm/a32.decode | 48
> target/arm/t32.decode | 46
> 3 files changed, 396 insertions(+), 258 deletions(-)
> +static bool op_stl(DisasContext *s, arg_STL *a, TCGMemOp mop)
> +{
> +TCGv_i32 addr, tmp;
> +
> +if (!ENABLE_ARCH_8) {
> +return false;
> +}
> +addr = load_reg(s, a->rn);
> +
> +tmp = load_reg(s, a->rt);
> +tcg_gen_mb(TCG_MO_ALL | TCG_BAR_STRL);
> +gen_aa32_st_i32(s, tmp, addr, get_mem_index(s), mop | s->be_data);
> +disas_set_da_iss(s, mop, a->rt | ISSIsAcqRel | ISSIsWrite);
> +
> +tcg_temp_free_i32(tmp);
> +tcg_temp_free_i32(addr);
> +return true;
> +}
> +
> -rm = insn & 0xf;
> -tmp = load_reg(s, rm);
> -switch (op1) {
> -case 0: /* stl */
> -gen_aa32_st32_iss(s, tmp, addr,
> - get_mem_index(s),
> - rm | ISSIsAcqRel);
> -break;
> -case 2: /* stlb */
> -gen_aa32_st8_iss(s, tmp, addr,
> - get_mem_index(s),
> - rm | ISSIsAcqRel);
> -break;
> -case 3: /* stlh */
> -gen_aa32_st16_iss(s, tmp, addr,
> - get_mem_index(s),
> - rm | ISSIsAcqRel);
> -break;
Any particular reason for doing separate gen_aa32_st_i32()
and disas_set_da_iss() calls rather than using the _gen_aa32_st32_iss()
function that the old decoder did?
either way
Reviewed-by: Peter Maydell
thanks
-- PMM
