From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id C4B2FC433EF for ; Tue, 5 Jul 2022 00:39:57 +0000 (UTC) Received: from localhost ([::1]:52252 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1o8Wbb-00026h-BP for qemu-devel@archiver.kernel.org; Mon, 04 Jul 2022 20:39:56 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:52934) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o8WaJ-0000wu-Pl; Mon, 04 Jul 2022 20:38:35 -0400 Received: from smtp84.cstnet.cn ([159.226.251.84]:55030 helo=cstnet.cn) by eggs.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1o8WaG-0004rp-58; Mon, 04 Jul 2022 20:38:35 -0400 Received: from [192.168.0.138] (unknown [117.151.235.252]) by APP-05 (Coremail) with SMTP id zQCowAD3pu3+h8NiJoqECg--.13300S2; Tue, 05 Jul 2022 08:38:22 +0800 (CST) Subject: Re: [PATCH v10 04/12] target/riscv: pmu: Make number of counters configurable From: Weiwei Li To: Atish Patra , qemu-devel@nongnu.org Cc: Bin Meng , Alistair Francis , Bin Meng , Palmer Dabbelt , qemu-riscv@nongnu.org, frank.chang@sifive.com References: <20220620231603.2547260-1-atishp@rivosinc.com> <20220620231603.2547260-5-atishp@rivosinc.com> Message-ID: Date: Tue, 5 Jul 2022 08:38:22 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US X-CM-TRANSID: zQCowAD3pu3+h8NiJoqECg--.13300S2 X-Coremail-Antispam: 1UD129KBjvJXoWfJw1fXr17Cr1xuFy5tw1UJrb_yoWkGFWxpr ykKrWDJry5JrykAFZ5Cw1UJryavr18Ja98Cw4rX3Wjyr48XrsYqr1UWr1vgr48Jr4rX34x Jr1j9wn3Za17GFDanT9S1TB71UUUUUUqnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2 9KBjDU0xBIdaVrnRJUUU9014x267AKxVW8JVW5JwAFc2x0x2IEx4CE42xK8VAvwI8IcIk0 rVWrJVCq3wAFIxvE14AKwVWUJVWUGwA2ocxC64kIII0Yj41l84x0c7CEw4AK67xGY2AK02 1l84ACjcxK6xIIjxv20xvE14v26r4j6ryUM28EF7xvwVC0I7IYx2IY6xkF7I0E14v26r4j 6F4UM28EF7xvwVC2z280aVAFwI0_Gr0_Cr1l84ACjcxK6I8E87Iv6xkF7I0E14v26r4j6r 4UJwAS0I0E0xvYzxvE52x082IY62kv0487Mc02F40EFcxC0VAKzVAqx4xG6I80ewAv7VC0 I7IYx2IY67AKxVWUGVWUXwAv7VC2z280aVAFwI0_Jr0_Gr1lOx8S6xCaFVCjc4AY6r1j6r 4UM4x0Y48IcVAKI48JM4x0x7Aq67IIx4CEVc8vx2IErcIFxwCYjI0SjxkI62AI1cAE67vI Y487MxkF7I0Ew4C26cxK6c8Ij28IcwCF04k20xvY0x0EwIxGrwCFx2IqxVCFs4IE7xkEbV WUJVW8JwC20s026c02F40E14v26r1j6r18MI8I3I0E7480Y4vE14v26r106r1rMI8E67AF 67kF1VAFwI0_Jw0_GFylIxkGc2Ij64vIr41lIxAIcVC0I7IYx2IY67AKxVWUJVWUCwCI42 IY6xIIjxv20xvEc7CjxVAFwI0_Gr0_Cr1lIxAIcVCF04k26cxKx2IYs7xG6rWUJVWrZr1U MIIF0xvEx4A2jsIE14v26r1j6r4UMIIF0xvEx4A2jsIEc7CjxVAFwI0_Gr0_Gr1UYxBIda VFxhVjvjDU0xZFpf9x0JUTOJ5UUUUU= X-Originating-IP: [117.151.235.252] X-CM-SenderInfo: 5olzvxxzhlqxpvfd2hldfou0/ Received-SPF: pass client-ip=159.226.251.84; envelope-from=liweiwei@iscas.ac.cn; helo=cstnet.cn X-Spam_score_int: -18 X-Spam_score: -1.9 X-Spam_bar: - X-Spam_report: (-1.9 / 5.0 requ) BAYES_00=-1.9, NICE_REPLY_A=-0.001, SPF_HELO_PASS=-0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" 在 2022/7/4 下午11:26, Weiwei Li 写道: > > 在 2022/6/21 上午7:15, Atish Patra 写道: >> The RISC-V privilege specification provides flexibility to implement >> any number of counters from 29 programmable counters. However, the QEMU >> implements all the counters. >> >> Make it configurable through pmu config parameter which now will >> indicate >> how many programmable counters should be implemented by the cpu. >> >> Reviewed-by: Bin Meng >> Reviewed-by: Alistair Francis >> Signed-off-by: Atish Patra >> Signed-off-by: Atish Patra >> --- >>   target/riscv/cpu.c |  3 +- >>   target/riscv/cpu.h |  2 +- >>   target/riscv/csr.c | 94 ++++++++++++++++++++++++++++++---------------- >>   3 files changed, 63 insertions(+), 36 deletions(-) >> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c >> index 1b57b3c43980..d12c6dc630ca 100644 >> --- a/target/riscv/cpu.c >> +++ b/target/riscv/cpu.c >> @@ -851,7 +851,6 @@ static void riscv_cpu_init(Object *obj) >>   { >>       RISCVCPU *cpu = RISCV_CPU(obj); >>   -    cpu->cfg.ext_pmu = true; >>       cpu->cfg.ext_ifencei = true; >>       cpu->cfg.ext_icsr = true; >>       cpu->cfg.mmu = true; >> @@ -879,7 +878,7 @@ static Property riscv_cpu_extensions[] = { >>       DEFINE_PROP_BOOL("u", RISCVCPU, cfg.ext_u, true), >>       DEFINE_PROP_BOOL("v", RISCVCPU, cfg.ext_v, false), >>       DEFINE_PROP_BOOL("h", RISCVCPU, cfg.ext_h, true), >> -    DEFINE_PROP_BOOL("pmu", RISCVCPU, cfg.ext_pmu, true), >> +    DEFINE_PROP_UINT8("pmu-num", RISCVCPU, cfg.pmu_num, 16), > > I think It's better to add  a check on cfg.pmu_num to  <= 29. > OK, I find this check in the following patch. >>       DEFINE_PROP_BOOL("Zifencei", RISCVCPU, cfg.ext_ifencei, true), >>       DEFINE_PROP_BOOL("Zicsr", RISCVCPU, cfg.ext_icsr, true), >>       DEFINE_PROP_BOOL("Zfh", RISCVCPU, cfg.ext_zfh, false), >> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h >> index 252c30a55d78..ffee54ea5c27 100644 >> --- a/target/riscv/cpu.h >> +++ b/target/riscv/cpu.h >> @@ -397,7 +397,6 @@ struct RISCVCPUConfig { >>       bool ext_zksed; >>       bool ext_zksh; >>       bool ext_zkt; >> -    bool ext_pmu; >>       bool ext_ifencei; >>       bool ext_icsr; >>       bool ext_svinval; >> @@ -421,6 +420,7 @@ struct RISCVCPUConfig { >>       /* Vendor-specific custom extensions */ >>       bool ext_XVentanaCondOps; >>   +    uint8_t pmu_num; >>       char *priv_spec; >>       char *user_spec; >>       char *bext_spec; >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c >> index 0ca05c77883c..b4a8e15f498f 100644 >> --- a/target/riscv/csr.c >> +++ b/target/riscv/csr.c >> @@ -73,9 +73,17 @@ static RISCVException ctr(CPURISCVState *env, int >> csrno) >>       CPUState *cs = env_cpu(env); >>       RISCVCPU *cpu = RISCV_CPU(cs); >>       int ctr_index; >> +    int base_csrno = CSR_HPMCOUNTER3; >> +    bool rv32 = riscv_cpu_mxl(env) == MXL_RV32 ? true : false; >>   -    if (!cpu->cfg.ext_pmu) { >> -        /* The PMU extension is not enabled */ >> +    if (rv32 && csrno >= CSR_CYCLEH) { >> +        /* Offset for RV32 hpmcounternh counters */ >> +        base_csrno += 0x80; >> +    } >> +    ctr_index = csrno - base_csrno; >> + >> +    if (!cpu->cfg.pmu_num || ctr_index >= (cpu->cfg.pmu_num)) { >> +        /* No counter is enabled in PMU or the counter is out of >> range */ > > I seems unnecessary to add '!cpu->cfg.pmu_num ' here, 'ctr_index >= > (cpu->cfg.pmu_num)' is true Typo.  I -> It > > when cpu->cfg.pmu_num is zero if the problem for base_csrno is fixed. > > Ragards, > > Weiwei Li > >>           return RISCV_EXCP_ILLEGAL_INST; >>       } >>   @@ -103,7 +111,7 @@ static RISCVException ctr(CPURISCVState *env, >> int csrno) >>               } >>               break; >>           } >> -        if (riscv_cpu_mxl(env) == MXL_RV32) { >> +        if (rv32) { >>               switch (csrno) { >>               case CSR_CYCLEH: >>                   if (!get_field(env->mcounteren, COUNTEREN_CY)) { >> @@ -158,7 +166,7 @@ static RISCVException ctr(CPURISCVState *env, int >> csrno) >>               } >>               break; >>           } >> -        if (riscv_cpu_mxl(env) == MXL_RV32) { >> +        if (rv32) { >>               switch (csrno) { >>               case CSR_CYCLEH: >>                   if (!get_field(env->hcounteren, COUNTEREN_CY) && >> @@ -202,6 +210,26 @@ static RISCVException ctr32(CPURISCVState *env, >> int csrno) >>   } >>     #if !defined(CONFIG_USER_ONLY) >> +static RISCVException mctr(CPURISCVState *env, int csrno) >> +{ >> +    CPUState *cs = env_cpu(env); >> +    RISCVCPU *cpu = RISCV_CPU(cs); >> +    int ctr_index; >> +    int base_csrno = CSR_MHPMCOUNTER3; >> + >> +    if ((riscv_cpu_mxl(env) == MXL_RV32) && csrno >= CSR_MCYCLEH) { >> +        /* Offset for RV32 mhpmcounternh counters */ >> +        base_csrno += 0x80; >> +    } >> +    ctr_index = csrno - base_csrno; >> +    if (!cpu->cfg.pmu_num || ctr_index >= cpu->cfg.pmu_num) { >> +        /* The PMU is not enabled or counter is out of range*/ >> +        return RISCV_EXCP_ILLEGAL_INST; >> +    } >> + >> +    return RISCV_EXCP_NONE; >> +} >> + >>   static RISCVException any(CPURISCVState *env, int csrno) >>   { >>       return RISCV_EXCP_NONE; >> @@ -3687,35 +3715,35 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = { >>       [CSR_HPMCOUNTER30]   = { "hpmcounter30",   ctr, read_zero }, >>       [CSR_HPMCOUNTER31]   = { "hpmcounter31",   ctr, read_zero }, >>   -    [CSR_MHPMCOUNTER3]   = { "mhpmcounter3",   any, read_zero }, >> -    [CSR_MHPMCOUNTER4]   = { "mhpmcounter4",   any, read_zero }, >> -    [CSR_MHPMCOUNTER5]   = { "mhpmcounter5",   any, read_zero }, >> -    [CSR_MHPMCOUNTER6]   = { "mhpmcounter6",   any, read_zero }, >> -    [CSR_MHPMCOUNTER7]   = { "mhpmcounter7",   any, read_zero }, >> -    [CSR_MHPMCOUNTER8]   = { "mhpmcounter8",   any, read_zero }, >> -    [CSR_MHPMCOUNTER9]   = { "mhpmcounter9",   any, read_zero }, >> -    [CSR_MHPMCOUNTER10]  = { "mhpmcounter10",  any, read_zero }, >> -    [CSR_MHPMCOUNTER11]  = { "mhpmcounter11",  any, read_zero }, >> -    [CSR_MHPMCOUNTER12]  = { "mhpmcounter12",  any, read_zero }, >> -    [CSR_MHPMCOUNTER13]  = { "mhpmcounter13",  any, read_zero }, >> -    [CSR_MHPMCOUNTER14]  = { "mhpmcounter14",  any, read_zero }, >> -    [CSR_MHPMCOUNTER15]  = { "mhpmcounter15",  any, read_zero }, >> -    [CSR_MHPMCOUNTER16]  = { "mhpmcounter16",  any, read_zero }, >> -    [CSR_MHPMCOUNTER17]  = { "mhpmcounter17",  any, read_zero }, >> -    [CSR_MHPMCOUNTER18]  = { "mhpmcounter18",  any, read_zero }, >> -    [CSR_MHPMCOUNTER19]  = { "mhpmcounter19",  any, read_zero }, >> -    [CSR_MHPMCOUNTER20]  = { "mhpmcounter20",  any, read_zero }, >> -    [CSR_MHPMCOUNTER21]  = { "mhpmcounter21",  any, read_zero }, >> -    [CSR_MHPMCOUNTER22]  = { "mhpmcounter22",  any, read_zero }, >> -    [CSR_MHPMCOUNTER23]  = { "mhpmcounter23",  any, read_zero }, >> -    [CSR_MHPMCOUNTER24]  = { "mhpmcounter24",  any, read_zero }, >> -    [CSR_MHPMCOUNTER25]  = { "mhpmcounter25",  any, read_zero }, >> -    [CSR_MHPMCOUNTER26]  = { "mhpmcounter26",  any, read_zero }, >> -    [CSR_MHPMCOUNTER27]  = { "mhpmcounter27",  any, read_zero }, >> -    [CSR_MHPMCOUNTER28]  = { "mhpmcounter28",  any, read_zero }, >> -    [CSR_MHPMCOUNTER29]  = { "mhpmcounter29",  any, read_zero }, >> -    [CSR_MHPMCOUNTER30]  = { "mhpmcounter30",  any, read_zero }, >> -    [CSR_MHPMCOUNTER31]  = { "mhpmcounter31",  any, read_zero }, >> +    [CSR_MHPMCOUNTER3]   = { "mhpmcounter3",   mctr, read_zero }, >> +    [CSR_MHPMCOUNTER4]   = { "mhpmcounter4",   mctr, read_zero }, >> +    [CSR_MHPMCOUNTER5]   = { "mhpmcounter5",   mctr, read_zero }, >> +    [CSR_MHPMCOUNTER6]   = { "mhpmcounter6",   mctr, read_zero }, >> +    [CSR_MHPMCOUNTER7]   = { "mhpmcounter7",   mctr, read_zero }, >> +    [CSR_MHPMCOUNTER8]   = { "mhpmcounter8",   mctr, read_zero }, >> +    [CSR_MHPMCOUNTER9]   = { "mhpmcounter9",   mctr, read_zero }, >> +    [CSR_MHPMCOUNTER10]  = { "mhpmcounter10",  mctr, read_zero }, >> +    [CSR_MHPMCOUNTER11]  = { "mhpmcounter11",  mctr, read_zero }, >> +    [CSR_MHPMCOUNTER12]  = { "mhpmcounter12",  mctr, read_zero }, >> +    [CSR_MHPMCOUNTER13]  = { "mhpmcounter13",  mctr, read_zero }, >> +    [CSR_MHPMCOUNTER14]  = { "mhpmcounter14",  mctr, read_zero }, >> +    [CSR_MHPMCOUNTER15]  = { "mhpmcounter15",  mctr, read_zero }, >> +    [CSR_MHPMCOUNTER16]  = { "mhpmcounter16",  mctr, read_zero }, >> +    [CSR_MHPMCOUNTER17]  = { "mhpmcounter17",  mctr, read_zero }, >> +    [CSR_MHPMCOUNTER18]  = { "mhpmcounter18",  mctr, read_zero }, >> +    [CSR_MHPMCOUNTER19]  = { "mhpmcounter19",  mctr, read_zero }, >> +    [CSR_MHPMCOUNTER20]  = { "mhpmcounter20",  mctr, read_zero }, >> +    [CSR_MHPMCOUNTER21]  = { "mhpmcounter21",  mctr, read_zero }, >> +    [CSR_MHPMCOUNTER22]  = { "mhpmcounter22",  mctr, read_zero }, >> +    [CSR_MHPMCOUNTER23]  = { "mhpmcounter23",  mctr, read_zero }, >> +    [CSR_MHPMCOUNTER24]  = { "mhpmcounter24",  mctr, read_zero }, >> +    [CSR_MHPMCOUNTER25]  = { "mhpmcounter25",  mctr, read_zero }, >> +    [CSR_MHPMCOUNTER26]  = { "mhpmcounter26",  mctr, read_zero }, >> +    [CSR_MHPMCOUNTER27]  = { "mhpmcounter27",  mctr, read_zero }, >> +    [CSR_MHPMCOUNTER28]  = { "mhpmcounter28",  mctr, read_zero }, >> +    [CSR_MHPMCOUNTER29]  = { "mhpmcounter29",  mctr, read_zero }, >> +    [CSR_MHPMCOUNTER30]  = { "mhpmcounter30",  mctr, read_zero }, >> +    [CSR_MHPMCOUNTER31]  = { "mhpmcounter31",  mctr, read_zero }, >>         [CSR_MHPMEVENT3]     = { "mhpmevent3",     any, read_zero }, >>       [CSR_MHPMEVENT4]     = { "mhpmevent4",     any, read_zero }, >