Re: [Qemu-devel] [PATCH v1 11/28] target/riscv: Add background register swapping function

From: Palmer Dabbelt <palmer@sifive.com>
To: Alistair Francis <Alistair.Francis@wdc.com>
Cc: qemu-riscv@nongnu.org, Anup Patel <Anup.Patel@wdc.com>,
	qemu-devel@nongnu.org, Atish Patra <Atish.Patra@wdc.com>,
	Alistair Francis <Alistair.Francis@wdc.com>,
	alistair23@gmail.com
Subject: Re: [Qemu-devel] [PATCH v1 11/28] target/riscv: Add background register swapping function
Date: Wed, 11 Sep 2019 07:17:00 -0700 (PDT)	[thread overview]
Message-ID: <mhng-fe7f7960-29da-4b78-a6e1-37b03bc878b8@palmer-si-x1e> (raw)
In-Reply-To: <ec79c985398944a8443eac5673d40bc0969f8380.1566603412.git.alistair.francis@wdc.com>

On Fri, 23 Aug 2019 16:38:18 PDT (-0700), Alistair Francis wrote:
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  target/riscv/cpu.h        | 24 ++++++++---
>  target/riscv/cpu_bits.h   |  7 ++++
>  target/riscv/cpu_helper.c | 88 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 113 insertions(+), 6 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 680592cb60..05957f32a8 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -125,15 +125,18 @@ struct CPURISCVState {
>      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.
> +     * CAUTION! Unlike the rest of this struct, mip and mip_novirt 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 if
> +     * mip is non-zero.
>       * mip is 32-bits to allow atomic_read on 32-bit hosts.
>       */
>      uint32_t mip;
> +    uint32_t mip_novirt;
> +
>      uint32_t miclaim;
>
>      target_ulong *mie;
> @@ -179,6 +182,14 @@ struct CPURISCVState {
>      target_ulong vstval;
>      target_ulong vsatp;
>
> +    /* HS Backup CSRs */
> +    target_ulong stvec_hs;
> +    target_ulong sscratch_hs;
> +    target_ulong sepc_hs;
> +    target_ulong scause_hs;
> +    target_ulong stval_hs;
> +    target_ulong satp_hs;
> +
>      target_ulong scounteren;
>      target_ulong mcounteren;
>
> @@ -306,6 +317,7 @@ void riscv_cpu_list(void);
>  #define cpu_mmu_index riscv_cpu_mmu_index
>
>  #ifndef CONFIG_USER_ONLY
> +void riscv_cpu_swap_hypervisor_regs(CPURISCVState *env);
>  int riscv_cpu_claim_interrupts(RISCVCPU *cpu, uint32_t interrupts);
>  uint32_t riscv_cpu_update_mip(RISCVCPU *cpu, uint32_t mask, uint32_t value);
>  #define BOOL_TO_MASK(x) (-!!(x)) /* helper for riscv_cpu_update_mip value */
> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> index 78067901a2..5cee72b726 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -556,4 +556,11 @@
>  #define SIP_STIP                           MIP_STIP
>  #define SIP_SEIP                           MIP_SEIP
>
> +/* MIE masks */
> +#define MIE_SEIE                           (1 << IRQ_S_EXT)
> +#define MIE_UEIE                           (1 << IRQ_U_EXT)
> +#define MIE_STIE                           (1 << IRQ_S_TIMER)
> +#define MIE_UTIE                           (1 << IRQ_U_TIMER)
> +#define MIE_SSIE                           (1 << IRQ_S_SOFT)
> +#define MIE_USIE                           (1 << IRQ_U_SOFT)
>  #endif
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index c597523d74..41d4368128 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -81,6 +81,94 @@ bool riscv_cpu_fp_enabled(CPURISCVState *env)
>      return false;
>  }
>
> +void riscv_cpu_swap_hypervisor_regs(CPURISCVState *env)
> +{
> +    RISCVCPU *cpu = RISCV_CPU(env_cpu(env));
> +    uint32_t tmp;
> +    target_ulong mstatus_mask = MSTATUS_MXR | MSTATUS_SUM | MSTATUS_FS |
> +                                MSTATUS_SPP | MSTATUS_SPIE | MSTATUS_SIE;
> +    target_ulong sie_mask = MIE_SEIE | MIE_STIE | MIE_SSIE |
> +                            MIE_UEIE | MIE_UTIE | MIE_USIE;
> +    target_ulong mip_mask = MIP_SSIP | MIP_STIP | MIP_SEIP;
> +    bool current_virt = riscv_cpu_virt_enabled(env);
> +
> +    g_assert(riscv_has_ext(env, RVH));
> +
> +#if defined(TARGET_RISCV64)
> +    mstatus_mask |= MSTATUS64_UXL;
> +#endif
> +
> +    if (current_virt) {

This worries me more than the pointer stuff: specifically, my worry is keeping 
V in sync with the register set in use.

> +        /* Current V=1 and we are about to change to V=0 */
> +        env->mstatus = &env->mstatus_novirt;
> +        *env->mstatus &= mstatus_mask;
> +        *env->mstatus |= env->vsstatus & ~mstatus_mask;
> +        /* Ensure that vsstatus only holds the correct bits */
> +        env->vsstatus &= mstatus_mask;
> +
> +        env->mie = &env->mie_novirt;
> +        *env->mie &= sie_mask;
> +        *env->mie |= env->vsie & ~sie_mask;
> +        /* Ensure that vsie only holds the correct bits */
> +        env->vsie &= sie_mask;
> +
> +        env->vstvec = env->stvec;
> +        env->stvec = env->stvec_hs;
> +
> +        env->vsscratch = env->sscratch;
> +        env->sscratch = env->sscratch_hs;
> +
> +        env->vsepc = env->sepc;
> +        env->sepc = env->sepc_hs;
> +
> +        env->vscause = env->scause;
> +        env->scause = env->scause_hs;
> +
> +        env->vstval = env->sbadaddr;
> +        env->sbadaddr = env->stval_hs;
> +
> +        env->vsatp = env->satp;
> +        env->satp = env->satp_hs;
> +
> +        tmp = (uint32_t)atomic_read(&env->mip_novirt);
> +        tmp = riscv_cpu_update_mip(cpu, mip_mask, tmp);
> +        tmp &= mip_mask;
> +        atomic_set(&env->vsip, tmp);

We talked about this in person, and Alistair is going to do the fake interrupt 
thing to avoid the atomicity requirements entirely.

> +    } else {
> +        /* Current V=0 and we are about to change to V=1 */
> +        env->mstatus = &env->vsstatus;
> +        *env->mstatus &= mstatus_mask;
> +        *env->mstatus |= env->mstatus_novirt & ~mstatus_mask;
> +
> +        env->mie = &env->vsie;
> +        *env->mie &= sie_mask;
> +        *env->mie |= env->mie_novirt & ~sie_mask;
> +
> +        env->stvec_hs = env->stvec;
> +        env->stvec = env->vstvec;
> +
> +        env->sscratch_hs = env->sscratch;
> +        env->sscratch = env->vsscratch;
> +
> +        env->sepc_hs = env->sepc;
> +        env->sepc = env->vsepc;
> +
> +        env->scause_hs = env->scause;
> +        env->scause = env->vscause;
> +
> +        env->stval_hs = env->sbadaddr;
> +        env->sbadaddr = env->vstval;
> +
> +        env->satp_hs = env->satp;
> +        env->satp = env->vsatp;
> +
> +        tmp = (uint32_t)atomic_read(&env->vsip);
> +        tmp = riscv_cpu_update_mip(cpu, mip_mask, tmp);
> +        tmp &= mip_mask;
> +        atomic_set(&env->mip_novirt, tmp);
> +    }
> +}
> +
>  bool riscv_cpu_virt_enabled(CPURISCVState *env)
>  {
>      bool tmp;