[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
RE: [PATCH v2] Hexagon (target/hexagon) implement mutability mask for GP
|
From: |
Taylor Simpson |
|
Subject: |
RE: [PATCH v2] Hexagon (target/hexagon) implement mutability mask for GPRs |
|
Date: |
Mon, 19 Dec 2022 18:23:50 +0000 |
> -----Original Message-----
> From: Marco Liebel (QUIC) <quic_mliebel@quicinc.com>
> Sent: Friday, December 16, 2022 2:04 PM
> To: qemu-devel@nongnu.org
> Cc: Brian Cain <bcain@quicinc.com>; Taylor Simpson
> <tsimpson@quicinc.com>; Marco Liebel (QUIC) <quic_mliebel@quicinc.com>
> Subject: [PATCH v2] 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: Marco Liebel <quic_mliebel@quicinc.com>
> ---
> This incorporates the feedback given on Brian's patch
>
> target/hexagon/genptr.c | 43 ++++-
> tests/tcg/hexagon/Makefile.target | 3 +
> tests/tcg/hexagon/reg_mut.c | 307
> ++++++++++++++++++++++++++++++
> 3 files changed, 351 insertions(+), 2 deletions(-) create mode 100644
> tests/tcg/hexagon/reg_mut.c
>
> diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c index
> 806d0974ff..a91db8c76d 100644
> --- a/target/hexagon/genptr.c
> +++ b/target/hexagon/genptr.c
> @@ -30,6 +30,32 @@
> #include "gen_tcg.h"
> #include "gen_tcg_hvx.h"
>
> +static const target_ulong reg_immut_masks[TOTAL_PER_THREAD_REGS] =
> {
> + [HEX_REG_USR] = 0xc13000c0,
> + [HEX_REG_PC] = UINT32_MAX,
> + [HEX_REG_GP] = 0x3f,
> + [HEX_REG_UPCYCLELO] = UINT32_MAX,
> + [HEX_REG_UPCYCLEHI] = UINT32_MAX,
> + [HEX_REG_UTIMERLO] = UINT32_MAX,
> + [HEX_REG_UTIMERHI] = UINT32_MAX,
> +};
UINT_MAX is confusing. Use ~0 instead.
#define IMMUTABLE (~0)
...
[HEX_REG_PC] = IMMUTABLE
...
> +
> +static inline void gen_masked_reg_write(TCGv result, TCGv new_val, TCGv
> cur_val,
> + target_ulong reg_mask) {
> + if (reg_mask) {
> + TCGv tmp = tcg_temp_new();
> +
> + /* out_val = (in_val & reg_mask) | (cur_val & ~reg_mask) */
Comment doesn't match the TCG code below. Should be
/* result = (new_val & ~reg_mask) | (cur_val & reg_mask) */
> + /* result is used to avoid creating a second temporary variable */
> + tcg_gen_andi_tl(result, new_val, ~reg_mask);
> + tcg_gen_andi_tl(tmp, cur_val, reg_mask);
> + tcg_gen_or_tl(result, result, tmp);
> +
> + tcg_temp_free(tmp);
> + }
> +}
All of the callers of this function pass the same TCGv for result and new_val.
So, we are getting lucky that this function doesn't set result when reg_mask is
zero. In order to future-proof this function, either eliminate the result
parameter or assign to it from new_val when reg_mask is zero.
> +
> static inline void gen_log_predicated_reg_write(int rnum, TCGv val, int slot)
> {
> TCGv zero = tcg_constant_tl(0);
> diff --git a/tests/tcg/hexagon/Makefile.target
> b/tests/tcg/hexagon/Makefile.target
> index 96a4d7a614..4e9a20960b 100644
> --- a/tests/tcg/hexagon/Makefile.target
> +++ b/tests/tcg/hexagon/Makefile.target
> @@ -43,9 +43,12 @@ HEX_TESTS += load_align HEX_TESTS += atomics
> HEX_TESTS += fpstuff HEX_TESTS += overflow
> +HEX_TESTS += reg_mut
>
> TESTS += $(HEX_TESTS)
>
> +reg_mut: CFLAGS += -I$(SRC_PATH)/target/hexagon/
> +
You won't need this when you remove hex_regs.h from the test (see below).
> # This test has to be compiled for the -mv67t target
> usr: usr.c
> $(CC) $(CFLAGS) -mv67t -O2 -Wno-inline-asm -Wno-expansion-to-
> defined $< -o $@ $(LDFLAGS) diff --git a/tests/tcg/hexagon/reg_mut.c
> b/tests/tcg/hexagon/reg_mut.c new file mode 100644 index
> 0000000000..8bbc1559dd
> --- /dev/null
> +++ b/tests/tcg/hexagon/reg_mut.c
> @@ -0,0 +1,307 @@
> +
> +/*
> + * 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>
> +
> +#include "hex_regs.h"
Don't include headers from QEMU itself in the test cases.
You don't need this because you are just converting from the enum to a string.
Just use the string itself and skip the switch statement.
> +
> +static int err;
> +
> +enum {
> + HEX_REG_PAIR_C9_8,
> + HEX_REG_PAIR_C11_10,
> + HEX_REG_PAIR_C15_14,
> + HEX_REG_PAIR_C31_30,
> +};
Shouldn't need this if you switch to using strings for the register names.
> +#define WRITE_REG(reg_name, output, input) \
> + asm volatile(reg_name " = %1\n\t" \
> + "%0 = " reg_name "\n\t" \
> + : "=r"(output) \
> + : "r"(input) \
> + : reg_name);
> +
> +#define WRITE_REG_IN_PACKET(reg_name, output, input) \
> + asm volatile("{ " reg_name " = %1 }\n\t" \
This is no different from the WRITE_REG above. Instructions on a line with no
curly braces are a single packet.
> + "%0 = " reg_name "\n\t" \
> + : "=r"(output) \
> + : "r"(input) \
> + : reg_name);
> +
> +#define WRITE_REG_ENCODED(reg_name, encoding, output, input) \
> + asm volatile("r0 = %1\n\t" \
> + encoding \
> + "%0 = " reg_name "\n\t" \
> + : "=r"(output) \
> + : "r"(input) \
> + : "r0");
> +
> +#define WRITE_REG_ENCODED_IN_PACKET(reg_name, encoding, output,
> input) \
> + asm volatile("{ r0 = %1 }\n\t" \
This is also the same as WRITE_REG_ENCODED.
> + encoding \
> + "%0 = " reg_name "\n\t" \
> + : "=r"(output) \
> + : "r"(input) \
> + : "r0");
> +
> +#define WRITE_REG_PAIR_ENCODED(reg_name, encoding, output, input) \
> + asm volatile("r1:0 = %1\n\t" \
> + encoding \
> + "%0 = " reg_name "\n\t" \
> + : "=r"(output) \
> + : "r"(input) \
> + : "r1:0");
> +
> +#define WRITE_REG_PAIR_ENCODED_IN_PACKET(reg_name, encoding,
> output, input) \
> + asm volatile("{ r1:0 = %1 }\n\t" \
> + encoding \
> + "%0 = " reg_name "\n\t" \
> + : "=r"(output) \
> + : "r"(input) \
> + : "r1:0");
> +
> +/*
> + * 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"
> +#define GP_EQ_R0 ".word 0x6220c00b\n\t"
> +#define UPCYCLELO_EQ_R0 ".word 0x6220c00e\n\t"
> +#define UPCYCLEHI_EQ_R0 ".word 0x6220c00f\n\t"
> +#define UTIMERLO_EQ_R0 ".word 0x6220c01e\n\t"
> +#define UTIMERHI_EQ_R0 ".word 0x6220c01f\n\t"
> +
> +#define C9_8_EQ_R1_0 ".word 0x6320c008\n\t"
> +#define C11_10_EQ_R1_0 ".word 0x6320c00a\n\t"
> +#define C15_14_EQ_R1_0 ".word 0x6320c00e\n\t"
> +#define C31_30_EQ_R1_0 ".word 0x6320c01e\n\t"
Only the assignment to PC and C9 (which is an alias for PC) are not allowed by
the assembler. For the others, use the normal assembly syntax.
> +
> +static inline uint32_t write_reg(int rnum, uint32_t val) {
> + uint32_t result;
> + switch (rnum) {
> + case HEX_REG_USR:
> + WRITE_REG("usr", result, val);
> + break;
> + case HEX_REG_PC:
> + WRITE_REG_ENCODED("pc", PC_EQ_R0, result, val);
> + break;
> + case HEX_REG_GP:
> + WRITE_REG_ENCODED("gp", GP_EQ_R0, result, val);
> + break;
> + case HEX_REG_UPCYCLELO:
> + WRITE_REG_ENCODED("upcyclelo", UPCYCLELO_EQ_R0, result, val);
> + break;
> + case HEX_REG_UPCYCLEHI:
> + WRITE_REG_ENCODED("upcyclehi", UPCYCLEHI_EQ_R0, result, val);
> + break;
> + case HEX_REG_UTIMERLO:
> + WRITE_REG_ENCODED("utimerlo", UTIMERLO_EQ_R0, result, val);
> + break;
> + case HEX_REG_UTIMERHI:
> + WRITE_REG_ENCODED("utimerhi", UTIMERHI_EQ_R0, result, val);
> + break;
> + }
> + return result;
> +}
There's a 1-1 mapping from the invocations of this to a line in the switch
statement. Replace the invocation with the body of each case, so you won't
have to include hex_regs.h
> +
> +static inline uint32_t write_reg_in_packet(int rnum, uint32_t val) {
> + uint32_t result;
> + switch (rnum) {
> + case HEX_REG_USR:
> + WRITE_REG_IN_PACKET("usr", result, val);
> + break;
> + case HEX_REG_PC:
> + WRITE_REG_ENCODED_IN_PACKET("pc", PC_EQ_R0, result, val);
> + break;
> + case HEX_REG_GP:
> + WRITE_REG_ENCODED_IN_PACKET("gp", GP_EQ_R0, result, val);
> + break;
> + case HEX_REG_UPCYCLELO:
> + WRITE_REG_ENCODED_IN_PACKET("upcyclelo", UPCYCLELO_EQ_R0,
> result, val);
> + break;
> + case HEX_REG_UPCYCLEHI:
> + WRITE_REG_ENCODED_IN_PACKET("upcyclehi", UPCYCLEHI_EQ_R0,
> result, val);
> + break;
> + case HEX_REG_UTIMERLO:
> + WRITE_REG_ENCODED_IN_PACKET("utimerlo", UTIMERLO_EQ_R0,
> result, val);
> + break;
> + case HEX_REG_UTIMERHI:
> + WRITE_REG_ENCODED_IN_PACKET("utimerhi", UTIMERHI_EQ_R0,
> result, val);
> + break;
> + }
> + return result;
> +}
This is redundant because the IN_PACKET macros are redundant.
> +
> +static inline uint64_t write_reg_pair(int rnum, uint32_t val_hi,
> + uint32_t val_lo) {
> + uint64_t val = (uint64_t) val_hi << 32 | val_lo;
> + uint64_t result;
> + switch (rnum) {
> + case HEX_REG_PAIR_C9_8:
> + WRITE_REG_PAIR_ENCODED("c9:8", C9_8_EQ_R1_0, result, val);
> + break;
> + case HEX_REG_PAIR_C11_10:
> + WRITE_REG_PAIR_ENCODED("c11:10", C11_10_EQ_R1_0, result, val);
> + break;
> + case HEX_REG_PAIR_C15_14:
> + WRITE_REG_PAIR_ENCODED("c15:14", C15_14_EQ_R1_0, result, val);
> + break;
> + case HEX_REG_PAIR_C31_30:
> + WRITE_REG_PAIR_ENCODED("c31:30", C31_30_EQ_R1_0, result, val);
> + break;
> + }
> + return result;
> +}
> +
> +static inline uint64_t write_reg_pair_in_packet(int rnum, uint32_t val_hi,
> + uint32_t val_lo) {
> + uint64_t val = (uint64_t) val_hi << 32 | val_lo;
> + uint64_t result;
> + switch (rnum) {
> + case HEX_REG_PAIR_C9_8:
> + WRITE_REG_PAIR_ENCODED_IN_PACKET("c9:8", C9_8_EQ_R1_0,
> result, val);
> + break;
> + case HEX_REG_PAIR_C11_10:
> + WRITE_REG_PAIR_ENCODED_IN_PACKET("c11:10", C11_10_EQ_R1_0,
> result, val);
> + break;
> + case HEX_REG_PAIR_C15_14:
> + WRITE_REG_PAIR_ENCODED_IN_PACKET("c15:14", C15_14_EQ_R1_0,
> result, val);
> + break;
> + case HEX_REG_PAIR_C31_30:
> + WRITE_REG_PAIR_ENCODED_IN_PACKET("c31:30", C31_30_EQ_R1_0,
> result, val);
> + break;
> + }
> + return result;
> +}
This is redundant also.
> +
> +static inline void write_control_registers(void) {
> + check(write_reg(HEX_REG_USR, 0xffffffff), 0x3ecfff3f);
> + check(write_reg(HEX_REG_GP, 0xffffffff), 0xffffffc0);
> + check(write_reg(HEX_REG_UPCYCLELO, 0xffffffff), 0x0);
> + check(write_reg(HEX_REG_UPCYCLEHI, 0xffffffff), 0x0);
> + check(write_reg(HEX_REG_UTIMERLO, 0xffffffff), 0x0);
> + check(write_reg(HEX_REG_UTIMERHI, 0xffffffff), 0x0);
> +
> + /*
> + * PC is special. Setting it to these values
> + * should cause a catastrophic failure.
> + */
> + check_ne(write_reg(HEX_REG_PC, 0x00000000), 0x00000000);
> + check_ne(write_reg(HEX_REG_PC, 0x00000000), 0x00000001);
> + check_ne(write_reg(HEX_REG_PC, 0xffffffff), 0xffffffff);
> + check_ne(write_reg(HEX_REG_PC, 0x00000000), 0x00000000); }
This is the same as the first test for PC.
> +
> +static inline void write_control_registers_in_packets(void)
> +{
> + check(write_reg_in_packet(HEX_REG_USR, 0xffffffff), 0x3ecfff3f);
> + check(write_reg_in_packet(HEX_REG_GP, 0xffffffff), 0xffffffc0);
> + check(write_reg_in_packet(HEX_REG_UPCYCLELO, 0xffffffff), 0x0);
> + check(write_reg_in_packet(HEX_REG_UPCYCLEHI, 0xffffffff), 0x0);
> + check(write_reg_in_packet(HEX_REG_UTIMERLO, 0xffffffff), 0x0);
> + check(write_reg_in_packet(HEX_REG_UTIMERHI, 0xffffffff), 0x0);
> +
> + check_ne(write_reg_in_packet(HEX_REG_PC, 0x00000000), 0x00000000);
> + check_ne(write_reg_in_packet(HEX_REG_PC, 0x00000001), 0x00000001);
> + check_ne(write_reg_in_packet(HEX_REG_PC, 0xffffffff), 0xffffffff);
> + check_ne(write_reg_in_packet(HEX_REG_PC, 0x00000000), 0x00000000);
> +}
This is redundant.
> +
> +static inline void write_control_register_pairs(void)
> +{
> + check(write_reg_pair(HEX_REG_PAIR_C11_10, 0xffffffff, 0xffffffff),
> + 0xffffffc0ffffffff);
> + check(write_reg_pair(HEX_REG_PAIR_C15_14, 0xffffffff, 0xffffffff), 0x0);
> + check(write_reg_pair(HEX_REG_PAIR_C31_30, 0xffffffff, 0xffffffff),
> +0x0);
> +
> + check_ne(write_reg_pair(HEX_REG_PAIR_C9_8, 0x00000000,
> 0x00000000),
> + 0x0000000000000000);
> + check_ne(write_reg_pair(HEX_REG_PAIR_C9_8, 0x00000001,
> 0x00000000),
> + 0x0000000100000000);
> + check_ne(write_reg_pair(HEX_REG_PAIR_C9_8, 0xffffffff, 0xffffffff),
> + 0xffffffffffffffff);
> + check_ne(write_reg_pair(HEX_REG_PAIR_C9_8, 0x00000000,
> 0x00000000),
> + 0x0000000000000000);
> +}
> +
> +static inline void write_control_register_pairs_in_packets(void)
> +{
> + check(write_reg_pair_in_packet(HEX_REG_PAIR_C11_10, 0xffffffff,
> 0xffffffff),
> + 0xffffffc0ffffffff);
> + check(write_reg_pair_in_packet(HEX_REG_PAIR_C15_14, 0xffffffff,
> 0xffffffff),
> + 0x0);
> + check(write_reg_pair_in_packet(HEX_REG_PAIR_C31_30, 0xffffffff,
> 0xffffffff),
> + 0x0);
> +
> + check_ne(write_reg_pair_in_packet(HEX_REG_PAIR_C9_8, 0x00000000,
> + 0x00000000), 0x0000000000000000);
> + check_ne(write_reg_pair_in_packet(HEX_REG_PAIR_C9_8, 0x00000001,
> + 0x00000000), 0x0000000100000000);
> + check_ne(write_reg_pair_in_packet(HEX_REG_PAIR_C9_8, 0xffffffff,
> + 0xffffffff), 0xffffffffffffffff);
> + check_ne(write_reg_pair_in_packet(HEX_REG_PAIR_C9_8, 0x00000000,
> + 0x00000000), 0x0000000000000000); }
Also redundant.
> +
> +int main()
> +{
> + err = 0;
> +
> + write_control_registers();
> + write_control_registers_in_packets();
> + write_control_register_pairs();
> + write_control_register_pairs_in_packets();
> +
> + puts(err ? "FAIL" : "PASS");
> + return err;
> +}
> --
> 2.25.1