Re: [PATCH 2/2] target/riscv: Support xtheadmaee for thead-c906

[LIU Zhiwei <zhiwei_liu@linux.alibaba.com>]

On 2024/1/30 19:43, Christoph Müllner wrote:
> On Tue, Jan 30, 2024 at 12:12 PM LIU Zhiwei
> <zhiwei_liu@linux.alibaba.com> wrote:
>> thead-c906 uses some flags in pte [60-63] bits. It has history reasons that
>> SVPBMT didn't exist when thead-c906 came to world.
>>
>> We named this feature as xtheadmaee. this feature is controlled by an custom
>> CSR named mxstatus, whose maee field encodes whether enable the pte [60-63] bits.
>>
>> The sections "5.2.2.1 Page table structure" and "15.1.7.1 M-mode extension
>> status register (MXSTATUS)" in document[1] give the detailed information
>> about its design.
> I would prefer if we would not define an extension like XTheadMaee
> without a specification.
> The linked document defines the bit MAEE in a custom CSR, but the
> scope of XTheadMaee
> is not clearly defined (the term XTheadMaee is not even part of the PDF).
>
> We have all the XThead* extensions well described here:
>    https://github.com/T-head-Semi/thead-extension-spec/tree/master
> And it would not be much effort to add XTheadMaee there as well.

Done.  Thanks for the comment. The XTheadMaee specification is here:

https://github.com/T-head-Semi/thead-extension-spec/blob/master/xtheadmaee.adoc

Thanks,
Zhiwei

>
> For those who don't know the context of this patch, here is the c906
> boot regression report from Björn:
>    https://lists.gnu.org/archive/html/qemu-devel/2024-01/msg04766.html
>
> BR
> Christoph
>
> Christoph
>
>> [1]:https://occ-intl-prod.oss-ap-southeast-1.aliyuncs.com/resource//1699265191641/XuanTie-Openc906-UserManual.pdf
>> Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
>> ---
>>   target/riscv/cpu.c         |  6 ++++
>>   target/riscv/cpu.h         |  9 ++++++
>>   target/riscv/cpu_bits.h    |  6 ++++
>>   target/riscv/cpu_cfg.h     |  4 ++-
>>   target/riscv/cpu_helper.c  | 25 ++++++++-------
>>   target/riscv/meson.build   |  1 +
>>   target/riscv/tcg/tcg-cpu.c |  9 ++----
>>   target/riscv/xthead_csr.c  | 63 ++++++++++++++++++++++++++++++++++++++
>>   8 files changed, 105 insertions(+), 18 deletions(-)
>>   create mode 100644 target/riscv/xthead_csr.c
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index 2dcbc9ff32..bfdbb0539a 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -171,6 +171,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
>>       ISA_EXT_DATA_ENTRY(xtheadmemidx, PRIV_VERSION_1_11_0, ext_xtheadmemidx),
>>       ISA_EXT_DATA_ENTRY(xtheadmempair, PRIV_VERSION_1_11_0, ext_xtheadmempair),
>>       ISA_EXT_DATA_ENTRY(xtheadsync, PRIV_VERSION_1_11_0, ext_xtheadsync),
>> +    ISA_EXT_DATA_ENTRY(xtheadmaee, PRIV_VERSION_1_11_0, ext_xtheadmaee),
>>       ISA_EXT_DATA_ENTRY(xventanacondops, PRIV_VERSION_1_12_0, ext_XVentanaCondOps),
>>
>>       DEFINE_PROP_END_OF_LIST(),
>> @@ -506,6 +507,7 @@ static void rv64_thead_c906_cpu_init(Object *obj)
>>
>>       cpu->cfg.mvendorid = THEAD_VENDOR_ID;
>>   #ifndef CONFIG_USER_ONLY
>> +    cpu->cfg.ext_xtheadmaee = true;
>>       set_satp_mode_max_supported(cpu, VM_1_10_SV39);
>>   #endif
>>
>> @@ -949,6 +951,9 @@ static void riscv_cpu_reset_hold(Object *obj)
>>       }
>>
>>       pmp_unlock_entries(env);
>> +    if (riscv_cpu_cfg(env)->ext_xtheadmaee) {
>> +        env->th_mxstatus |= TH_MXSTATUS_MAEE;
>> +    }
>>   #endif
>>       env->xl = riscv_cpu_mxl(env);
>>       riscv_cpu_update_mask(env);
>> @@ -1439,6 +1444,7 @@ const RISCVCPUMultiExtConfig riscv_cpu_vendor_exts[] = {
>>       MULTI_EXT_CFG_BOOL("xtheadmemidx", ext_xtheadmemidx, false),
>>       MULTI_EXT_CFG_BOOL("xtheadmempair", ext_xtheadmempair, false),
>>       MULTI_EXT_CFG_BOOL("xtheadsync", ext_xtheadsync, false),
>> +    MULTI_EXT_CFG_BOOL("xtheadmaee", ext_xtheadmaee, false),
>>       MULTI_EXT_CFG_BOOL("xventanacondops", ext_XVentanaCondOps, false),
>>
>>       DEFINE_PROP_END_OF_LIST(),
>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>> index 5f3955c38d..1bacf40355 100644
>> --- a/target/riscv/cpu.h
>> +++ b/target/riscv/cpu.h
>> @@ -412,6 +412,14 @@ struct CPUArchState {
>>       target_ulong cur_pmmask;
>>       target_ulong cur_pmbase;
>>
>> +    union {
>> +        /* Custom CSR for Xuantie CPU */
>> +        struct {
>> +#ifndef CONFIG_USER_ONLY
>> +            target_ulong th_mxstatus;
>> +#endif
>> +        };
>> +    };
>>       /* Fields from here on are preserved across CPU reset. */
>>       QEMUTimer *stimer; /* Internal timer for S-mode interrupt */
>>       QEMUTimer *vstimer; /* Internal timer for VS-mode interrupt */
>> @@ -799,6 +807,7 @@ void riscv_add_satp_mode_properties(Object *obj);
>>   bool riscv_cpu_accelerator_compatible(RISCVCPU *cpu);
>>
>>   /* CSR function table */
>> +extern riscv_csr_operations th_csr_ops[CSR_TABLE_SIZE];
>>   extern riscv_csr_operations csr_ops[CSR_TABLE_SIZE];
>>
>>   extern const bool valid_vm_1_10_32[], valid_vm_1_10_64[];
>> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
>> index e116f6c252..67ebb1cefe 100644
>> --- a/target/riscv/cpu_bits.h
>> +++ b/target/riscv/cpu_bits.h
>> @@ -897,4 +897,10 @@ typedef enum RISCVException {
>>   /* JVT CSR bits */
>>   #define JVT_MODE                           0x3F
>>   #define JVT_BASE                           (~0x3F)
>> +
>> +/* Xuantie custom CSRs */
>> +#define CSR_TH_MXSTATUS 0x7c0
>> +
>> +#define TH_MXSTATUS_MAEE_SHIFT  21
>> +#define TH_MXSTATUS_MAEE        (0x1 << TH_MXSTATUS_MAEE_SHIFT)
>>   #endif
>> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
>> index 780ae6ef17..3735c69fd6 100644
>> --- a/target/riscv/cpu_cfg.h
>> +++ b/target/riscv/cpu_cfg.h
>> @@ -136,6 +136,7 @@ struct RISCVCPUConfig {
>>       bool ext_xtheadmemidx;
>>       bool ext_xtheadmempair;
>>       bool ext_xtheadsync;
>> +    bool ext_xtheadmaee;
>>       bool ext_XVentanaCondOps;
>>
>>       uint32_t pmu_mask;
>> @@ -176,7 +177,8 @@ static inline bool has_xthead_p(const RISCVCPUConfig *cfg)
>>              cfg->ext_xtheadcondmov ||
>>              cfg->ext_xtheadfmemidx || cfg->ext_xtheadfmv ||
>>              cfg->ext_xtheadmac || cfg->ext_xtheadmemidx ||
>> -           cfg->ext_xtheadmempair || cfg->ext_xtheadsync;
>> +           cfg->ext_xtheadmempair || cfg->ext_xtheadsync ||
>> +           cfg->ext_xtheadmaee;
>>   }
>>
>>   #define MATERIALISE_EXT_PREDICATE(ext) \
>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>> index c7cc7eb423..5c1f380276 100644
>> --- a/target/riscv/cpu_helper.c
>> +++ b/target/riscv/cpu_helper.c
>> @@ -812,6 +812,7 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
>>       int napot_bits = 0;
>>       target_ulong napot_mask;
>>
>> +    bool skip_pte_check = env->th_mxstatus & TH_MXSTATUS_MAEE;
>>       /*
>>        * Check if we should use the background registers for the two
>>        * stage translation. We don't need to check if we actually need
>> @@ -974,18 +975,19 @@ restart:
>>           if (riscv_cpu_sxl(env) == MXL_RV32) {
>>               ppn = pte >> PTE_PPN_SHIFT;
>>           } else {
>> -            if (pte & PTE_RESERVED) {
>> -                return TRANSLATE_FAIL;
>> -            }
>> +            if (!skip_pte_check) {
>> +                if (pte & PTE_RESERVED) {
>> +                    return TRANSLATE_FAIL;
>> +                }
>>
>> -            if (!pbmte && (pte & PTE_PBMT)) {
>> -                return TRANSLATE_FAIL;
>> -            }
>> +                if (!pbmte && (pte & PTE_PBMT)) {
>> +                    return TRANSLATE_FAIL;
>> +                }
>>
>> -            if (!riscv_cpu_cfg(env)->ext_svnapot && (pte & PTE_N)) {
>> -                return TRANSLATE_FAIL;
>> +                if (!riscv_cpu_cfg(env)->ext_svnapot && (pte & PTE_N)) {
>> +                    return TRANSLATE_FAIL;
>> +                }
>>               }
>> -
>>               ppn = (pte & (target_ulong)PTE_PPN_MASK) >> PTE_PPN_SHIFT;
>>           }
>>
>> @@ -998,7 +1000,8 @@ restart:
>>           }
>>
>>           /* Inner PTE, continue walking */
>> -        if (pte & (PTE_D | PTE_A | PTE_U | PTE_ATTR)) {
>> +        if ((pte & (PTE_D | PTE_A | PTE_U)) ||
>> +            (!skip_pte_check && (pte & PTE_ATTR))) {
>>               return TRANSLATE_FAIL;
>>           }
>>           base = ppn << PGSHIFT;
>> @@ -1012,7 +1015,7 @@ restart:
>>           /* Misaligned PPN */
>>           return TRANSLATE_FAIL;
>>       }
>> -    if (!pbmte && (pte & PTE_PBMT)) {
>> +    if (!skip_pte_check && !pbmte && (pte & PTE_PBMT)) {
>>           /* Reserved without Svpbmt. */
>>           return TRANSLATE_FAIL;
>>       }
>> diff --git a/target/riscv/meson.build b/target/riscv/meson.build
>> index a5e0734e7f..d7f675881d 100644
>> --- a/target/riscv/meson.build
>> +++ b/target/riscv/meson.build
>> @@ -12,6 +12,7 @@ riscv_ss.add(files(
>>     'cpu.c',
>>     'cpu_helper.c',
>>     'csr.c',
>> +  'xthead_csr.c',
>>     'fpu_helper.c',
>>     'gdbstub.c',
>>     'op_helper.c',
>> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
>> index 408b2ebffa..18a22dfb7a 100644
>> --- a/target/riscv/tcg/tcg-cpu.c
>> +++ b/target/riscv/tcg/tcg-cpu.c
>> @@ -763,7 +763,6 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>>           cpu->cfg.pmu_mask = 0;
>>           cpu->pmu_avail_ctrs = 0;
>>       }
>> -
>>       /*
>>        * Disable isa extensions based on priv spec after we
>>        * validated and set everything we need.
>> @@ -871,11 +870,9 @@ static void riscv_cpu_validate_profiles(RISCVCPU *cpu)
>>       }
>>   }
>>
>> -/* This stub just works for making vendors array not empty */
>> -riscv_csr_operations stub_csr_ops[CSR_TABLE_SIZE];
>> -static inline bool never_p(const RISCVCPUConfig *cfg)
>> +static inline bool th_csr_p(const RISCVCPUConfig *cfg)
>>   {
>> -    return false;
>> +    return cfg->ext_xtheadmaee;
>>   }
>>
>>   void riscv_tcg_cpu_register_vendor_csr(RISCVCPU *cpu)
>> @@ -884,7 +881,7 @@ void riscv_tcg_cpu_register_vendor_csr(RISCVCPU *cpu)
>>           bool (*guard_func)(const RISCVCPUConfig *);
>>           riscv_csr_operations *csr_ops;
>>       } vendors[] = {
>> -        { never_p, stub_csr_ops },
>> +        { th_csr_p, th_csr_ops },
>>       };
>>       for (size_t i = 0; i < ARRAY_SIZE(vendors); ++i) {
>>           if (!vendors[i].guard_func(&cpu->cfg)) {
>> diff --git a/target/riscv/xthead_csr.c b/target/riscv/xthead_csr.c
>> new file mode 100644
>> index 0000000000..c119a18789
>> --- /dev/null
>> +++ b/target/riscv/xthead_csr.c
>> @@ -0,0 +1,63 @@
>> +/*
>> + * Xuantie implementation for RISC-V Control and Status Registers.
>> + *
>> + * Copyright (c) 2024 Alibaba Group. All rights reserved.
>> + *
>> + * 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/>.
>> + */
>> +
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/log.h"
>> +#include "cpu.h"
>> +#include "tcg/tcg-cpu.h"
>> +#include "exec/exec-all.h"
>> +#include "exec/tb-flush.h"
>> +#include "qapi/error.h"
>> +
>> +static RISCVException th_maee_check(CPURISCVState *env, int csrno)
>> +{
>> +    if (riscv_cpu_cfg(env)->ext_xtheadmaee) {
>> +        return RISCV_EXCP_ILLEGAL_INST;
>> +    }
>> +    return RISCV_EXCP_NONE;
>> +}
>> +
>> +static RISCVException
>> +read_th_mxstatus(CPURISCVState *env, int csrno, target_ulong *val)
>> +{
>> +    *val = env->th_mxstatus;
>> +    return RISCV_EXCP_NONE;
>> +}
>> +
>> +static RISCVException
>> +write_th_mxstatus(CPURISCVState *env, int csrno, target_ulong val)
>> +{
>> +    uint64_t mxstatus = env->th_mxstatus;
>> +    uint64_t mask = TH_MXSTATUS_MAEE;
>> +
>> +    if ((val ^ mxstatus) & TH_MXSTATUS_MAEE) {
>> +        tlb_flush(env_cpu(env));
>> +    }
>> +
>> +    mxstatus = (mxstatus & ~mask) | (val & mask);
>> +    env->th_mxstatus = mxstatus;
>> +    return RISCV_EXCP_NONE;
>> +}
>> +
>> +riscv_csr_operations th_csr_ops[CSR_TABLE_SIZE] = {
>> +#if !defined(CONFIG_USER_ONLY)
>> +    [CSR_TH_MXSTATUS]     = { "th_mxstatus", th_maee_check, read_th_mxstatus,
>> +                                                            write_th_mxstatus},
>> +#endif /* !CONFIG_USER_ONLY */
>> +};
>> --
>> 2.25.1
>>