RE: [PATCH] RISC-V: Remove riscv_cpu_claim_interrupts()

[張哲嘉]

Hi Zhiwei,

> -----Original Message-----
> From: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
> Sent: Monday, July 29, 2024 1:47 PM
> To: Alvin Che-Chia Chang(張哲嘉) <alvinga@andestech.com>;
> qemu-riscv@nongnu.org; qemu-devel@nongnu.org
> Cc: alistair.francis@wdc.com; bin.meng@windriver.com;
> liwei1518@gmail.com; dbarboza@ventanamicro.com
> Subject: Re: [PATCH] RISC-V: Remove riscv_cpu_claim_interrupts()
>
> [EXTERNAL MAIL]
>
> On 2024/7/27 12:27, Alvin Chang wrote:
> > The function of riscv_cpu_claim_interrupts() was introduced in commit
> > e3e7039 ("RISC-V: Allow interrupt controllers to claim interrupts") to
> > enforce hardware controlled of SEIP signal when there is an attached
> > external interrupt controller.
> >
> > In later RISC-V privileged specification version 1.10, SEIP became
> > software-writable, even if there is an attached external interrupt
> > controller. Thus, the commit 33fe584 ("target/riscv: Allow software
> > access to MIP SEIP") was introduced to remove that limitation, and it
> > also removed the usage of "miclaim" mask.
> >
> > It seems the function of riscv_cpu_claim_interrupts() is no longer used.
> > Therefore, we remove it in this commit.
> As MTIP/MSIP/MEIP in mip are still read-only fields in mip. I think we should
> not remove it.

IIUC, riscv_cpu_claim_interrupts() was used to achieve exclusive control 
between external interrupt controller and software.
It is not used to check the read-only fields such as MTIP/MSIP/MEIP in mip.
Moreover, env->miclaim is no longer used in latest code.

> Perhaps we should  add an assert for read-only fields for this function.

I agree. Maybe we can add relevant assertions for those fields in rmw_mip64() ?


Best regards,
Alvin

>
> Thanks,
> Zhiwei
>
> >
> > Signed-off-by: Alvin Chang <alvinga@andestech.com>
> > ---
> >   hw/intc/riscv_aclint.c    | 20 --------------------
> >   hw/intc/riscv_aplic.c     | 11 -----------
> >   hw/intc/riscv_imsic.c     |  8 --------
> >   hw/intc/sifive_plic.c     | 15 ---------------
> >   target/riscv/cpu.c        |  1 -
> >   target/riscv/cpu.h        |  3 ---
> >   target/riscv/cpu_helper.c | 11 -----------
> >   target/riscv/machine.c    |  1 -
> >   8 files changed, 70 deletions(-)
> >
> > diff --git a/hw/intc/riscv_aclint.c b/hw/intc/riscv_aclint.c index
> > e9f0536b1c..54cf69dada 100644
> > --- a/hw/intc/riscv_aclint.c
> > +++ b/hw/intc/riscv_aclint.c
> > @@ -280,7 +280,6 @@ static Property riscv_aclint_mtimer_properties[] = {
> >   static void riscv_aclint_mtimer_realize(DeviceState *dev, Error **errp)
> >   {
> >       RISCVAclintMTimerState *s = RISCV_ACLINT_MTIMER(dev);
> > -    int i;
> >
> >       memory_region_init_io(&s->mmio, OBJECT(dev),
> &riscv_aclint_mtimer_ops,
> >                             s, TYPE_RISCV_ACLINT_MTIMER,
> > s->aperture_size); @@ -291,14 +290,6 @@ static void
> > riscv_aclint_mtimer_realize(DeviceState *dev, Error **errp)
> >
> >       s->timers = g_new0(QEMUTimer *, s->num_harts);
> >       s->timecmp = g_new0(uint64_t, s->num_harts);
> > -    /* Claim timer interrupt bits */
> > -    for (i = 0; i < s->num_harts; i++) {
> > -        RISCVCPU *cpu = RISCV_CPU(cpu_by_arch_id(s->hartid_base + i));
> > -        if (riscv_cpu_claim_interrupts(cpu, MIP_MTIP) < 0) {
> > -            error_report("MTIP already claimed");
> > -            exit(1);
> > -        }
> > -    }
> >   }
> >
> >   static void riscv_aclint_mtimer_reset_enter(Object *obj, ResetType
> > type) @@ -472,7 +463,6 @@ static Property riscv_aclint_swi_properties[] = {
> >   static void riscv_aclint_swi_realize(DeviceState *dev, Error **errp)
> >   {
> >       RISCVAclintSwiState *swi = RISCV_ACLINT_SWI(dev);
> > -    int i;
> >
> >       memory_region_init_io(&swi->mmio, OBJECT(dev),
> &riscv_aclint_swi_ops, swi,
> >                             TYPE_RISCV_ACLINT_SWI,
> > RISCV_ACLINT_SWI_SIZE); @@ -480,16 +470,6 @@ static void
> > riscv_aclint_swi_realize(DeviceState *dev, Error **errp)
> >
> >       swi->soft_irqs = g_new(qemu_irq, swi->num_harts);
> >       qdev_init_gpio_out(dev, swi->soft_irqs, swi->num_harts);
> > -
> > -    /* Claim software interrupt bits */
> > -    for (i = 0; i < swi->num_harts; i++) {
> > -        RISCVCPU *cpu = RISCV_CPU(qemu_get_cpu(swi->hartid_base +
> i));
> > -        /* We don't claim mip.SSIP because it is writable by software */
> > -        if (riscv_cpu_claim_interrupts(cpu, swi->sswi ? 0 : MIP_MSIP) < 0)
> {
> > -            error_report("MSIP already claimed");
> > -            exit(1);
> > -        }
> > -    }
> >   }
> >
> >   static void riscv_aclint_swi_reset_enter(Object *obj, ResetType
> > type) diff --git a/hw/intc/riscv_aplic.c b/hw/intc/riscv_aplic.c index
> > 32edd6d07b..cde8337542 100644
> > --- a/hw/intc/riscv_aplic.c
> > +++ b/hw/intc/riscv_aplic.c
> > @@ -873,17 +873,6 @@ static void riscv_aplic_realize(DeviceState *dev,
> Error **errp)
> >       if (!aplic->msimode) {
> >           aplic->external_irqs = g_malloc(sizeof(qemu_irq) *
> aplic->num_harts);
> >           qdev_init_gpio_out(dev, aplic->external_irqs,
> > aplic->num_harts);
> > -
> > -        /* Claim the CPU interrupt to be triggered by this APLIC */
> > -        for (i = 0; i < aplic->num_harts; i++) {
> > -            RISCVCPU *cpu =
> RISCV_CPU(cpu_by_arch_id(aplic->hartid_base + i));
> > -            if (riscv_cpu_claim_interrupts(cpu,
> > -                (aplic->mmode) ? MIP_MEIP : MIP_SEIP) < 0) {
> > -                error_report("%s already claimed",
> > -                             (aplic->mmode) ? "MEIP" : "SEIP");
> > -                exit(1);
> > -            }
> > -        }
> >       }
> >
> >       msi_nonbroken = true;
> > diff --git a/hw/intc/riscv_imsic.c b/hw/intc/riscv_imsic.c index
> > b90f0d731d..8c61a5f28b 100644
> > --- a/hw/intc/riscv_imsic.c
> > +++ b/hw/intc/riscv_imsic.c
> > @@ -347,14 +347,6 @@ static void riscv_imsic_realize(DeviceState *dev,
> Error **errp)
> >                             IMSIC_MMIO_SIZE(imsic->num_pages));
> >       sysbus_init_mmio(SYS_BUS_DEVICE(dev), &imsic->mmio);
> >
> > -    /* Claim the CPU interrupt to be triggered by this IMSIC */
> > -    if (riscv_cpu_claim_interrupts(rcpu,
> > -            (imsic->mmode) ? MIP_MEIP : MIP_SEIP) < 0) {
> > -        error_setg(errp, "%s already claimed",
> > -                   (imsic->mmode) ? "MEIP" : "SEIP");
> > -        return;
> > -    }
> > -
> >       /* Create output IRQ lines */
> >       imsic->external_irqs = g_malloc(sizeof(qemu_irq) *
> imsic->num_pages);
> >       qdev_init_gpio_out(dev, imsic->external_irqs, imsic->num_pages);
> > diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c index
> > e559f11805..f0f3dcce1f 100644
> > --- a/hw/intc/sifive_plic.c
> > +++ b/hw/intc/sifive_plic.c
> > @@ -356,7 +356,6 @@ static void sifive_plic_irq_request(void *opaque, int
> irq, int level)
> >   static void sifive_plic_realize(DeviceState *dev, Error **errp)
> >   {
> >       SiFivePLICState *s = SIFIVE_PLIC(dev);
> > -    int i;
> >
> >       memory_region_init_io(&s->mmio, OBJECT(dev), &sifive_plic_ops, s,
> >                             TYPE_SIFIVE_PLIC, s->aperture_size); @@
> > -385,20 +384,6 @@ static void sifive_plic_realize(DeviceState *dev, Error
> **errp)
> >       s->m_external_irqs = g_malloc(sizeof(qemu_irq) * s->num_harts);
> >       qdev_init_gpio_out(dev, s->m_external_irqs, s->num_harts);
> >
> > -    /*
> > -     * We can't allow the supervisor to control SEIP as this would allow 
> > the
> > -     * supervisor to clear a pending external interrupt which will result 
> > in
> > -     * lost a interrupt in the case a PLIC is attached. The SEIP bit must 
> > be
> > -     * hardware controlled when a PLIC is attached.
> > -     */
> > -    for (i = 0; i < s->num_harts; i++) {
> > -        RISCVCPU *cpu = RISCV_CPU(qemu_get_cpu(s->hartid_base + i));
> > -        if (riscv_cpu_claim_interrupts(cpu, MIP_SEIP) < 0) {
> > -            error_setg(errp, "SEIP already claimed");
> > -            return;
> > -        }
> > -    }
> > -
> >       msi_nonbroken = true;
> >   }
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index
> > a90808a3ba..19feb032d6 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -967,7 +967,6 @@ static void riscv_cpu_reset_hold(Object *obj,
> ResetType type)
> >           }
> >       }
> >       env->mcause = 0;
> > -    env->miclaim = MIP_SGEIP;
> >       env->pc = env->resetvec;
> >       env->bins = 0;
> >       env->two_stage_lookup = false;
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h index
> > 1619c3acb6..6277979afd 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -258,8 +258,6 @@ struct CPUArchState {
> >       bool external_seip;
> >       bool software_seip;
> >
> > -    uint64_t miclaim;
> > -
> >       uint64_t mie;
> >       uint64_t mideleg;
> >
> > @@ -565,7 +563,6 @@ void riscv_cpu_do_transaction_failed(CPUState *cs,
> hwaddr physaddr,
> >   hwaddr riscv_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
> >   bool riscv_cpu_exec_interrupt(CPUState *cs, int interrupt_request);
> >   void riscv_cpu_swap_hypervisor_regs(CPURISCVState *env); -int
> > riscv_cpu_claim_interrupts(RISCVCPU *cpu, uint64_t interrupts);
> >   uint64_t riscv_cpu_update_mip(CPURISCVState *env, uint64_t mask,
> >                                 uint64_t value);
> >   void riscv_cpu_interrupt(CPURISCVState *env); diff --git
> > a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c index
> > 395a1d9140..bcafa55acd 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -619,17 +619,6 @@ void riscv_cpu_set_geilen(CPURISCVState *env,
> target_ulong geilen)
> >       env->geilen = geilen;
> >   }
> >
> > -int riscv_cpu_claim_interrupts(RISCVCPU *cpu, uint64_t interrupts) -{
> > -    CPURISCVState *env = &cpu->env;
> > -    if (env->miclaim & interrupts) {
> > -        return -1;
> > -    } else {
> > -        env->miclaim |= interrupts;
> > -        return 0;
> > -    }
> > -}
> > -
> >   void riscv_cpu_interrupt(CPURISCVState *env)
> >   {
> >       uint64_t gein, vsgein = 0, vstip = 0, irqf = 0; diff --git
> > a/target/riscv/machine.c b/target/riscv/machine.c index
> > 492c2c6d9d..0eabb6c076 100644
> > --- a/target/riscv/machine.c
> > +++ b/target/riscv/machine.c
> > @@ -378,7 +378,6 @@ const VMStateDescription vmstate_riscv_cpu = {
> >           VMSTATE_UINTTL(env.mhartid, RISCVCPU),
> >           VMSTATE_UINT64(env.mstatus, RISCVCPU),
> >           VMSTATE_UINT64(env.mip, RISCVCPU),
> > -        VMSTATE_UINT64(env.miclaim, RISCVCPU),
> >           VMSTATE_UINT64(env.mie, RISCVCPU),
> >           VMSTATE_UINT64(env.mvien, RISCVCPU),
> >           VMSTATE_UINT64(env.mvip, RISCVCPU),
CONFIDENTIALITY NOTICE:

This e-mail (and its attachments) may contain confidential and legally 
privileged information or information protected from disclosure. If you are not 
the intended recipient, you are hereby notified that any disclosure, copying, 
distribution, or use of the information contained herein is strictly 
prohibited. In this case, please immediately notify the sender by return 
e-mail, delete the message (and any accompanying documents) and destroy all 
printed hard copies. Thank you for your cooperation.

Copyright ANDES TECHNOLOGY CORPORATION - All Rights Reserved.