Re: [PATCH 02/11] RISC-V: Adding T-Head CMO instructions

[Richard Henderson <richard.henderson@linaro.org>]

On 9/16/22 08:43, LIU Zhiwei wrote:
> 
> On 2022/9/6 20:22, Christoph Muellner wrote:
>> From: Christoph Müllner <christoph.muellner@vrull.eu>
>>
>> This patch adds support for the T-Head CMO instructions.
>> To avoid interfering with standard extensions, decoder and translation
>> are in its own T-Head specific files.
>> Future patches should be able to easily add additional T-Head extesions.
>>
>> The implementation does not have much functionality (besides accepting
>> the instructions and not qualifying them as illegal instructions if
>> the hart executes in the required privilege level for the instruction),
>> as QEMU does not model CPU caches and instructions don't have any
>> exception behaviour (at least not documented).
>>
>> The documentation shows, that the instructions are gated by
>> mxstatus.theadisaee and mxstatus.ucme. However, since these
>> settings are not changed by the upstream Linux kernel,
>> we simply enable the instructions in all modes.
>>
>> Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
>> ---
>>   target/riscv/cpu.c                         |  1 +
>>   target/riscv/cpu.h                         |  1 +
>>   target/riscv/insn_trans/trans_xthead.c.inc | 66 ++++++++++++++++++++++
>>   target/riscv/meson.build                   |  1 +
>>   target/riscv/translate.c                   | 11 +++-
>>   target/riscv/xtheadcmo.decode              | 43 ++++++++++++++
>>   6 files changed, 120 insertions(+), 3 deletions(-)
>>   create mode 100644 target/riscv/insn_trans/trans_xthead.c.inc
>>   create mode 100644 target/riscv/xtheadcmo.decode
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index ac6f82ebd0..7718ab0478 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -920,6 +920,7 @@ static Property riscv_cpu_extensions[] = {
>>       DEFINE_PROP_BOOL("zhinxmin", RISCVCPU, cfg.ext_zhinxmin, false),
>>       /* Vendor-specific custom extensions */
>> +    DEFINE_PROP_BOOL("xtheadcmo", RISCVCPU, cfg.ext_xtheadcmo, false),
>>       DEFINE_PROP_BOOL("xventanacondops", RISCVCPU, cfg.ext_XVentanaCondOps, false),
>>       /* These are experimental so mark with 'x-' */
>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>> index 5c7acc055a..b7ab53b7b8 100644
>> --- a/target/riscv/cpu.h
>> +++ b/target/riscv/cpu.h
>> @@ -440,6 +440,7 @@ struct RISCVCPUConfig {
>>       uint64_t mimpid;
>>       /* Vendor-specific custom extensions */
>> +    bool ext_xtheadcmo;
>>       bool ext_XVentanaCondOps;
>>       uint8_t pmu_num;
>> diff --git a/target/riscv/insn_trans/trans_xthead.c.inc 
>> b/target/riscv/insn_trans/trans_xthead.c.inc
>> new file mode 100644
>> index 0000000000..1b1e21ab77
>> --- /dev/null
>> +++ b/target/riscv/insn_trans/trans_xthead.c.inc
>> @@ -0,0 +1,66 @@
>> +/*
>> + * RISC-V translation routines for the T-Head vendor extensions (xthead*).
>> + *
>> + * Copyright (c) 2022 VRULL GmbH.
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2 or later, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope 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/>.
>> + */
>> +
>> +#define REQUIRE_PRIV_MHSU(ctx)
>> +
>> +#ifndef CONFIG_USER_ONLY
>> +#define REQUIRE_PRIV_MHS(ctx)                                   \
>> +do {                                                            \
>> +    if (!(ctx->priv == PRV_M ||                                 \
>> +          ctx->priv == PRV_H ||                                 \
>> +          ctx->priv == PRV_S)) {                                \
>> +        return false;                                           \
>> +    }                                                           \
>> +} while (0)
> No ';' here. And in #else
>> +#else
>> +#define REQUIRE_PRIV_MHS(ctx)                                   \
>> +  return false;
> 
> with ';'

Using the inline function that I suggested in response to patch 1, this ifdef is not 
required.  Also, better with an explicit test against PRV_U?


r~