On Tue, 29 Oct 2019 03:49:23 PDT (-0700), alistai...@gmail.com wrote:
> On Fri, Oct 18, 2019 at 7:44 PM Alistair Francis <alistai...@gmail.com> wrote:
>>
>> On Fri, Oct 18, 2019 at 9:51 AM Palmer Dabbelt <pal...@sifive.com> wrote:
>> >
>> > On Tue, 08 Oct 2019 15:04:18 PDT (-0700), Alistair Francis wrote:
>> > > Instead of relying on atomics to access the MIP register let's update
>> > > our helper function to instead just lock the IO mutex thread before
>> > > writing. This follows the same concept as used in PPC for handling
>> > > interrupts
>> > >
>> > > Signed-off-by: Alistair Francis <alistair.fran...@wdc.com>
>> > > ---
>> > > target/riscv/cpu.c | 5 ++--
>> > > target/riscv/cpu.h | 9 --------
>> > > target/riscv/cpu_helper.c | 48 +++++++++++++++------------------------
>> > > target/riscv/csr.c | 2 +-
>> > > 4 files changed, 21 insertions(+), 43 deletions(-)
>> > >
>> > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> > > index f13e298a36..e09dd7aa23 100644
>> > > --- a/target/riscv/cpu.c
>> > > +++ b/target/riscv/cpu.c
>> > > @@ -224,8 +224,7 @@ static void riscv_cpu_dump_state(CPUState *cs, FILE
*f, int flags)
>> > > #ifndef CONFIG_USER_ONLY
>> > > qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mhartid ",
env->mhartid);
>> > > qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mstatus ",
env->mstatus);
>> > > - qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mip ",
>> > > - (target_ulong)atomic_read(&env->mip));
>> > > + qemu_fprintf(f, " %s 0x%x\n", "mip ", env->mip);
>> > > qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mie ", env->mie);
>> > > qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mideleg ",
env->mideleg);
>> > > qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "medeleg ",
env->medeleg);
>> > > @@ -275,7 +274,7 @@ static bool riscv_cpu_has_work(CPUState *cs)
>> > > * Definition of the WFI instruction requires it to ignore the
privilege
>> > > * mode and delegation registers, but respect individual enables
>> > > */
>> > > - return (atomic_read(&env->mip) & env->mie) != 0;
>> > > + return (env->mip & env->mie) != 0;
>> > > #else
>> > > return true;
>> > > #endif
>> > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>> > > index 124ed33ee4..a71473b243 100644
>> > > --- a/target/riscv/cpu.h
>> > > +++ b/target/riscv/cpu.h
>> > > @@ -121,15 +121,6 @@ struct CPURISCVState {
>> > > target_ulong mhartid;
>> > > target_ulong mstatus;
>> > >
>> > > - /*
>> > > - * CAUTION! Unlike the rest of this struct, mip is accessed
asynchonously
>> > > - * by I/O threads. It should be read with atomic_read. It should be
updated
>> > > - * using riscv_cpu_update_mip with the iothread mutex held. The
iothread
>> > > - * mutex must be held because mip must be consistent with the CPU
inturrept
>> > > - * state. riscv_cpu_update_mip calls cpu_interrupt or
cpu_reset_interrupt
>> > > - * wuth the invariant that CPU_INTERRUPT_HARD is set iff mip is
non-zero.
>> > > - * mip is 32-bits to allow atomic_read on 32-bit hosts.
>> > > - */
>> > > uint32_t mip;
>> > > uint32_t miclaim;
>> > >
>> > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>> > > index 87dd6a6ece..4334978c2e 100644
>> > > --- a/target/riscv/cpu_helper.c
>> > > +++ b/target/riscv/cpu_helper.c
>> > > @@ -19,6 +19,7 @@
>> > >
>> > > #include "qemu/osdep.h"
>> > > #include "qemu/log.h"
>> > > +#include "qemu/main-loop.h"
>> > > #include "cpu.h"
>> > > #include "exec/exec-all.h"
>> > > #include "tcg-op.h"
>> > > @@ -38,7 +39,7 @@ static int riscv_cpu_local_irq_pending(CPURISCVState
*env)
>> > > {
>> > > target_ulong mstatus_mie = get_field(env->mstatus, MSTATUS_MIE);
>> > > target_ulong mstatus_sie = get_field(env->mstatus, MSTATUS_SIE);
>> > > - target_ulong pending = atomic_read(&env->mip) & env->mie;
>> > > + target_ulong pending = env->mip & env->mie;
>> > > target_ulong mie = env->priv < PRV_M || (env->priv == PRV_M &&
mstatus_mie);
>> > > target_ulong sie = env->priv < PRV_S || (env->priv == PRV_S &&
mstatus_sie);
>> > > target_ulong irqs = (pending & ~env->mideleg & -mie) |
>> > > @@ -92,42 +93,29 @@ int riscv_cpu_claim_interrupts(RISCVCPU *cpu,
uint32_t interrupts)
>> > > }
>> > > }
>> > >
>> > > -struct CpuAsyncInfo {
>> > > - uint32_t new_mip;
>> > > -};
>> > > -
>> > > -static void riscv_cpu_update_mip_irqs_async(CPUState *target_cpu_state,
>> > > - run_on_cpu_data data)
>> > > -{
>> > > - struct CpuAsyncInfo *info = (struct CpuAsyncInfo *) data.host_ptr;
>> > > -
>> > > - if (info->new_mip) {
>> > > - cpu_interrupt(target_cpu_state, CPU_INTERRUPT_HARD);
>> > > - } else {
>> > > - cpu_reset_interrupt(target_cpu_state, CPU_INTERRUPT_HARD);
>> > > - }
>> > > -
>> > > - g_free(info);
>> > > -}
>> > > -
>> > > uint32_t riscv_cpu_update_mip(RISCVCPU *cpu, uint32_t mask, uint32_t
value)
>> > > {
>> > > CPURISCVState *env = &cpu->env;
>> > > CPUState *cs = CPU(cpu);
>> > > - struct CpuAsyncInfo *info;
>> > > - uint32_t old, new, cmp = atomic_read(&env->mip);
>> > > + uint32_t old = env->mip;
>> > > + bool locked = false;
>> > > +
>> > > + if (!qemu_mutex_iothread_locked()) {
>> > > + locked = true;
>> > > + qemu_mutex_lock_iothread();
>> > > + }
>> >
>> > I must be lost here, because I have no idea what this is trying to do.
>>
>> We lock the QEMU IO Thread before we trigger an interrupt. This way we
>> can call it from the PLIC.
>
> Ping! This missed the latest PR.
Sorry, I missed your reply.
I really don't think this does that. For example: if two threads enter this
function at the same time, both will see that the lock is not taken and try to
take it. One will succeed and the other will fail, but both will have
