On Mon, Jul 29, 2024 at 5:52 PM LIU Zhiwei <zhiwei_...@linux.alibaba.com> wrote:
>
>
> On 2024/7/29 14:07, Alvin Che-Chia Chang(張哲嘉) wrote:
> > Hi Zhiwei,
> >
> >> -----Original Message-----
> >> From: LIU Zhiwei <zhiwei_...@linux.alibaba.com>
> >> Sent: Monday, July 29, 2024 1:47 PM
> >> To: Alvin Che-Chia Chang(張哲嘉) <alvi...@andestech.com>;
> >> qemu-ri...@nongnu.org; qemu-devel@nongnu.org
> >> Cc: alistair.fran...@wdc.com; bin.m...@windriver.com;
> >> liwei1...@gmail.com; dbarb...@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 still can be used as an exclusive check. For example, two interrupt
> controllers try to bind the same MEIP field.
Exactly, that's what riscv_cpu_claim_interrupts() is doing. It's
checking to ensure that multiple interrupt controllers don't bind to
the same MIP field.
So the function is still used.
I think we could debate if we need it or not. I do think it's probably
worth keeping as it doesn't really cost us anything and it might help
catch setup issues.
Alistair
> > It is not used to check the read-only fields such as MTIP/MSIP/MEIP in mip.
> Once it was.
> > Moreover, env->miclaim is no longer used in latest code.
> Yes, we don't need it any more. When you remove it, upgrade the
> version number in machine.c is necessary.
> >
> >> 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() ?
>
> Just make them readable by adding a write mask for them.
>
> Thanks,
> Zhiwei
>
> >
> >
> > Best regards,
> > Alvin
> >
> >> Thanks,
> >> Zhiwei
> >>
> >>> Signed-off-by: Alvin Chang <alvi...@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.
>
