| Message ID | 20220901212931.27310-1-bcain@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Hexagon (target/hexagon) implement mutability mask for GPRs | expand |
On 9/1/22 22:29, Brian Cain wrote: > +void gen_masked_reg_write(TCGv cur_val, TCGv in_val, TCGv out_val, > + target_ulong reg_mask) { > + TCGv set_bits = tcg_temp_new(); > + TCGv cleared_bits = tcg_temp_new(); > + > + /* > + * set_bits = in_val & reg_mask > + * cleared_bits = (~in_val) & reg_mask > + */ > + tcg_gen_andi_tl(set_bits, in_val, reg_mask); > + tcg_gen_not_tl(cleared_bits, in_val); > + tcg_gen_andi_tl(cleared_bits, cleared_bits, reg_mask); > + > + /* > + * result = (reg_cur | set_bits) & (~cleared_bits) > + */ > + tcg_gen_not_tl(cleared_bits, cleared_bits); > + tcg_gen_or_tl(set_bits, set_bits, cur_val); > + tcg_gen_and_tl(out_val, set_bits, cleared_bits); This is overly complicated. It should be out = (in & mask) | (cur & ~mask) which is only 3 operations instead of 6: tcg_gen_andi_tl(t1, in_val, reg_mask); tcg_gen_andi_tl(t2, cur_val, ~reg_mask); tcg_gen_or_tl(out_val, t1, t2); I'm surprised about new files for this simple operation. Surely a subroutine within genptr.c would be sufficient. > +static const hexagon_mut_entry gpr_mut_masks[HEX_REG_LAST_VALUE] = { > + [HEX_REG_PC] = {true, 0x00000000}, > + [HEX_REG_GP] = {true, 0xffffffc0}, > + [HEX_REG_USR] = {true, 0x3ecfff3f}, > + [HEX_REG_UTIMERLO] = {true, 0x00000000}, > + [HEX_REG_UTIMERHI] = {true, 0x00000000}, > +}; ... > static inline void gen_log_reg_write(int rnum, TCGv val) > { > - tcg_gen_mov_tl(hex_new_value[rnum], val); > + const hexagon_mut_entry entry = gpr_mut_masks[rnum]; > + if (entry.present) { > + gen_masked_reg_write(hex_gpr[rnum], val, hex_new_value[rnum], > + entry.mask); > + } else { > + tcg_gen_mov_tl(hex_new_value[rnum], val); > + } You could avoid the structure and .present flag by initializing all other entries to UINT32_MAX. E.g. static const target_ulong gpr_mut_masks[HEX_REG_LAST_VALUE] = { [0 ... HEX_REG_LAST_VALUE - 1] = UINT32_MAX, [HEX_REG_PC] = 0 ... }; It might be clearer, and easier to initialize, if you invert the sense of the mask: only set bits that are immutable, so that you get static const target_ulong gpr_immutable_masks[HEX_REG_LAST_VALUE] = { [HEX_REG_PC] = UINT32_MAX, [HEX_REG_GP] = 0x3f, ... }; r~
> -----Original Message----- > From: Richard Henderson <richard.henderson@linaro.org> ... > It might be clearer, and easier to initialize, if you invert the sense of the mask: Ok -- thanks for the suggestions! I'll give 'em all a try. -Brian
> -----Original Message----- > From: Brian Cain <bcain@quicinc.com> > Sent: Thursday, September 1, 2022 4:30 PM > To: qemu-devel@nongnu.org; Taylor Simpson <tsimpson@quicinc.com> > Cc: Richard Henderson <richard.henderson@linaro.org>; Brian Cain > <bcain@quicinc.com> > Subject: [PATCH] Hexagon (target/hexagon) implement mutability mask for > GPRs > > Some registers are defined to have immutable bits, this commit will > implement that behavior. > > Signed-off-by: Brian Cain <bcain@quicinc.com> > --- > target/hexagon/gen_masked.c | 44 ++++++++++++ > target/hexagon/gen_masked.h | 26 ++++++++ > target/hexagon/genptr.c | 33 ++++++++- > target/hexagon/hex_regs.h | 6 ++ > target/hexagon/meson.build | 1 + > tests/tcg/hexagon/Makefile.target | 1 + > tests/tcg/hexagon/reg_mut.c | 107 > ++++++++++++++++++++++++++++++ > 7 files changed, 217 insertions(+), 1 deletion(-) create mode 100644 > target/hexagon/gen_masked.c create mode 100644 > target/hexagon/gen_masked.h create mode 100644 > tests/tcg/hexagon/reg_mut.c > > diff --git a/target/hexagon/gen_masked.c b/target/hexagon/gen_masked.c > new file mode 100644 index 0000000000..faffee2e13 Run git config diff.orderFile scripts/git.orderFile in your workspace. This will ensure (among other things) that the .h files appear in the patch before the .c files. This makes it easier for the reviewers. > --- /dev/null > +++ b/target/hexagon/gen_masked.c > @@ -0,0 +1,44 @@ > +void gen_masked_reg_write(TCGv cur_val, TCGv in_val, TCGv out_val, > + target_ulong reg_mask) { > + TCGv set_bits = tcg_temp_new(); > + TCGv cleared_bits = tcg_temp_new(); > + > + /* > + * set_bits = in_val & reg_mask > + * cleared_bits = (~in_val) & reg_mask > + */ > + tcg_gen_andi_tl(set_bits, in_val, reg_mask); > + tcg_gen_not_tl(cleared_bits, in_val); > + tcg_gen_andi_tl(cleared_bits, cleared_bits, reg_mask); > + > + /* > + * result = (reg_cur | set_bits) & (~cleared_bits) > + */ > + tcg_gen_not_tl(cleared_bits, cleared_bits); > + tcg_gen_or_tl(set_bits, set_bits, cur_val); > + tcg_gen_and_tl(out_val, set_bits, cleared_bits); > + > + tcg_temp_free(set_bits); > + tcg_temp_free(cleared_bits); > +} In addition to Richard's feedback, you should do nothing when reg_mask is zero. > diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c index > 8a334ba07b..21385f556e 100644 > --- a/target/hexagon/genptr.c > +++ b/target/hexagon/genptr.c > static inline void gen_log_reg_write(int rnum, TCGv val) { > - tcg_gen_mov_tl(hex_new_value[rnum], val); > + const hexagon_mut_entry entry = gpr_mut_masks[rnum]; > + if (entry.present) { > + gen_masked_reg_write(hex_gpr[rnum], val, hex_new_value[rnum], You can't write to hex_gpr here. You have to wait to make sure the packet will commit. Put this result back into val and do the mov to hex_new_value unconditionally. > + entry.mask); > + } else { > + tcg_gen_mov_tl(hex_new_value[rnum], val); > + } > static void gen_log_predicated_reg_write_pair(int rnum, TCGv_i64 val, int > slot) { > + const hexagon_mut_entry entry0 = gpr_mut_masks[rnum]; > + const hexagon_mut_entry entry1 = gpr_mut_masks[rnum + 1]; > TCGv val32 = tcg_temp_new(); > TCGv zero = tcg_constant_tl(0); > TCGv slot_mask = tcg_temp_new(); > + TCGv tmp_val = tcg_temp_new(); > > tcg_gen_andi_tl(slot_mask, hex_slot_cancelled, 1 << slot); > + > /* Low word */ > tcg_gen_extrl_i64_i32(val32, val); > + if (entry0.present) { > + tcg_gen_mov_tl(tmp_val, val32); > + gen_masked_reg_write(hex_gpr[rnum], tmp_val, val32, entry0.mask); See previous comment. Put the result back into val32 and let the subsequent code put it into hex_new_value if the slot isn't cancelled. > + tcg_temp_free(tmp_val); > + } > tcg_gen_movcond_tl(TCG_COND_EQ, hex_new_value[rnum], > slot_mask, zero, > val32, hex_new_value[rnum]); > + > /* High word */ > tcg_gen_extrh_i64_i32(val32, val); > + if (entry1.present) { > + tcg_gen_mov_tl(tmp_val, val32); > + gen_masked_reg_write(hex_gpr[rnum], tmp_val, val32, entry1.mask); Ditto. > + } > a/target/hexagon/hex_regs.h b/target/hexagon/hex_regs.h index > a63c2c0fd5..db48cff96e 100644 > --- a/target/hexagon/hex_regs.h > +++ b/target/hexagon/hex_regs.h > @@ -79,6 +79,12 @@ enum { > HEX_REG_QEMU_HVX_CNT = 54, > HEX_REG_UTIMERLO = 62, > HEX_REG_UTIMERHI = 63, > + HEX_REG_LAST_VALUE = 64, You can use TOTAL_PER_THREAD_REGS (defined in cpu.h). > }; > diff --git a/tests/tcg/hexagon/reg_mut.c b/tests/tcg/hexagon/reg_mut.c > new file mode 100644 index 0000000000..7e81ec6bf3 > --- /dev/null > +++ b/tests/tcg/hexagon/reg_mut.c > @@ -0,0 +1,107 @@ > +#define READ_REG(reg_name, out_reg) \ > + asm volatile ("%0 = " reg_name "\n\t" \ > + : "=r"(out_reg) \ > + : \ > + : \ > + ); \ > + Remove this - it isn't used. > +int main() > +{ > + check(set_usr(0x00), 0x00); > + check(set_usr(0xffffffff), 0x3ecfff3f); > + check(set_usr(0x00), 0x00); > + check(set_usr(0x01), 0x01); > + check(set_usr(0xff), 0x3f); > + > + /* > + * PC is special. Setting it to these values > + * should cause an instruction fetch miss. Why would there be an instruction fetch miss? The gpr_mut_masks[HEX_REG_PC] is zero, so this instruction won't change PC. > + */ > + check_ne(set_pc(0x00000000), 0x00000000); > + check_ne(set_pc(0xffffffff), 0xffffffff); > + check_ne(set_pc(0x00000001), 0x00000001); > + check_ne(set_pc(0x000000ff), 0x000000ff); > + > + puts(err ? "FAIL" : "PASS"); > + return err; > +} Add some tests that do the assignment inside a packet and using a register pair assignment. Thanks, Taylor
On 9/6/22 23:26, Taylor Simpson wrote: >> diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c index >> 8a334ba07b..21385f556e 100644 >> --- a/target/hexagon/genptr.c >> +++ b/target/hexagon/genptr.c >> static inline void gen_log_reg_write(int rnum, TCGv val) { >> - tcg_gen_mov_tl(hex_new_value[rnum], val); >> + const hexagon_mut_entry entry = gpr_mut_masks[rnum]; >> + if (entry.present) { >> + gen_masked_reg_write(hex_gpr[rnum], val, hex_new_value[rnum], > > You can't write to hex_gpr here. You have to wait to make sure the packet will commit. Put this result back into val and do the mov to hex_new_value unconditionally. The feedback, then, is that the operands are confusingly ordered -- the output is to hex_new_value. Brian, tcg functions generally list outputs first. r~
> -----Original Message----- > From: Qemu-devel <qemu-devel-bounces+bcain=quicinc.com@nongnu.org> > On Behalf Of Anton Johansson via ... > Hi, Brian! > > I've taken a look and most of this patch seems good, however I have a few > comments/observations. Anton, sorry I missed this message last week, only following up now. > > Some registers are defined to have immutable bits, this commit > > will implement that behavior. > > > > Signed-off-by: Brian Cain <bcain@quicinc.com> > > --- > > target/hexagon/gen_masked.c | 44 ++++++++++++ > > target/hexagon/gen_masked.h | 26 ++++++++ > > target/hexagon/genptr.c | 33 ++++++++- > > target/hexagon/hex_regs.h | 6 ++ > > target/hexagon/meson.build | 1 + > > tests/tcg/hexagon/Makefile.target | 1 + > > tests/tcg/hexagon/reg_mut.c | 107 > ++++++++++++++++++++++++++++++ > > 7 files changed, 217 insertions(+), 1 deletion(-) > > create mode 100644 target/hexagon/gen_masked.c > > create mode 100644 target/hexagon/gen_masked.h > > create mode 100644 tests/tcg/hexagon/reg_mut.c > > > > diff --git a/target/hexagon/gen_masked.c b/target/hexagon/gen_masked.c > > new file mode 100644 > > index 0000000000..faffee2e13 > > --- /dev/null > > +++ b/target/hexagon/gen_masked.c > > @@ -0,0 +1,44 @@ > > +/* > > + * Copyright(c) 2022 Qualcomm Innovation Center, Inc. All Rights Reserved. > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License as published by > > + * the Free Software Foundation; either version 2 of the License, or > > + * (at your option) any later version. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + * > > + * You should have received a copy of the GNU General Public License > > + * along with this program; if not, see <http://www.gnu.org/licenses/>. > > + */ > > + > > +#include "qemu/osdep.h" > > +#include "tcg/tcg-op.h" > > +#include "gen_masked.h" > > + > > +void gen_masked_reg_write(TCGv cur_val, TCGv in_val, TCGv out_val, > > + target_ulong reg_mask) { > > Brace on new line per coding standard. Also I would line up the > indentation with Hmm - odd, I could have sworn checkpatch gave this a green light. ☹ I will fix it. > the rest of the arguments to match the rest of the hexagon frontend. I would > suggest putting output arguments first to match other TCG ops, could become > confusing otherwise, so > > void gen_masked_reg_write(TCGv out_val, TCGv in_val, TCGv cur_val, > target_ulong reg_mask) > { > > > + TCGv set_bits = tcg_temp_new(); > > + TCGv cleared_bits = tcg_temp_new(); > > + > > + /* > > + * set_bits = in_val & reg_mask > > + * cleared_bits = (~in_val) & reg_mask > > + */ > > + tcg_gen_andi_tl(set_bits, in_val, reg_mask); > > + tcg_gen_not_tl(cleared_bits, in_val); > > + tcg_gen_andi_tl(cleared_bits, cleared_bits, reg_mask); > > + > > + /* > > + * result = (reg_cur | set_bits) & (~cleared_bits) > > + */ > > + tcg_gen_not_tl(cleared_bits, cleared_bits); > > + tcg_gen_or_tl(set_bits, set_bits, cur_val); > > + tcg_gen_and_tl(out_val, set_bits, cleared_bits); > > + > > + tcg_temp_free(set_bits); > > + tcg_temp_free(cleared_bits); > > +} > > You could cut down on a single not operation in this function since > > ~cleared_bits = ~((~in_val) & reg_mask) > = in_val | (~reg_mask) > > I looked at the output of qemu-hexagon -d op_opt and this operation > is not performed by the TCG optimizer, so we would end up saving > an instruction. IIUC Richard's suggestion reduces it yet further, so I'll use that algorithm instead. > > diff --git a/target/hexagon/gen_masked.h b/target/hexagon/gen_masked.h > > new file mode 100644 > > index 0000000000..71f4b2818b > > --- /dev/null > > +++ b/target/hexagon/gen_masked.h > > @@ -0,0 +1,26 @@ > > +/* > > + * Copyright(c) 2022 Qualcomm Innovation Center, Inc. All Rights Reserved. > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License as published by > > + * the Free Software Foundation; either version 2 of the License, or > > + * (at your option) any later version. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + * > > + * You should have received a copy of the GNU General Public License > > + * along with this program; if not, see <http://www.gnu.org/licenses/>. > > + */ > > + > > +#ifndef GEN_MASKED_H > > +#define GEN_MASKED_H > > + > > +#include "tcg/tcg-op.h" > > + > > +void gen_masked_reg_write(TCGv cur_val, TCGv in_val, TCGv out_val, > > + target_ulong reg_mask); > > + > > +#endif /* GEN_MASKED_H */ > > diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c > > index 8a334ba07b..21385f556e 100644 > > --- a/target/hexagon/genptr.c > > +++ b/target/hexagon/genptr.c > > @@ -29,6 +29,7 @@ > > #undef QEMU_GENERATE > > #include "gen_tcg.h" > > #include "gen_tcg_hvx.h" > > +#include "gen_masked.h" > > > > static inline void gen_log_predicated_reg_write(int rnum, TCGv val, int slot) > > { > > @@ -53,9 +54,24 @@ static inline void gen_log_predicated_reg_write(int > rnum, TCGv val, int slot) > > tcg_temp_free(slot_mask); > > } > > > > +static const hexagon_mut_entry gpr_mut_masks[HEX_REG_LAST_VALUE] = > { > > + [HEX_REG_PC] = {true, 0x00000000}, > > + [HEX_REG_GP] = {true, 0xffffffc0}, > > + [HEX_REG_USR] = {true, 0x3ecfff3f}, > > + [HEX_REG_UTIMERLO] = {true, 0x00000000}, > > + [HEX_REG_UTIMERHI] = {true, 0x00000000}, > > +}; > > + > > + > > Remove extra newline. > > Also I notice > > gen_log_reg_write > gen_log_predicated_reg_write_pair > > now call gen_masked_reg_write, is there any reason why > > gen_log_reg_write_pair > gen_log_predicated_reg_write > > have been excluded? Urk - that seems like an error. I will look closer and probably end up including it in both of those. > > static inline void gen_log_reg_write(int rnum, TCGv val) > > { > > - tcg_gen_mov_tl(hex_new_value[rnum], val); > > + const hexagon_mut_entry entry = gpr_mut_masks[rnum]; > > + if (entry.present) { > > + gen_masked_reg_write(hex_gpr[rnum], val, hex_new_value[rnum], > > + entry.mask); > > Line up entry.mask); with rest of arguments. Richard's suggestion to remove the structure will obsolete this alignment issue. > > + } else { > > + tcg_gen_mov_tl(hex_new_value[rnum], val); > > + } > > if (HEX_DEBUG) { > > /* Do this so HELPER(debug_commit_end) will know */ > > tcg_gen_movi_tl(hex_reg_written[rnum], 1); > > @@ -64,18 +80,32 @@ static inline void gen_log_reg_write(int rnum, TCGv > val) > > > > static void gen_log_predicated_reg_write_pair(int rnum, TCGv_i64 val, int > slot) > > { > > + const hexagon_mut_entry entry0 = gpr_mut_masks[rnum]; > > + const hexagon_mut_entry entry1 = gpr_mut_masks[rnum + 1]; > > TCGv val32 = tcg_temp_new(); > > TCGv zero = tcg_constant_tl(0); > > TCGv slot_mask = tcg_temp_new(); > > + TCGv tmp_val = tcg_temp_new(); > > > > tcg_gen_andi_tl(slot_mask, hex_slot_cancelled, 1 << slot); > > + > > /* Low word */ > > tcg_gen_extrl_i64_i32(val32, val); > > + if (entry0.present) { > > + tcg_gen_mov_tl(tmp_val, val32); > > + gen_masked_reg_write(hex_gpr[rnum], tmp_val, val32, entry0.mask); > > + tcg_temp_free(tmp_val); > > There's a double free of tmp_val here. Even better would be to get rid of > tmp_val, and instead use > > gen_masked_reg_write(hex_gpr[rnum], val32, val32, entry0.mask); > > > > + } > > tcg_gen_movcond_tl(TCG_COND_EQ, hex_new_value[rnum], > > slot_mask, zero, > > val32, hex_new_value[rnum]); > > + > > /* High word */ > > tcg_gen_extrh_i64_i32(val32, val); > > + if (entry1.present) { > > + tcg_gen_mov_tl(tmp_val, val32); > > + gen_masked_reg_write(hex_gpr[rnum], tmp_val, val32, entry1.mask); > > Same applies here, should be able to get rid of tmp_val, also shouldn't it > be hex_gpr[rnum+1] in the call to gen_masked_reg_write? Hmm - good catch. Underscores the need for regpair test cases, like Taylor's suggestion. > > + } > > tcg_gen_movcond_tl(TCG_COND_EQ, hex_new_value[rnum + 1], > > slot_mask, zero, > > val32, hex_new_value[rnum + 1]); > > @@ -95,6 +125,7 @@ static void gen_log_predicated_reg_write_pair(int > rnum, TCGv_i64 val, int slot) > > > > tcg_temp_free(val32); > > tcg_temp_free(slot_mask); > > + tcg_temp_free(tmp_val); > > } > > > > static void gen_log_reg_write_pair(int rnum, TCGv_i64 val) > > diff --git a/target/hexagon/hex_regs.h b/target/hexagon/hex_regs.h > > index a63c2c0fd5..db48cff96e 100644 > > --- a/target/hexagon/hex_regs.h > > +++ b/target/hexagon/hex_regs.h > > @@ -79,6 +79,12 @@ enum { > > HEX_REG_QEMU_HVX_CNT = 54, > > HEX_REG_UTIMERLO = 62, > > HEX_REG_UTIMERHI = 63, > > + HEX_REG_LAST_VALUE = 64, > > }; > > > > + > > +typedef struct { > > + bool present; > > + target_ulong mask; > > +} hexagon_mut_entry; > > struct names should be CamelCase. > > > > #endif > > diff --git a/target/hexagon/meson.build b/target/hexagon/meson.build > > index b61243103f..ea1f66982a 100644 > > --- a/target/hexagon/meson.build > > +++ b/target/hexagon/meson.build > > @@ -168,6 +168,7 @@ hexagon_ss.add(files( > > 'op_helper.c', > > 'gdbstub.c', > > 'genptr.c', > > + 'gen_masked.c', > > 'reg_fields.c', > > 'decode.c', > > 'iclass.c', > > diff --git a/tests/tcg/hexagon/Makefile.target > b/tests/tcg/hexagon/Makefile.target > > index 96a4d7a614..385d787a00 100644 > > --- a/tests/tcg/hexagon/Makefile.target > > +++ b/tests/tcg/hexagon/Makefile.target > > @@ -43,6 +43,7 @@ HEX_TESTS += load_align > > HEX_TESTS += atomics > > HEX_TESTS += fpstuff > > HEX_TESTS += overflow > > +HEX_TESTS += reg_mut > > > > TESTS += $(HEX_TESTS) > > > > diff --git a/tests/tcg/hexagon/reg_mut.c b/tests/tcg/hexagon/reg_mut.c > > new file mode 100644 > > index 0000000000..7e81ec6bf3 > > --- /dev/null > > +++ b/tests/tcg/hexagon/reg_mut.c > > @@ -0,0 +1,107 @@ > > +/* > > + * Copyright(c) 2022 Qualcomm Innovation Center, Inc. All Rights Reserved. > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License as published by > > + * the Free Software Foundation; either version 2 of the License, or > > + * (at your option) any later version. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + * > > + * You should have received a copy of the GNU General Public License > > + * along with this program; if not, see <http://www.gnu.org/licenses/>. > > + */ > > + > > +#include <stdio.h> > > +#include <stdint.h> > > + > > +int err; > > + > > +#define check_ne(N, EXPECT) \ > > + { uint32_t value = N; \ > > + if (value == EXPECT) { \ > > + printf("ERROR: \"%s\" 0x%04x == 0x%04x at %s:%d\n", #N, value, \ > > + EXPECT, __FILE__, __LINE__); \ > > + err++; \ > > + } \ > > + } > > + > > +#define check(N, EXPECT) \ > > + { uint32_t value = N; \ > > + if (value != EXPECT) { \ > > + printf("ERROR: \"%s\" 0x%04x != 0x%04x at %s:%d\n", #N, value, \ > > + EXPECT, __FILE__, __LINE__); \ > > + err++; \ > > + } \ > > + } > > + > > Wrap these two macros in do {...} while(0) instead > > > > +#define READ_REG(reg_name, out_reg) \ > > + asm volatile ("%0 = " reg_name "\n\t" \ > > + : "=r"(out_reg) \ > > + : \ > > + : \ > > + ); \ > > + > > +#define WRITE_REG(reg_name, out_reg, in_reg) \ > > + asm volatile (reg_name " = %1\n\t" \ > > + "%0 = " reg_name "\n\t" \ > > + : "=r"(out_reg) \ > > + : "r"(in_reg) \ > > + : reg_name \ > > + ); \ > > 3 minor nitpicks, use 4-space indents for asm volatile lines, remove the > trailing ; \ at the end of the macros, and READ_REG is unused. Ok: I will fix these. > > > + > > + /* > > + * Instruction word: { pc = r0 } > > + * > > + * This instruction is barred by the assembler. > > + * > > + * 3 2 1 > > + * 1 0 9 8 7 6 5 4 3 2 1 0 9 8 7 6 5 4 3 2 1 0 9 8 7 6 5 4 3 2 1 0 > > + * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ > > + * | Opc[A2_tfrrcr] | Src[R0] |P P| | C9/PC | > > + * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ > > + */ > > Very nice ascii art! > > > > +#define PC_EQ_R0 ".word 0x6220c009\n\t" > > + > > +static inline uint32_t set_pc(uint32_t in_reg) > > +{ > > + uint32_t out_reg; > > + asm volatile("r0 = %1\n\t" > > + PC_EQ_R0 > > + "%0 = pc\n\t" > > + : "=r"(out_reg) > > + : "r"(in_reg) > > + : "r0"); > > + return out_reg; > > +} > > + > > +static inline uint32_t set_usr(uint32_t in_reg) > > +{ > > + uint32_t out_reg; > > + WRITE_REG("usr", out_reg, in_reg); > > + return out_reg; > > +} > > + > > +int main() > > +{ > > + check(set_usr(0x00), 0x00); > > + check(set_usr(0xffffffff), 0x3ecfff3f); > > + check(set_usr(0x00), 0x00); > > + check(set_usr(0x01), 0x01); > > + check(set_usr(0xff), 0x3f); > > + > > + /* > > + * PC is special. Setting it to these values > > + * should cause an instruction fetch miss. > > + */ > > + check_ne(set_pc(0x00000000), 0x00000000); > > + check_ne(set_pc(0xffffffff), 0xffffffff); > > + check_ne(set_pc(0x00000001), 0x00000001); > > + check_ne(set_pc(0x000000ff), 0x000000ff); > > + > > + puts(err ? "FAIL" : "PASS"); > > + return err; > > +} > > -- > Anton Johansson, > rev.ng Labs Srl. >
diff --git a/target/hexagon/gen_masked.c b/target/hexagon/gen_masked.c new file mode 100644 index 0000000000..faffee2e13 --- /dev/null +++ b/target/hexagon/gen_masked.c @@ -0,0 +1,44 @@ +/* + * Copyright(c) 2022 Qualcomm Innovation Center, Inc. All Rights Reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, see <http://www.gnu.org/licenses/>. + */ + +#include "qemu/osdep.h" +#include "tcg/tcg-op.h" +#include "gen_masked.h" + +void gen_masked_reg_write(TCGv cur_val, TCGv in_val, TCGv out_val, + target_ulong reg_mask) { + TCGv set_bits = tcg_temp_new(); + TCGv cleared_bits = tcg_temp_new(); + + /* + * set_bits = in_val & reg_mask + * cleared_bits = (~in_val) & reg_mask + */ + tcg_gen_andi_tl(set_bits, in_val, reg_mask); + tcg_gen_not_tl(cleared_bits, in_val); + tcg_gen_andi_tl(cleared_bits, cleared_bits, reg_mask); + + /* + * result = (reg_cur | set_bits) & (~cleared_bits) + */ + tcg_gen_not_tl(cleared_bits, cleared_bits); + tcg_gen_or_tl(set_bits, set_bits, cur_val); + tcg_gen_and_tl(out_val, set_bits, cleared_bits); + + tcg_temp_free(set_bits); + tcg_temp_free(cleared_bits); +} diff --git a/target/hexagon/gen_masked.h b/target/hexagon/gen_masked.h new file mode 100644 index 0000000000..71f4b2818b --- /dev/null +++ b/target/hexagon/gen_masked.h @@ -0,0 +1,26 @@ +/* + * Copyright(c) 2022 Qualcomm Innovation Center, Inc. All Rights Reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, see <http://www.gnu.org/licenses/>. + */ + +#ifndef GEN_MASKED_H +#define GEN_MASKED_H + +#include "tcg/tcg-op.h" + +void gen_masked_reg_write(TCGv cur_val, TCGv in_val, TCGv out_val, + target_ulong reg_mask); + +#endif /* GEN_MASKED_H */ diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c index 8a334ba07b..21385f556e 100644 --- a/target/hexagon/genptr.c +++ b/target/hexagon/genptr.c @@ -29,6 +29,7 @@ #undef QEMU_GENERATE #include "gen_tcg.h" #include "gen_tcg_hvx.h" +#include "gen_masked.h" static inline void gen_log_predicated_reg_write(int rnum, TCGv val, int slot) { @@ -53,9 +54,24 @@ static inline void gen_log_predicated_reg_write(int rnum, TCGv val, int slot) tcg_temp_free(slot_mask); } +static const hexagon_mut_entry gpr_mut_masks[HEX_REG_LAST_VALUE] = { + [HEX_REG_PC] = {true, 0x00000000}, + [HEX_REG_GP] = {true, 0xffffffc0}, + [HEX_REG_USR] = {true, 0x3ecfff3f}, + [HEX_REG_UTIMERLO] = {true, 0x00000000}, + [HEX_REG_UTIMERHI] = {true, 0x00000000}, +}; + + static inline void gen_log_reg_write(int rnum, TCGv val) { - tcg_gen_mov_tl(hex_new_value[rnum], val); + const hexagon_mut_entry entry = gpr_mut_masks[rnum]; + if (entry.present) { + gen_masked_reg_write(hex_gpr[rnum], val, hex_new_value[rnum], + entry.mask); + } else { + tcg_gen_mov_tl(hex_new_value[rnum], val); + } if (HEX_DEBUG) { /* Do this so HELPER(debug_commit_end) will know */ tcg_gen_movi_tl(hex_reg_written[rnum], 1); @@ -64,18 +80,32 @@ static inline void gen_log_reg_write(int rnum, TCGv val) static void gen_log_predicated_reg_write_pair(int rnum, TCGv_i64 val, int slot) { + const hexagon_mut_entry entry0 = gpr_mut_masks[rnum]; + const hexagon_mut_entry entry1 = gpr_mut_masks[rnum + 1]; TCGv val32 = tcg_temp_new(); TCGv zero = tcg_constant_tl(0); TCGv slot_mask = tcg_temp_new(); + TCGv tmp_val = tcg_temp_new(); tcg_gen_andi_tl(slot_mask, hex_slot_cancelled, 1 << slot); + /* Low word */ tcg_gen_extrl_i64_i32(val32, val); + if (entry0.present) { + tcg_gen_mov_tl(tmp_val, val32); + gen_masked_reg_write(hex_gpr[rnum], tmp_val, val32, entry0.mask); + tcg_temp_free(tmp_val); + } tcg_gen_movcond_tl(TCG_COND_EQ, hex_new_value[rnum], slot_mask, zero, val32, hex_new_value[rnum]); + /* High word */ tcg_gen_extrh_i64_i32(val32, val); + if (entry1.present) { + tcg_gen_mov_tl(tmp_val, val32); + gen_masked_reg_write(hex_gpr[rnum], tmp_val, val32, entry1.mask); + } tcg_gen_movcond_tl(TCG_COND_EQ, hex_new_value[rnum + 1], slot_mask, zero, val32, hex_new_value[rnum + 1]); @@ -95,6 +125,7 @@ static void gen_log_predicated_reg_write_pair(int rnum, TCGv_i64 val, int slot) tcg_temp_free(val32); tcg_temp_free(slot_mask); + tcg_temp_free(tmp_val); } static void gen_log_reg_write_pair(int rnum, TCGv_i64 val) diff --git a/target/hexagon/hex_regs.h b/target/hexagon/hex_regs.h index a63c2c0fd5..db48cff96e 100644 --- a/target/hexagon/hex_regs.h +++ b/target/hexagon/hex_regs.h @@ -79,6 +79,12 @@ enum { HEX_REG_QEMU_HVX_CNT = 54, HEX_REG_UTIMERLO = 62, HEX_REG_UTIMERHI = 63, + HEX_REG_LAST_VALUE = 64, }; + +typedef struct { + bool present; + target_ulong mask; +} hexagon_mut_entry; #endif diff --git a/target/hexagon/meson.build b/target/hexagon/meson.build index b61243103f..ea1f66982a 100644 --- a/target/hexagon/meson.build +++ b/target/hexagon/meson.build @@ -168,6 +168,7 @@ hexagon_ss.add(files( 'op_helper.c', 'gdbstub.c', 'genptr.c', + 'gen_masked.c', 'reg_fields.c', 'decode.c', 'iclass.c', diff --git a/tests/tcg/hexagon/Makefile.target b/tests/tcg/hexagon/Makefile.target index 96a4d7a614..385d787a00 100644 --- a/tests/tcg/hexagon/Makefile.target +++ b/tests/tcg/hexagon/Makefile.target @@ -43,6 +43,7 @@ HEX_TESTS += load_align HEX_TESTS += atomics HEX_TESTS += fpstuff HEX_TESTS += overflow +HEX_TESTS += reg_mut TESTS += $(HEX_TESTS) diff --git a/tests/tcg/hexagon/reg_mut.c b/tests/tcg/hexagon/reg_mut.c new file mode 100644 index 0000000000..7e81ec6bf3 --- /dev/null +++ b/tests/tcg/hexagon/reg_mut.c @@ -0,0 +1,107 @@ +/* + * Copyright(c) 2022 Qualcomm Innovation Center, Inc. All Rights Reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, see <http://www.gnu.org/licenses/>. + */ + +#include <stdio.h> +#include <stdint.h> + +int err; + +#define check_ne(N, EXPECT) \ + { uint32_t value = N; \ + if (value == EXPECT) { \ + printf("ERROR: \"%s\" 0x%04x == 0x%04x at %s:%d\n", #N, value, \ + EXPECT, __FILE__, __LINE__); \ + err++; \ + } \ + } + +#define check(N, EXPECT) \ + { uint32_t value = N; \ + if (value != EXPECT) { \ + printf("ERROR: \"%s\" 0x%04x != 0x%04x at %s:%d\n", #N, value, \ + EXPECT, __FILE__, __LINE__); \ + err++; \ + } \ + } + +#define READ_REG(reg_name, out_reg) \ + asm volatile ("%0 = " reg_name "\n\t" \ + : "=r"(out_reg) \ + : \ + : \ + ); \ + +#define WRITE_REG(reg_name, out_reg, in_reg) \ + asm volatile (reg_name " = %1\n\t" \ + "%0 = " reg_name "\n\t" \ + : "=r"(out_reg) \ + : "r"(in_reg) \ + : reg_name \ + ); \ + + /* + * Instruction word: { pc = r0 } + * + * This instruction is barred by the assembler. + * + * 3 2 1 + * 1 0 9 8 7 6 5 4 3 2 1 0 9 8 7 6 5 4 3 2 1 0 9 8 7 6 5 4 3 2 1 0 + * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ + * | Opc[A2_tfrrcr] | Src[R0] |P P| | C9/PC | + * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ + */ +#define PC_EQ_R0 ".word 0x6220c009\n\t" + +static inline uint32_t set_pc(uint32_t in_reg) +{ + uint32_t out_reg; + asm volatile("r0 = %1\n\t" + PC_EQ_R0 + "%0 = pc\n\t" + : "=r"(out_reg) + : "r"(in_reg) + : "r0"); + return out_reg; +} + +static inline uint32_t set_usr(uint32_t in_reg) +{ + uint32_t out_reg; + WRITE_REG("usr", out_reg, in_reg); + return out_reg; +} + +int main() +{ + check(set_usr(0x00), 0x00); + check(set_usr(0xffffffff), 0x3ecfff3f); + check(set_usr(0x00), 0x00); + check(set_usr(0x01), 0x01); + check(set_usr(0xff), 0x3f); + + /* + * PC is special. Setting it to these values + * should cause an instruction fetch miss. + */ + check_ne(set_pc(0x00000000), 0x00000000); + check_ne(set_pc(0xffffffff), 0xffffffff); + check_ne(set_pc(0x00000001), 0x00000001); + check_ne(set_pc(0x000000ff), 0x000000ff); + + puts(err ? "FAIL" : "PASS"); + return err; +}
Some registers are defined to have immutable bits, this commit will implement that behavior. Signed-off-by: Brian Cain <bcain@quicinc.com> --- target/hexagon/gen_masked.c | 44 ++++++++++++ target/hexagon/gen_masked.h | 26 ++++++++ target/hexagon/genptr.c | 33 ++++++++- target/hexagon/hex_regs.h | 6 ++ target/hexagon/meson.build | 1 + tests/tcg/hexagon/Makefile.target | 1 + tests/tcg/hexagon/reg_mut.c | 107 ++++++++++++++++++++++++++++++ 7 files changed, 217 insertions(+), 1 deletion(-) create mode 100644 target/hexagon/gen_masked.c create mode 100644 target/hexagon/gen_masked.h create mode 100644 tests/tcg/hexagon/reg_mut.c