[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH 08/11] target/riscv: Update CSR xnxti in CLIC mode
|
From: |
Alistair Francis |
|
Subject: |
Re: [RFC PATCH 08/11] target/riscv: Update CSR xnxti in CLIC mode |
|
Date: |
Tue, 15 Jun 2021 17:45:39 +1000 |
On Fri, Jun 11, 2021 at 7:08 PM Frank Chang <0xc0de0125@gmail.com> wrote:
>
> LIU Zhiwei <zhiwei_liu@c-sky.com> 於 2021年6月11日 週五 下午4:56寫道：
>>
>>
>> On 6/11/21 4:42 PM, Frank Chang wrote:
>>
>> LIU Zhiwei <zhiwei_liu@c-sky.com> 於 2021年6月11日 週五 下午4:30寫道：
>>>
>>>
>>> On 6/11/21 4:15 PM, Frank Chang wrote:
>>>
>>> LIU Zhiwei <zhiwei_liu@c-sky.com> 於 2021年4月9日 週五 下午3:52寫道：
>>>>
>>>> The CSR can be used by software to service the next horizontal interrupt
>>>> when it has greater level than the saved interrupt context
>>>> (held in xcause`.pil`) and greater level than the interrupt threshold of
>>>> the corresponding privilege mode,
>>>>
>>>> Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
>>>> ---
>>>> target/riscv/cpu_bits.h | 16 ++++++
>>>> target/riscv/csr.c | 114 ++++++++++++++++++++++++++++++++++++++++
>>>> 2 files changed, 130 insertions(+)
>>>>
>>>> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
>>>> index 7922097776..494e41edc9 100644
>>>> --- a/target/riscv/cpu_bits.h
>>>> +++ b/target/riscv/cpu_bits.h
>>>> @@ -166,6 +166,7 @@
>>>> #define CSR_MCAUSE 0x342
>>>> #define CSR_MTVAL 0x343
>>>> #define CSR_MIP 0x344
>>>> +#define CSR_MNXTI 0x345 /* clic-spec-draft */
>>>> #define CSR_MINTSTATUS 0x346 /* clic-spec-draft */
>>>> #define CSR_MINTTHRESH 0x347 /* clic-spec-draft */
>>>>
>>>> @@ -187,6 +188,7 @@
>>>> #define CSR_SCAUSE 0x142
>>>> #define CSR_STVAL 0x143
>>>> #define CSR_SIP 0x144
>>>> +#define CSR_SNXTI 0x145 /* clic-spec-draft */
>>>> #define CSR_SINTSTATUS 0x146 /* clic-spec-draft */
>>>> #define CSR_SINTTHRESH 0x147 /* clic-spec-draft */
>>>>
>>>> @@ -596,10 +598,24 @@
>>>> #define MINTSTATUS_SIL 0x0000ff00 /* sil[7:0] */
>>>> #define MINTSTATUS_UIL 0x000000ff /* uil[7:0] */
>>>>
>>>> +/* mcause */
>>>> +#define MCAUSE_MINHV 0x40000000 /* minhv */
>>>> +#define MCAUSE_MPP 0x30000000 /* mpp[1:0] */
>>>> +#define MCAUSE_MPIE 0x08000000 /* mpie */
>>>> +#define MCAUSE_MPIL 0x00ff0000 /* mpil[7:0] */
>>>> +#define MCAUSE_EXCCODE 0x00000fff /* exccode[11:0] */
>>>> +
>>>> /* sintstatus */
>>>> #define SINTSTATUS_SIL 0x0000ff00 /* sil[7:0] */
>>>> #define SINTSTATUS_UIL 0x000000ff /* uil[7:0] */
>>>>
>>>> +/* scause */
>>>> +#define SCAUSE_SINHV 0x40000000 /* sinhv */
>>>> +#define SCAUSE_SPP 0x10000000 /* spp */
>>>> +#define SCAUSE_SPIE 0x08000000 /* spie */
>>>> +#define SCAUSE_SPIL 0x00ff0000 /* spil[7:0] */
>>>> +#define SCAUSE_EXCCODE 0x00000fff /* exccode[11:0] */
>>>> +
>>>> /* 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 e12222b77f..72cba080bf 100644
>>>> --- a/target/riscv/csr.c
>>>> +++ b/target/riscv/csr.c
>>>> @@ -774,6 +774,80 @@ static int rmw_mip(CPURISCVState *env, int csrno,
>>>> target_ulong *ret_value,
>>>> return 0;
>>>> }
>>>>
>>>> +static bool get_xnxti_status(CPURISCVState *env)
>>>> +{
>>>> + CPUState *cs = env_cpu(env);
>>>> + int clic_irq, clic_priv, clic_il, pil;
>>>> +
>>>> + if (!env->exccode) { /* No interrupt */
>>>> + return false;
>>>> + }
>>>> + /* The system is not in a CLIC mode */
>>>> + if (!riscv_clic_is_clic_mode(env)) {
>>>> + return false;
>>>> + } else {
>>>> + riscv_clic_decode_exccode(env->exccode, &clic_priv, &clic_il,
>>>> + &clic_irq);
>>>> +
>>>> + if (env->priv == PRV_M) {
>>>> + pil = MAX(get_field(env->mcause, MCAUSE_MPIL),
>>>> env->mintthresh);
>>>> + } else if (env->priv == PRV_S) {
>>>> + pil = MAX(get_field(env->scause, SCAUSE_SPIL),
>>>> env->sintthresh);
>>>> + } else {
>>>> + qemu_log_mask(LOG_GUEST_ERROR,
>>>> + "CSR: rmw xnxti with unsupported mode\n");
>>>> + exit(1);
>>>> + }
>>>> +
>>>> + if ((clic_priv != env->priv) || /* No horizontal interrupt */
>>>> + (clic_il <= pil) || /* No higher level interrupt */
>>>> + (riscv_clic_shv_interrupt(env->clic, clic_priv, cs->cpu_index,
>>>> + clic_irq))) { /* CLIC vector mode */
>>>> + return false;
>>>> + } else {
>>>> + return true;
>>>> + }
>>>> + }
>>>> +}
>>>> +
>>>> +static int rmw_mnxti(CPURISCVState *env, int csrno, target_ulong
>>>> *ret_value,
>>>> + target_ulong new_value, target_ulong write_mask)
>>>> +{
>>>> + int clic_priv, clic_il, clic_irq;
>>>> + bool ready;
>>>> + CPUState *cs = env_cpu(env);
>>>> + if (write_mask) {
>>>> + env->mstatus |= new_value & (write_mask & 0b11111);
>>>> + }
>>>> +
>>>> + qemu_mutex_lock_iothread();
>>>
>>>
>>> Hi Zhiwei,
>>>
>>> May I ask what's the purpose to request the BQL here with
>>> qemu_mutex_lock_iothread()?
>>> Is there any critical data we need to protect in rmw_mnxti()?
>>> As far I see, rmw_mnxti() won't call cpu_interrupt() which need BQL to be
>>> held before calling.
>>> Am I missing anything?
>>>
>>> In my opinion, if you read or write any MMIO register, you need to hold
>>> the BQL. As you can quickly see,
>>> it calls riscv_clic_clean_pending. That's why it should hold the BQL.
>>>
>>> Zhiwei
>>
>>
>> Oh, I see.
>> The MMIO register reads and writes should also be protected by BQL.
>> Thanks for the explanation.
>>
>> I am glad to know that you are reviewing this patch set. As Sifive
>> implements the initial v0.7 CLIC, I think you may need this patch set for
>> your SOC.
>> If you like to, I am happy to see that you connect this patch set to your
>> SOC, and resend it again. I can also provide the qtest of this patch set if
>> you need.
>>
>> As you may see, the v6.1 soft freeze will come in July. I am afraid I can't
>> upstream a new SOC in so short time.
>>
>> Zhiwei
>
> Thanks, I think we will leverage the hard works you have done into our
> implementation.
> However, I'm not sure I can catch up the deadline before v6.1's soft-freeze.
> But I think I can help to review the patches, at least we can speed up the
> review process.
I just wanted to point out that in terms of *submitting* RISC-V
patches for QEMU, don't worry about the soft-freeze dates.
Feel free to send the patches during the freeze and they can be
reviewed (but not merged, unless they fix a bug). I will even apply
them to a local tree in preparation for the next merge window, after
the release.
Alistair
>
> Regarding qtest, I saw your head commit mentioned the repo you are using: [1].
> Is it okay to just grab the qtest from this repo?
>
> [1]:
> https://github.com/romanheros/qemu/commit/bce1845ea9b079b4c360440292dc47725d1b24ab
>
> Thanks,
> Frank Chang
>
>>
>>
>> Regards,
>> Frank Chang
>>
>>>
>>>
>>> Regard,
>>> Frank Chang
>>>
>>>>
>>>> + ready = get_xnxti_status(env);
>>>> + if (ready) {
>>>> + riscv_clic_decode_exccode(env->exccode, &clic_priv, &clic_il,
>>>> + &clic_irq);
>>>> + if (write_mask) {
>>>> + bool edge = riscv_clic_edge_triggered(env->clic, clic_priv,
>>>> + cs->cpu_index,
>>>> clic_irq);
>>>> + if (edge) {
>>>> + riscv_clic_clean_pending(env->clic, clic_priv,
>>>> + cs->cpu_index, clic_irq);
>>>> + }
>>>> + env->mintstatus = set_field(env->mintstatus,
>>>> + MINTSTATUS_MIL, clic_il);
>>>> + env->mcause = set_field(env->mcause, MCAUSE_EXCCODE,
>>>> clic_irq);
>>>> + }
>>>> + if (ret_value) {
>>>> + *ret_value = (env->mtvt & ~0x3f) + sizeof(target_ulong) *
>>>> clic_irq;
>>>> + }
>>>> + } else {
>>>> + if (ret_value) {
>>>> + *ret_value = 0;
>>>> + }
>>>> + }
>>>> + qemu_mutex_unlock_iothread();
>>>> + return 0;
>>>> +}
>>>> +
>>>> static int read_mintstatus(CPURISCVState *env, int csrno, target_ulong
>>>> *val)
>>>> {
>>>> *val = env->mintstatus;
>>>> @@ -982,6 +1056,44 @@ static int rmw_sip(CPURISCVState *env, int csrno,
>>>> target_ulong *ret_value,
>>>> return ret;
>>>> }
>>>>
>>>> +static int rmw_snxti(CPURISCVState *env, int csrno, target_ulong
>>>> *ret_value,
>>>> + target_ulong new_value, target_ulong write_mask)
>>>> +{
>>>> + int clic_priv, clic_il, clic_irq;
>>>> + bool ready;
>>>> + CPUState *cs = env_cpu(env);
>>>> + if (write_mask) {
>>>> + env->mstatus |= new_value & (write_mask & 0b11111);
>>>> + }
>>>> +
>>>> + qemu_mutex_lock_iothread();
>>>> + ready = get_xnxti_status(env);
>>>> + if (ready) {
>>>> + riscv_clic_decode_exccode(env->exccode, &clic_priv, &clic_il,
>>>> + &clic_irq);
>>>> + if (write_mask) {
>>>> + bool edge = riscv_clic_edge_triggered(env->clic, clic_priv,
>>>> + cs->cpu_index,
>>>> clic_irq);
>>>> + if (edge) {
>>>> + riscv_clic_clean_pending(env->clic, clic_priv,
>>>> + cs->cpu_index, clic_irq);
>>>> + }
>>>> + env->mintstatus = set_field(env->mintstatus,
>>>> + MINTSTATUS_SIL, clic_il);
>>>> + env->scause = set_field(env->scause, SCAUSE_EXCCODE,
>>>> clic_irq);
>>>> + }
>>>> + if (ret_value) {
>>>> + *ret_value = (env->stvt & ~0x3f) + sizeof(target_ulong) *
>>>> clic_irq;
>>>> + }
>>>> + } else {
>>>> + if (ret_value) {
>>>> + *ret_value = 0;
>>>> + }
>>>> + }
>>>> + qemu_mutex_unlock_iothread();
>>>> + return 0;
>>>> +}
>>>> +
>>>> static int read_sintstatus(CPURISCVState *env, int csrno, target_ulong
>>>> *val)
>>>> {
>>>> target_ulong mask = SINTSTATUS_SIL | SINTSTATUS_UIL;
>>>> @@ -1755,6 +1867,7 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
>>>>
>>>> /* Machine Mode Core Level Interrupt Controller */
>>>> [CSR_MTVT] = { "mtvt", clic, read_mtvt, write_mtvt },
>>>> + [CSR_MNXTI] = { "mnxti", clic, NULL, NULL, rmw_mnxti },
>>>> [CSR_MINTSTATUS] = { "mintstatus", clic, read_mintstatus },
>>>> [CSR_MINTTHRESH] = { "mintthresh", clic, read_mintthresh,
>>>> write_mintthresh },
>>>> @@ -1766,6 +1879,7 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
>>>>
>>>> /* Supervisor Mode Core Level Interrupt Controller */
>>>> [CSR_STVT] = { "stvt", clic, read_stvt, write_stvt },
>>>> + [CSR_SNXTI] = { "snxti", clic, NULL, NULL, rmw_snxti },
>>>>
>>>> #endif /* !CONFIG_USER_ONLY */
>>>> };
>>>> --
>>>> 2.25.1
>>>>
>>>>