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 X-Spam-Level: X-Spam-Status: No, score=-12.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,HTML_MESSAGE,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,UNPARSEABLE_RELAY, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 84BEDC11F64 for ; Thu, 1 Jul 2021 09:40:30 +0000 (UTC) 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 mail.kernel.org (Postfix) with ESMTPS id 680AC61449 for ; Thu, 1 Jul 2021 09:40:29 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 680AC61449 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=c-sky.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Received: from localhost ([::1]:47442 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1lytBM-0000RT-EV for qemu-devel@archiver.kernel.org; Thu, 01 Jul 2021 05:40:28 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:47664) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lytAe-0008CB-RN; Thu, 01 Jul 2021 05:39:44 -0400 Received: from out28-5.mail.aliyun.com ([115.124.28.5]:50551) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lytAa-0007LA-Fe; Thu, 01 Jul 2021 05:39:44 -0400 X-Alimail-AntiSpam: AC=CONTINUE; BC=0.07183115|-1; CH=green; DM=|CONTINUE|false|; DS=CONTINUE|ham_enroll_verification|0.444704-0.000262462-0.555034; FP=0|0|0|0|0|-1|-1|-1; HT=ay29a033018047190; MF=zhiwei_liu@c-sky.com; NM=1; PH=DS; RN=7; RT=7; SR=0; TI=SMTPD_---.KahuT9m_1625132372; Received: from 10.0.2.15(mailfrom:zhiwei_liu@c-sky.com fp:SMTPD_---.KahuT9m_1625132372) by smtp.aliyun-inc.com(10.147.41.158); Thu, 01 Jul 2021 17:39:32 +0800 Subject: Re: [RFC PATCH 01/11] target/riscv: Add CLIC CSR mintstatus To: Frank Chang References: <20210409074857.166082-1-zhiwei_liu@c-sky.com> <20210409074857.166082-2-zhiwei_liu@c-sky.com> <20304ba9-6f87-f7b9-a24d-15d4b3d3f75a@c-sky.com> From: LIU Zhiwei Message-ID: Date: Thu, 1 Jul 2021 17:38:10 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/alternative; boundary="------------5BBB9B2C501C837B9C24C1AE" Content-Language: en-US Received-SPF: none client-ip=115.124.28.5; envelope-from=zhiwei_liu@c-sky.com; helo=out28-5.mail.aliyun.com 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, HTML_MESSAGE=0.001, NICE_REPLY_A=-0.001, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H3=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_NONE=0.001, UNPARSEABLE_RELAY=0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "open list:RISC-V" , "qemu-devel@nongnu.org Developers" , wxy194768@alibaba-inc.com, Alistair Francis , Alistair Francis , Palmer Dabbelt Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" This is a multi-part message in MIME format. --------------5BBB9B2C501C837B9C24C1AE Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit On 2021/7/1 下午4:45, Frank Chang wrote: > LIU Zhiwei > 於 > 2021年4月20日 週二 上午8:49寫道： > > > On 2021/4/20 上午7:23, Alistair Francis wrote: > > On Fri, Apr 9, 2021 at 5:52 PM LIU Zhiwei > wrote: > >> CSR mintstatus holds the active interrupt level for each supported > >> privilege mode. sintstatus, and user, uintstatus, provide > restricted > >> views of mintstatus. > >> > >> Signed-off-by: LIU Zhiwei > > >> --- > >>   target/riscv/cpu.h      |  2 ++ > >>   target/riscv/cpu_bits.h | 11 +++++++++++ > >>   target/riscv/csr.c      | 26 ++++++++++++++++++++++++++ > >>   3 files changed, 39 insertions(+) > >> > >> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > >> index 0a33d387ba..1a44ca62c7 100644 > >> --- a/target/riscv/cpu.h > >> +++ b/target/riscv/cpu.h > >> @@ -159,6 +159,7 @@ struct CPURISCVState { > >>       target_ulong mip; > >> > >>       uint32_t miclaim; > >> +    uint32_t mintstatus; /* clic-spec */ > >> > >>       target_ulong mie; > >>       target_ulong mideleg; > >> @@ -243,6 +244,7 @@ struct CPURISCVState { > >> > >>       /* Fields from here on are preserved across CPU reset. */ > >>       QEMUTimer *timer; /* Internal timer */ > >> +    void *clic;       /* clic interrupt controller */ > > This should be the CLIC type. > > OK. > > Actually there are many versions of CLIC in my branch as different > devices. But it is better to use CLIC type for the upstream version. > > > Hi Alistair and Zhiwei, > > Replacing void *clic with RISCVCLICState *clic may create a circular loop > because CPURISCVState is also referenced in riscv_clic.h. > As there is only one reference to CPURISCVState,  it is not difficult to get rid of it. > However, I would like to ask what is the best approach to add > the reference of CLIC device in CPURISCVState struct? > > There may be different kinds of CLIC devices. > AFAK, there was another RFC patchset trying to add void *eclic > for Nuclei processor into CPURISCVState struct: > https://patchwork.kernel.org/project/qemu-devel/patch/20210507081654.11056-2-wangjunqiang@iscas.ac.cn/ > > > Is it okay to add the device reference directly into CPURISCVState > struct like that, > or we should create some abstraction for these CLIC devices? > (However, I'm not sure how big the differences are for these CLIC > devices...) > In my opinion, we should be very cautious to include this kind of code,  although I suffer from it too. We should only support the features defined in the specification. If some feature are neither defined by the specification, nor  under the customized range specified by the specification, we should review the code and exclude it from the mainline. The standard implementation of an old, ratified  as drafted specification can be allowed in the mainline. If a new specification comes,  and the old implementation can carefully avoid  the conflicting , it can still in the mainline.  Otherwise, the old implementation should be obsoleted. Thanks, Zhiwei > Thanks, > Frank Chang > > > > > >>   }; > >> > >>   OBJECT_DECLARE_TYPE(RISCVCPU, RISCVCPUClass, > >> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h > >> index caf4599207..c4ce6ec3d9 100644 > >> --- a/target/riscv/cpu_bits.h > >> +++ b/target/riscv/cpu_bits.h > >> @@ -165,6 +165,7 @@ > >>   #define CSR_MCAUSE          0x342 > >>   #define CSR_MTVAL           0x343 > >>   #define CSR_MIP             0x344 > >> +#define CSR_MINTSTATUS      0x346 /* clic-spec-draft */ > >> > >>   /* Legacy Machine Trap Handling (priv v1.9.1) */ > >>   #define CSR_MBADADDR        0x343 > >> @@ -183,6 +184,7 @@ > >>   #define CSR_SCAUSE          0x142 > >>   #define CSR_STVAL           0x143 > >>   #define CSR_SIP             0x144 > >> +#define CSR_SINTSTATUS      0x146 /* clic-spec-draft */ > >> > >>   /* Legacy Supervisor Trap Handling (priv v1.9.1) */ > >>   #define CSR_SBADADDR        0x143 > >> @@ -585,6 +587,15 @@ > >>   #define SIP_STIP  MIP_STIP > >>   #define SIP_SEIP  MIP_SEIP > >> > >> +/* mintstatus */ > >> +#define MINTSTATUS_MIL  0xff000000 /* mil[7:0] */ > >> +#define MINTSTATUS_SIL  0x0000ff00 /* sil[7:0] */ > >> +#define MINTSTATUS_UIL  0x000000ff /* uil[7:0] */ > >> + > >> +/* sintstatus */ > >> +#define SINTSTATUS_SIL  0x0000ff00 /* sil[7:0] */ > >> +#define SINTSTATUS_UIL  0x000000ff /* uil[7:0] */ > > The bit fields in the comments are out of date. > > I didn't notice it.   Fix it in next version. > > Thanks. > > Zhiwei > > > > > Alistair > > > >> + > >>   /* MIE masks */ > >>   #define MIE_SEIE                           (1 << IRQ_S_EXT) > >>   #define MIE_UEIE                           (1 << IRQ_U_EXT) > >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c > >> index d2585395bf..320b18ab60 100644 > >> --- a/target/riscv/csr.c > >> +++ b/target/riscv/csr.c > >> @@ -188,6 +188,12 @@ static int pmp(CPURISCVState *env, int csrno) > >>   { > >>       return -!riscv_feature(env, RISCV_FEATURE_PMP); > >>   } > >> + > >> +static int clic(CPURISCVState *env, int csrno) > >> +{ > >> +    return !!env->clic; > >> +} > >> + > >>   #endif > >> > >>   /* User Floating-Point CSRs */ > >> @@ -734,6 +740,12 @@ static int rmw_mip(CPURISCVState *env, int > csrno, target_ulong *ret_value, > >>       return 0; > >>   } > >> > >> +static int read_mintstatus(CPURISCVState *env, int csrno, > target_ulong *val) > >> +{ > >> +    *val = env->mintstatus; > >> +    return 0; > >> +} > >> + > >>   /* Supervisor Trap Setup */ > >>   static int read_sstatus(CPURISCVState *env, int csrno, > target_ulong *val) > >>   { > >> @@ -893,6 +905,13 @@ static int rmw_sip(CPURISCVState *env, int > csrno, target_ulong *ret_value, > >>       return ret; > >>   } > >> > >> +static int read_sintstatus(CPURISCVState *env, int csrno, > target_ulong *val) > >> +{ > >> +    target_ulong mask = SINTSTATUS_SIL | SINTSTATUS_UIL; > >> +    *val = env->mintstatus & mask; > >> +    return 0; > >> +} > >> + > >>   /* Supervisor Protection and Translation */ > >>   static int read_satp(CPURISCVState *env, int csrno, > target_ulong *val) > >>   { > >> @@ -1644,5 +1663,12 @@ riscv_csr_operations > csr_ops[CSR_TABLE_SIZE] = { > >>       [CSR_MHPMCOUNTER29H] = { "mhpmcounter29h", any32,  > read_zero }, > >>       [CSR_MHPMCOUNTER30H] = { "mhpmcounter30h", any32,  > read_zero }, > >>       [CSR_MHPMCOUNTER31H] = { "mhpmcounter31h", any32,  > read_zero }, > >> + > >> +    /* Machine Mode Core Level Interrupt Controller */ > >> +    [CSR_MINTSTATUS] = { "mintstatus", clic, read_mintstatus }, > >> + > >> +    /* Supervisor Mode Core Level Interrupt Controller */ > >> +    [CSR_SINTSTATUS] = { "sintstatus", clic, read_sintstatus }, > >> + > >>   #endif /* !CONFIG_USER_ONLY */ > >>   }; > >> -- > >> 2.25.1 > >> > >> > --------------5BBB9B2C501C837B9C24C1AE Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 8bit


On 2021/7/1 下午4:45, Frank Chang wrote:
LIU Zhiwei <zhiwei_liu@c-sky.com> 於 2021年4月20日 週二 上午8:49寫道：

On 2021/4/20 上午7:23, Alistair Francis wrote:
> On Fri, Apr 9, 2021 at 5:52 PM LIU Zhiwei <zhiwei_liu@c-sky.com> wrote:
>> CSR mintstatus holds the active interrupt level for each supported
>> privilege mode. sintstatus, and user, uintstatus, provide restricted
>> views of mintstatus.
>>
>> Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
>> ---
>>   target/riscv/cpu.h      |  2 ++
>>   target/riscv/cpu_bits.h | 11 +++++++++++
>>   target/riscv/csr.c      | 26 ++++++++++++++++++++++++++
>>   3 files changed, 39 insertions(+)
>>
>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>> index 0a33d387ba..1a44ca62c7 100644
>> --- a/target/riscv/cpu.h
>> +++ b/target/riscv/cpu.h
>> @@ -159,6 +159,7 @@ struct CPURISCVState {
>>       target_ulong mip;
>>
>>       uint32_t miclaim;
>> +    uint32_t mintstatus; /* clic-spec */
>>
>>       target_ulong mie;
>>       target_ulong mideleg;
>> @@ -243,6 +244,7 @@ struct CPURISCVState {
>>
>>       /* Fields from here on are preserved across CPU reset. */
>>       QEMUTimer *timer; /* Internal timer */
>> +    void *clic;       /* clic interrupt controller */
> This should be the CLIC type.

OK.

Actually there are many versions of CLIC in my branch as different
devices. But it is better to use CLIC type for the upstream version.

Hi Alistair and Zhiwei,

Replacing void *clic with RISCVCLICState *clic may create a circular loop
because CPURISCVState is also referenced in riscv_clic.h.

As there is only one reference to CPURISCVState,  it is not difficult to get rid of it.

However, I would like to ask what is the best approach to add
the reference of CLIC device in CPURISCVState struct?

There may be different kinds of CLIC devices.
AFAK, there was another RFC patchset trying to add void *eclic
for Nuclei processor into CPURISCVState struct:

Is it okay to add the device reference directly into CPURISCVState struct like that,
or we should create some abstraction for these CLIC devices?
(However, I'm not sure how big the differences are for these CLIC devices...)

In my opinion, we should be very cautious to include this kind of code,  although I suffer from it too.

We should only support the features defined in the specification. If some feature are neither defined
by the specification, nor  under the customized range specified by the specification,
we should review the code and exclude it from the mainline.

The standard implementation of an old, ratified  as drafted specification can be allowed in the mainline.
If a new specification comes,  and the old implementation can carefully avoid  the conflicting , it can still in
the mainline.  Otherwise, the old implementation should be obsoleted.

Thanks,
Zhiwei

Thanks,
Frank Chang
 

>
>>   };
>>
>>   OBJECT_DECLARE_TYPE(RISCVCPU, RISCVCPUClass,
>> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
>> index caf4599207..c4ce6ec3d9 100644
>> --- a/target/riscv/cpu_bits.h
>> +++ b/target/riscv/cpu_bits.h
>> @@ -165,6 +165,7 @@
>>   #define CSR_MCAUSE          0x342
>>   #define CSR_MTVAL           0x343
>>   #define CSR_MIP             0x344
>> +#define CSR_MINTSTATUS      0x346 /* clic-spec-draft */
>>
>>   /* Legacy Machine Trap Handling (priv v1.9.1) */
>>   #define CSR_MBADADDR        0x343
>> @@ -183,6 +184,7 @@
>>   #define CSR_SCAUSE          0x142
>>   #define CSR_STVAL           0x143
>>   #define CSR_SIP             0x144
>> +#define CSR_SINTSTATUS      0x146 /* clic-spec-draft */
>>
>>   /* Legacy Supervisor Trap Handling (priv v1.9.1) */
>>   #define CSR_SBADADDR        0x143
>> @@ -585,6 +587,15 @@
>>   #define SIP_STIP                           MIP_STIP
>>   #define SIP_SEIP                           MIP_SEIP
>>
>> +/* mintstatus */
>> +#define MINTSTATUS_MIL                     0xff000000 /* mil[7:0] */
>> +#define MINTSTATUS_SIL                     0x0000ff00 /* sil[7:0] */
>> +#define MINTSTATUS_UIL                     0x000000ff /* uil[7:0] */
>> +
>> +/* sintstatus */
>> +#define SINTSTATUS_SIL                     0x0000ff00 /* sil[7:0] */
>> +#define SINTSTATUS_UIL                     0x000000ff /* uil[7:0] */
> The bit fields in the comments are out of date.

I didn't notice it.   Fix it in next version.

Thanks.

Zhiwei

>
> Alistair
>
>> +
>>   /* MIE masks */
>>   #define MIE_SEIE                           (1 << IRQ_S_EXT)
>>   #define MIE_UEIE                           (1 << IRQ_U_EXT)
>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>> index d2585395bf..320b18ab60 100644
>> --- a/target/riscv/csr.c
>> +++ b/target/riscv/csr.c
>> @@ -188,6 +188,12 @@ static int pmp(CPURISCVState *env, int csrno)
>>   {
>>       return -!riscv_feature(env, RISCV_FEATURE_PMP);
>>   }
>> +
>> +static int clic(CPURISCVState *env, int csrno)
>> +{
>> +    return !!env->clic;
>> +}
>> +
>>   #endif
>>
>>   /* User Floating-Point CSRs */
>> @@ -734,6 +740,12 @@ static int rmw_mip(CPURISCVState *env, int csrno, target_ulong *ret_value,
>>       return 0;
>>   }
>>
>> +static int read_mintstatus(CPURISCVState *env, int csrno, target_ulong *val)
>> +{
>> +    *val = env->mintstatus;
>> +    return 0;
>> +}
>> +
>>   /* Supervisor Trap Setup */
>>   static int read_sstatus(CPURISCVState *env, int csrno, target_ulong *val)
>>   {
>> @@ -893,6 +905,13 @@ static int rmw_sip(CPURISCVState *env, int csrno, target_ulong *ret_value,
>>       return ret;
>>   }
>>
>> +static int read_sintstatus(CPURISCVState *env, int csrno, target_ulong *val)
>> +{
>> +    target_ulong mask = SINTSTATUS_SIL | SINTSTATUS_UIL;
>> +    *val = env->mintstatus & mask;
>> +    return 0;
>> +}
>> +
>>   /* Supervisor Protection and Translation */
>>   static int read_satp(CPURISCVState *env, int csrno, target_ulong *val)
>>   {
>> @@ -1644,5 +1663,12 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
>>       [CSR_MHPMCOUNTER29H] = { "mhpmcounter29h", any32,  read_zero },
>>       [CSR_MHPMCOUNTER30H] = { "mhpmcounter30h", any32,  read_zero },
>>       [CSR_MHPMCOUNTER31H] = { "mhpmcounter31h", any32,  read_zero },
>> +
>> +    /* Machine Mode Core Level Interrupt Controller */
>> +    [CSR_MINTSTATUS] = { "mintstatus", clic,  read_mintstatus },
>> +
>> +    /* Supervisor Mode Core Level Interrupt Controller */
>> +    [CSR_SINTSTATUS] = { "sintstatus", clic,  read_sintstatus },
>> +
>>   #endif /* !CONFIG_USER_ONLY */
>>   };
>> --
>> 2.25.1
>>
>>

--------------5BBB9B2C501C837B9C24C1AE-- From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1lytAh-0008DX-Ac for mharc-qemu-riscv@gnu.org; Thu, 01 Jul 2021 05:39:47 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:47664) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lytAe-0008CB-RN; Thu, 01 Jul 2021 05:39:44 -0400 Received: from out28-5.mail.aliyun.com ([115.124.28.5]:50551) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lytAa-0007LA-Fe; Thu, 01 Jul 2021 05:39:44 -0400 X-Alimail-AntiSpam: AC=CONTINUE; BC=0.07183115|-1; CH=green; DM=|CONTINUE|false|; DS=CONTINUE|ham_enroll_verification|0.444704-0.000262462-0.555034; FP=0|0|0|0|0|-1|-1|-1; HT=ay29a033018047190; MF=zhiwei_liu@c-sky.com; NM=1; PH=DS; RN=7; RT=7; SR=0; TI=SMTPD_---.KahuT9m_1625132372; Received: from 10.0.2.15(mailfrom:zhiwei_liu@c-sky.com fp:SMTPD_---.KahuT9m_1625132372) by smtp.aliyun-inc.com(10.147.41.158); Thu, 01 Jul 2021 17:39:32 +0800 Subject: Re: [RFC PATCH 01/11] target/riscv: Add CLIC CSR mintstatus To: Frank Chang Cc: Alistair Francis , Palmer Dabbelt , Alistair Francis , "open list:RISC-V" , "qemu-devel@nongnu.org Developers" , wxy194768@alibaba-inc.com References: <20210409074857.166082-1-zhiwei_liu@c-sky.com> <20210409074857.166082-2-zhiwei_liu@c-sky.com> <20304ba9-6f87-f7b9-a24d-15d4b3d3f75a@c-sky.com> From: LIU Zhiwei Message-ID: Date: Thu, 1 Jul 2021 17:38:10 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/alternative; boundary="------------5BBB9B2C501C837B9C24C1AE" Content-Language: en-US Received-SPF: none client-ip=115.124.28.5; envelope-from=zhiwei_liu@c-sky.com; helo=out28-5.mail.aliyun.com 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, HTML_MESSAGE=0.001, NICE_REPLY_A=-0.001, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H3=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_NONE=0.001, UNPARSEABLE_RELAY=0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-riscv@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 01 Jul 2021 09:39:45 -0000 This is a multi-part message in MIME format. --------------5BBB9B2C501C837B9C24C1AE Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit On 2021/7/1 下午4:45, Frank Chang wrote: > LIU Zhiwei > 於 > 2021年4月20日 週二 上午8:49寫道： > > > On 2021/4/20 上午7:23, Alistair Francis wrote: > > On Fri, Apr 9, 2021 at 5:52 PM LIU Zhiwei > wrote: > >> CSR mintstatus holds the active interrupt level for each supported > >> privilege mode. sintstatus, and user, uintstatus, provide > restricted > >> views of mintstatus. > >> > >> Signed-off-by: LIU Zhiwei > > >> --- > >>   target/riscv/cpu.h      |  2 ++ > >>   target/riscv/cpu_bits.h | 11 +++++++++++ > >>   target/riscv/csr.c      | 26 ++++++++++++++++++++++++++ > >>   3 files changed, 39 insertions(+) > >> > >> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > >> index 0a33d387ba..1a44ca62c7 100644 > >> --- a/target/riscv/cpu.h > >> +++ b/target/riscv/cpu.h > >> @@ -159,6 +159,7 @@ struct CPURISCVState { > >>       target_ulong mip; > >> > >>       uint32_t miclaim; > >> +    uint32_t mintstatus; /* clic-spec */ > >> > >>       target_ulong mie; > >>       target_ulong mideleg; > >> @@ -243,6 +244,7 @@ struct CPURISCVState { > >> > >>       /* Fields from here on are preserved across CPU reset. */ > >>       QEMUTimer *timer; /* Internal timer */ > >> +    void *clic;       /* clic interrupt controller */ > > This should be the CLIC type. > > OK. > > Actually there are many versions of CLIC in my branch as different > devices. But it is better to use CLIC type for the upstream version. > > > Hi Alistair and Zhiwei, > > Replacing void *clic with RISCVCLICState *clic may create a circular loop > because CPURISCVState is also referenced in riscv_clic.h. > As there is only one reference to CPURISCVState,  it is not difficult to get rid of it. > However, I would like to ask what is the best approach to add > the reference of CLIC device in CPURISCVState struct? > > There may be different kinds of CLIC devices. > AFAK, there was another RFC patchset trying to add void *eclic > for Nuclei processor into CPURISCVState struct: > https://patchwork.kernel.org/project/qemu-devel/patch/20210507081654.11056-2-wangjunqiang@iscas.ac.cn/ > > > Is it okay to add the device reference directly into CPURISCVState > struct like that, > or we should create some abstraction for these CLIC devices? > (However, I'm not sure how big the differences are for these CLIC > devices...) > In my opinion, we should be very cautious to include this kind of code,  although I suffer from it too. We should only support the features defined in the specification. If some feature are neither defined by the specification, nor  under the customized range specified by the specification, we should review the code and exclude it from the mainline. The standard implementation of an old, ratified  as drafted specification can be allowed in the mainline. If a new specification comes,  and the old implementation can carefully avoid  the conflicting , it can still in the mainline.  Otherwise, the old implementation should be obsoleted. Thanks, Zhiwei > Thanks, > Frank Chang > > > > > >>   }; > >> > >>   OBJECT_DECLARE_TYPE(RISCVCPU, RISCVCPUClass, > >> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h > >> index caf4599207..c4ce6ec3d9 100644 > >> --- a/target/riscv/cpu_bits.h > >> +++ b/target/riscv/cpu_bits.h > >> @@ -165,6 +165,7 @@ > >>   #define CSR_MCAUSE          0x342 > >>   #define CSR_MTVAL           0x343 > >>   #define CSR_MIP             0x344 > >> +#define CSR_MINTSTATUS      0x346 /* clic-spec-draft */ > >> > >>   /* Legacy Machine Trap Handling (priv v1.9.1) */ > >>   #define CSR_MBADADDR        0x343 > >> @@ -183,6 +184,7 @@ > >>   #define CSR_SCAUSE          0x142 > >>   #define CSR_STVAL           0x143 > >>   #define CSR_SIP             0x144 > >> +#define CSR_SINTSTATUS      0x146 /* clic-spec-draft */ > >> > >>   /* Legacy Supervisor Trap Handling (priv v1.9.1) */ > >>   #define CSR_SBADADDR        0x143 > >> @@ -585,6 +587,15 @@ > >>   #define SIP_STIP  MIP_STIP > >>   #define SIP_SEIP  MIP_SEIP > >> > >> +/* mintstatus */ > >> +#define MINTSTATUS_MIL  0xff000000 /* mil[7:0] */ > >> +#define MINTSTATUS_SIL  0x0000ff00 /* sil[7:0] */ > >> +#define MINTSTATUS_UIL  0x000000ff /* uil[7:0] */ > >> + > >> +/* sintstatus */ > >> +#define SINTSTATUS_SIL  0x0000ff00 /* sil[7:0] */ > >> +#define SINTSTATUS_UIL  0x000000ff /* uil[7:0] */ > > The bit fields in the comments are out of date. > > I didn't notice it.   Fix it in next version. > > Thanks. > > Zhiwei > > > > > Alistair > > > >> + > >>   /* MIE masks */ > >>   #define MIE_SEIE                           (1 << IRQ_S_EXT) > >>   #define MIE_UEIE                           (1 << IRQ_U_EXT) > >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c > >> index d2585395bf..320b18ab60 100644 > >> --- a/target/riscv/csr.c > >> +++ b/target/riscv/csr.c > >> @@ -188,6 +188,12 @@ static int pmp(CPURISCVState *env, int csrno) > >>   { > >>       return -!riscv_feature(env, RISCV_FEATURE_PMP); > >>   } > >> + > >> +static int clic(CPURISCVState *env, int csrno) > >> +{ > >> +    return !!env->clic; > >> +} > >> + > >>   #endif > >> > >>   /* User Floating-Point CSRs */ > >> @@ -734,6 +740,12 @@ static int rmw_mip(CPURISCVState *env, int > csrno, target_ulong *ret_value, > >>       return 0; > >>   } > >> > >> +static int read_mintstatus(CPURISCVState *env, int csrno, > target_ulong *val) > >> +{ > >> +    *val = env->mintstatus; > >> +    return 0; > >> +} > >> + > >>   /* Supervisor Trap Setup */ > >>   static int read_sstatus(CPURISCVState *env, int csrno, > target_ulong *val) > >>   { > >> @@ -893,6 +905,13 @@ static int rmw_sip(CPURISCVState *env, int > csrno, target_ulong *ret_value, > >>       return ret; > >>   } > >> > >> +static int read_sintstatus(CPURISCVState *env, int csrno, > target_ulong *val) > >> +{ > >> +    target_ulong mask = SINTSTATUS_SIL | SINTSTATUS_UIL; > >> +    *val = env->mintstatus & mask; > >> +    return 0; > >> +} > >> + > >>   /* Supervisor Protection and Translation */ > >>   static int read_satp(CPURISCVState *env, int csrno, > target_ulong *val) > >>   { > >> @@ -1644,5 +1663,12 @@ riscv_csr_operations > csr_ops[CSR_TABLE_SIZE] = { > >>       [CSR_MHPMCOUNTER29H] = { "mhpmcounter29h", any32,  > read_zero }, > >>       [CSR_MHPMCOUNTER30H] = { "mhpmcounter30h", any32,  > read_zero }, > >>       [CSR_MHPMCOUNTER31H] = { "mhpmcounter31h", any32,  > read_zero }, > >> + > >> +    /* Machine Mode Core Level Interrupt Controller */ > >> +    [CSR_MINTSTATUS] = { "mintstatus", clic, read_mintstatus }, > >> + > >> +    /* Supervisor Mode Core Level Interrupt Controller */ > >> +    [CSR_SINTSTATUS] = { "sintstatus", clic, read_sintstatus }, > >> + > >>   #endif /* !CONFIG_USER_ONLY */ > >>   }; > >> -- > >> 2.25.1 > >> > >> > --------------5BBB9B2C501C837B9C24C1AE Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 8bit


On 2021/7/1 下午4:45, Frank Chang wrote:
LIU Zhiwei <zhiwei_liu@c-sky.com> 於 2021年4月20日 週二 上午8:49寫道：

On 2021/4/20 上午7:23, Alistair Francis wrote:
> On Fri, Apr 9, 2021 at 5:52 PM LIU Zhiwei <zhiwei_liu@c-sky.com> wrote:
>> CSR mintstatus holds the active interrupt level for each supported
>> privilege mode. sintstatus, and user, uintstatus, provide restricted
>> views of mintstatus.
>>
>> Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
>> ---
>>   target/riscv/cpu.h      |  2 ++
>>   target/riscv/cpu_bits.h | 11 +++++++++++
>>   target/riscv/csr.c      | 26 ++++++++++++++++++++++++++
>>   3 files changed, 39 insertions(+)
>>
>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>> index 0a33d387ba..1a44ca62c7 100644
>> --- a/target/riscv/cpu.h
>> +++ b/target/riscv/cpu.h
>> @@ -159,6 +159,7 @@ struct CPURISCVState {
>>       target_ulong mip;
>>
>>       uint32_t miclaim;
>> +    uint32_t mintstatus; /* clic-spec */
>>
>>       target_ulong mie;
>>       target_ulong mideleg;
>> @@ -243,6 +244,7 @@ struct CPURISCVState {
>>
>>       /* Fields from here on are preserved across CPU reset. */
>>       QEMUTimer *timer; /* Internal timer */
>> +    void *clic;       /* clic interrupt controller */
> This should be the CLIC type.

OK.

Actually there are many versions of CLIC in my branch as different
devices. But it is better to use CLIC type for the upstream version.

Hi Alistair and Zhiwei,

Replacing void *clic with RISCVCLICState *clic may create a circular loop
because CPURISCVState is also referenced in riscv_clic.h.

As there is only one reference to CPURISCVState,  it is not difficult to get rid of it.

However, I would like to ask what is the best approach to add
the reference of CLIC device in CPURISCVState struct?

There may be different kinds of CLIC devices.
AFAK, there was another RFC patchset trying to add void *eclic
for Nuclei processor into CPURISCVState struct:

Is it okay to add the device reference directly into CPURISCVState struct like that,
or we should create some abstraction for these CLIC devices?
(However, I'm not sure how big the differences are for these CLIC devices...)

In my opinion, we should be very cautious to include this kind of code,  although I suffer from it too.

We should only support the features defined in the specification. If some feature are neither defined
by the specification, nor  under the customized range specified by the specification,
we should review the code and exclude it from the mainline.

The standard implementation of an old, ratified  as drafted specification can be allowed in the mainline.
If a new specification comes,  and the old implementation can carefully avoid  the conflicting , it can still in
the mainline.  Otherwise, the old implementation should be obsoleted.

Thanks,
Zhiwei

Thanks,
Frank Chang
 

>
>>   };
>>
>>   OBJECT_DECLARE_TYPE(RISCVCPU, RISCVCPUClass,
>> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
>> index caf4599207..c4ce6ec3d9 100644
>> --- a/target/riscv/cpu_bits.h
>> +++ b/target/riscv/cpu_bits.h
>> @@ -165,6 +165,7 @@
>>   #define CSR_MCAUSE          0x342
>>   #define CSR_MTVAL           0x343
>>   #define CSR_MIP             0x344
>> +#define CSR_MINTSTATUS      0x346 /* clic-spec-draft */
>>
>>   /* Legacy Machine Trap Handling (priv v1.9.1) */
>>   #define CSR_MBADADDR        0x343
>> @@ -183,6 +184,7 @@
>>   #define CSR_SCAUSE          0x142
>>   #define CSR_STVAL           0x143
>>   #define CSR_SIP             0x144
>> +#define CSR_SINTSTATUS      0x146 /* clic-spec-draft */
>>
>>   /* Legacy Supervisor Trap Handling (priv v1.9.1) */
>>   #define CSR_SBADADDR        0x143
>> @@ -585,6 +587,15 @@
>>   #define SIP_STIP                           MIP_STIP
>>   #define SIP_SEIP                           MIP_SEIP
>>
>> +/* mintstatus */
>> +#define MINTSTATUS_MIL                     0xff000000 /* mil[7:0] */
>> +#define MINTSTATUS_SIL                     0x0000ff00 /* sil[7:0] */
>> +#define MINTSTATUS_UIL                     0x000000ff /* uil[7:0] */
>> +
>> +/* sintstatus */
>> +#define SINTSTATUS_SIL                     0x0000ff00 /* sil[7:0] */
>> +#define SINTSTATUS_UIL                     0x000000ff /* uil[7:0] */
> The bit fields in the comments are out of date.

I didn't notice it.   Fix it in next version.

Thanks.

Zhiwei

>
> Alistair
>
>> +
>>   /* MIE masks */
>>   #define MIE_SEIE                           (1 << IRQ_S_EXT)
>>   #define MIE_UEIE                           (1 << IRQ_U_EXT)
>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>> index d2585395bf..320b18ab60 100644
>> --- a/target/riscv/csr.c
>> +++ b/target/riscv/csr.c
>> @@ -188,6 +188,12 @@ static int pmp(CPURISCVState *env, int csrno)
>>   {
>>       return -!riscv_feature(env, RISCV_FEATURE_PMP);
>>   }
>> +
>> +static int clic(CPURISCVState *env, int csrno)
>> +{
>> +    return !!env->clic;
>> +}
>> +
>>   #endif
>>
>>   /* User Floating-Point CSRs */
>> @@ -734,6 +740,12 @@ static int rmw_mip(CPURISCVState *env, int csrno, target_ulong *ret_value,
>>       return 0;
>>   }
>>
>> +static int read_mintstatus(CPURISCVState *env, int csrno, target_ulong *val)
>> +{
>> +    *val = env->mintstatus;
>> +    return 0;
>> +}
>> +
>>   /* Supervisor Trap Setup */
>>   static int read_sstatus(CPURISCVState *env, int csrno, target_ulong *val)
>>   {
>> @@ -893,6 +905,13 @@ static int rmw_sip(CPURISCVState *env, int csrno, target_ulong *ret_value,
>>       return ret;
>>   }
>>
>> +static int read_sintstatus(CPURISCVState *env, int csrno, target_ulong *val)
>> +{
>> +    target_ulong mask = SINTSTATUS_SIL | SINTSTATUS_UIL;
>> +    *val = env->mintstatus & mask;
>> +    return 0;
>> +}
>> +
>>   /* Supervisor Protection and Translation */
>>   static int read_satp(CPURISCVState *env, int csrno, target_ulong *val)
>>   {
>> @@ -1644,5 +1663,12 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
>>       [CSR_MHPMCOUNTER29H] = { "mhpmcounter29h", any32,  read_zero },
>>       [CSR_MHPMCOUNTER30H] = { "mhpmcounter30h", any32,  read_zero },
>>       [CSR_MHPMCOUNTER31H] = { "mhpmcounter31h", any32,  read_zero },
>> +
>> +    /* Machine Mode Core Level Interrupt Controller */
>> +    [CSR_MINTSTATUS] = { "mintstatus", clic,  read_mintstatus },
>> +
>> +    /* Supervisor Mode Core Level Interrupt Controller */
>> +    [CSR_SINTSTATUS] = { "sintstatus", clic,  read_sintstatus },
>> +
>>   #endif /* !CONFIG_USER_ONLY */
>>   };
>> --
>> 2.25.1
>>
>>

--------------5BBB9B2C501C837B9C24C1AE--