On Tue, Aug 9, 2022 at 6:33 PM Weiwei Li wrote: > > 在 2022/8/10 上午3:34, Atish Kumar Patra 写道: > > > > > On Tue, Aug 9, 2022 at 12:01 AM Weiwei Li wrote: > >> >> 在 2022/8/9 上午1:20, Atish Kumar Patra 写道: >> >> >> >> On Sun, Aug 7, 2022 at 6:50 PM Weiwei Li wrote: >> >>> >>> 在 2022/8/4 上午9:42, Atish Patra 写道: >>> > vstimecmp CSR allows the guest OS or to program the next guest timer >>> > interrupt directly. Thus, hypervisor no longer need to inject the >>> > timer interrupt to the guest if vstimecmp is used. This was ratified >>> > as a part of the Sstc extension. >>> > >>> > Signed-off-by: Atish Patra >>> > --- >>> > target/riscv/cpu.h | 4 ++ >>> > target/riscv/cpu_bits.h | 4 ++ >>> > target/riscv/cpu_helper.c | 11 ++-- >>> > target/riscv/csr.c | 102 >>> ++++++++++++++++++++++++++++++++++++- >>> > target/riscv/machine.c | 1 + >>> > target/riscv/time_helper.c | 16 ++++++ >>> > 6 files changed, 133 insertions(+), 5 deletions(-) >>> > >>> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h >>> > index 4cda2905661e..1fd382b2717f 100644 >>> > --- a/target/riscv/cpu.h >>> > +++ b/target/riscv/cpu.h >>> > @@ -312,6 +312,8 @@ struct CPUArchState { >>> > /* Sstc CSRs */ >>> > uint64_t stimecmp; >>> > >>> > + uint64_t vstimecmp; >>> > + >>> > /* physical memory protection */ >>> > pmp_table_t pmp_state; >>> > target_ulong mseccfg; >>> > @@ -366,6 +368,8 @@ struct CPUArchState { >>> > >>> > /* Fields from here on are preserved across CPU reset. */ >>> > QEMUTimer *stimer; /* Internal timer for S-mode interrupt */ >>> > + QEMUTimer *vstimer; /* Internal timer for VS-mode interrupt */ >>> > + bool vstime_irq; >>> > >>> > hwaddr kernel_addr; >>> > hwaddr fdt_addr; >>> > diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h >>> > index ac17cf1515c0..095dab19f512 100644 >>> > --- a/target/riscv/cpu_bits.h >>> > +++ b/target/riscv/cpu_bits.h >>> > @@ -257,6 +257,10 @@ >>> > #define CSR_VSIP 0x244 >>> > #define CSR_VSATP 0x280 >>> > >>> > +/* Sstc virtual CSRs */ >>> > +#define CSR_VSTIMECMP 0x24D >>> > +#define CSR_VSTIMECMPH 0x25D >>> > + >>> > #define CSR_MTINST 0x34a >>> > #define CSR_MTVAL2 0x34b >>> > >>> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c >>> > index 650574accf0a..1e4faa84e839 100644 >>> > --- a/target/riscv/cpu_helper.c >>> > +++ b/target/riscv/cpu_helper.c >>> > @@ -345,8 +345,9 @@ uint64_t riscv_cpu_all_pending(CPURISCVState *env) >>> > { >>> > uint32_t gein = get_field(env->hstatus, HSTATUS_VGEIN); >>> > uint64_t vsgein = (env->hgeip & (1ULL << gein)) ? MIP_VSEIP : 0; >>> > + uint64_t vstip = (env->vstime_irq) ? MIP_VSTIP : 0; >>> > >>> > - return (env->mip | vsgein) & env->mie; >>> > + return (env->mip | vsgein | vstip) & env->mie; >>> > } >>> > >>> > int riscv_cpu_mirq_pending(CPURISCVState *env) >>> > @@ -605,7 +606,7 @@ uint64_t riscv_cpu_update_mip(RISCVCPU *cpu, >>> uint64_t mask, uint64_t value) >>> > { >>> > CPURISCVState *env = &cpu->env; >>> > CPUState *cs = CPU(cpu); >>> > - uint64_t gein, vsgein = 0, old = env->mip; >>> > + uint64_t gein, vsgein = 0, vstip = 0, old = env->mip; >>> > bool locked = false; >>> > >>> > if (riscv_cpu_virt_enabled(env)) { >>> > @@ -613,6 +614,10 @@ uint64_t riscv_cpu_update_mip(RISCVCPU *cpu, >>> uint64_t mask, uint64_t value) >>> > vsgein = (env->hgeip & (1ULL << gein)) ? MIP_VSEIP : 0; >>> > } >>> > >>> > + /* No need to update mip for VSTIP */ >>> > + mask = ((mask == MIP_VSTIP) && env->vstime_irq) ? 0 : mask; >>> > + vstip = env->vstime_irq ? MIP_VSTIP : 0; >>> > + >>> > if (!qemu_mutex_iothread_locked()) { >>> > locked = true; >>> > qemu_mutex_lock_iothread(); >>> > @@ -620,7 +625,7 @@ uint64_t riscv_cpu_update_mip(RISCVCPU *cpu, >>> uint64_t mask, uint64_t value) >>> > >>> > env->mip = (env->mip & ~mask) | (value & mask); >>> > >>> > - if (env->mip | vsgein) { >>> > + if (env->mip | vsgein | vstip) { >>> > cpu_interrupt(cs, CPU_INTERRUPT_HARD); >>> > } else { >>> > cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD); >>> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c >>> > index e18b000700e4..9da4d6515e7b 100644 >>> > --- a/target/riscv/csr.c >>> > +++ b/target/riscv/csr.c >>> > @@ -829,17 +829,100 @@ static RISCVException sstc(CPURISCVState *env, >>> int csrno) >>> > return smode(env, csrno); >>> > } >>> > >>> > +static RISCVException sstc_hmode(CPURISCVState *env, int csrno) >>> > +{ >>> > + CPUState *cs = env_cpu(env); >>> > + RISCVCPU *cpu = RISCV_CPU(cs); >>> > + >>> > + if (!cpu->cfg.ext_sstc || !env->rdtime_fn) { >>> > + return RISCV_EXCP_ILLEGAL_INST; >>> > + } >>> > + >>> > + if (env->priv == PRV_M) { >>> > + return RISCV_EXCP_NONE; >>> > + } >>> > + >>> > + if (!(get_field(env->mcounteren, COUNTEREN_TM) & >>> > + get_field(env->menvcfg, MENVCFG_STCE))) { >>> > + return RISCV_EXCP_ILLEGAL_INST; >>> > + } >>> > + >>> > + if (riscv_cpu_virt_enabled(env)) { >>> > + if (!(get_field(env->hcounteren, COUNTEREN_TM) & >>> > + get_field(env->henvcfg, HENVCFG_STCE))) { >>> > + return RISCV_EXCP_VIRT_INSTRUCTION_FAULT; >>> > + } >>> > + } >>> > + >>> >>> I think this check on hcounteren and henvcfg should be added to sstc >>> predicate, not here. >>> >>> Even though hcounteren and henvcfg finally controls the access of >>> vstimecmp, however >>> >>> >> We don't need to check hcounteren while accessing scountern. Thus it will >> be an unnecessary >> check there. Predicate function check should do the required sanity check >> required only for that specific CSR. >> That's why, I think it is the correct place. >> >> Sorry. It seems have no relationship with "check hcounteren while >> accessing scountern". >> >> > > >> As the sstc spec (Section 2.2 and Chapter 3) states: >> >> *"In addition, when the TM bit in the hcounteren register is clear, >> attempts to access the vstimecmp register (**via* >> *stimecmp**) while executing in VS-mode will cause a virtual instruction >> exception if the same bit in mcounteren is* >> *1. When this bit is set, access to the vstimecmp register (if >> implemented) is permitted in VS-mode."* >> >> *"When STCE in menvcfg is one but STCE in henvcfg is zero, an attempt to **access >> stimecmp **(really vstimecmp)* >> *when V = 1 raises a virtual instruction exception, and VSTIP in hip >> reverts to its defined behavior as if this* >> *extension is not implemented."* >> >> Both of them have stated the control is for stimecmp even though the >> final access is for vstimecmp just like >> >> your following modification for read/write_stimecmp. >> > > We can further simplify to remove sstc_hmode completely. After moving this > bit to sstc predicate function, it is enough for vstimecmp as well. > > Sorry. I cannot get your idea. Do you mean they share the same predicate? > They seem different: > > Vstimecmp is under is control of menvcfg and mcounteren. And stimecmp is > additionally controlled by senvcfg and scuonteren. > stimecmp is under control of menvcfg & mcounteren while vstimecmp is controlled by henvcfg & hcountern. senvcfg doesn't have a STCE bit. so vstimecmp predicate function should do the following things: - hmode check - sstc extension availability - menvcfg/mcounteren As vstimecmp can be accessed by HS mode/M mode only while VS mode access stimecmp (which is translated to vstimecmp if V=1) Thus stimecmp predicate function will do the following things: - smode check - sstc extension availability - menvcfg/mcounteren - henvcfg/hcountern (if V=1) That's why I was suggesting that we can simplify two predicate functions where only mode check will be different (based on CSR value). > On the other hand, Vstimecmp is a Hypervisor CSR(checked by hmode), > stimecmp is Supervisor CSR(checked by smode). > > Regards, > > Weiwei Li > > > >> >> From the other hand, direct access for VS CSRs (including vstimecmp) >> from VS/VU mode is never allowed, >> >> which is checked in riscv_csrrw_check. >> > Regards, >> >> Weiwei Li >> >> >> >>> it controls it via stimecmp. >>> >>> > + return hmode(env, csrno); >>> > +} >>> > + >>> > +static RISCVException read_vstimecmp(CPURISCVState *env, int csrno, >>> > + target_ulong *val) >>> > +{ >>> > + *val = env->vstimecmp; >>> > + >>> > + return RISCV_EXCP_NONE; >>> > +} >>> > + >>> > +static RISCVException read_vstimecmph(CPURISCVState *env, int csrno, >>> > + target_ulong *val) >>> > +{ >>> > + *val = env->vstimecmp >> 32; >>> > + >>> > + return RISCV_EXCP_NONE; >>> > +} >>> > + >>> > +static RISCVException write_vstimecmp(CPURISCVState *env, int csrno, >>> > + target_ulong val) >>> > +{ >>> > + RISCVCPU *cpu = env_archcpu(env); >>> > + >>> > + if (riscv_cpu_mxl(env) == MXL_RV32) { >>> > + env->vstimecmp = deposit64(env->vstimecmp, 0, 32, >>> (uint64_t)val); >>> > + } else { >>> > + env->vstimecmp = val; >>> > + } >>> > + >>> > + riscv_timer_write_timecmp(cpu, env->vstimer, env->vstimecmp, >>> > + env->htimedelta, MIP_VSTIP); >>> > + >>> > + return RISCV_EXCP_NONE; >>> > +} >>> > + >>> > +static RISCVException write_vstimecmph(CPURISCVState *env, int csrno, >>> > + target_ulong val) >>> > +{ >>> > + RISCVCPU *cpu = env_archcpu(env); >>> > + >>> > + env->vstimecmp = deposit64(env->vstimecmp, 32, 32, (uint64_t)val); >>> > + riscv_timer_write_timecmp(cpu, env->vstimer, env->vstimecmp, >>> > + env->htimedelta, MIP_VSTIP); >>> > + >>> > + return RISCV_EXCP_NONE; >>> > +} >>> > + >>> > static RISCVException read_stimecmp(CPURISCVState *env, int csrno, >>> > target_ulong *val) >>> > { >>> > - *val = env->stimecmp; >>> > + if (riscv_cpu_virt_enabled(env)) { >>> > + *val = env->vstimecmp; >>> > + } else { >>> > + *val = env->stimecmp; >>> > + } >>> > + >>> > return RISCV_EXCP_NONE; >>> > } >>> > >>> > static RISCVException read_stimecmph(CPURISCVState *env, int csrno, >>> > target_ulong *val) >>> > { >>> > - *val = env->stimecmp >> 32; >>> > + if (riscv_cpu_virt_enabled(env)) { >>> > + *val = env->vstimecmp >> 32; >>> > + } else { >>> > + *val = env->stimecmp >> 32; >>> > + } >>> > + >>> > return RISCV_EXCP_NONE; >>> > } >>> > >>> > @@ -848,6 +931,10 @@ static RISCVException >>> write_stimecmp(CPURISCVState *env, int csrno, >>> > { >>> > RISCVCPU *cpu = env_archcpu(env); >>> > >>> > + if (riscv_cpu_virt_enabled(env)) { >>> > + return write_vstimecmp(env, csrno, val); >>> > + } >>> > + >>> > if (riscv_cpu_mxl(env) == MXL_RV32) { >>> > env->stimecmp = deposit64(env->stimecmp, 0, 32, >>> (uint64_t)val); >>> > } else { >>> > @@ -864,6 +951,10 @@ static RISCVException >>> write_stimecmph(CPURISCVState *env, int csrno, >>> > { >>> > RISCVCPU *cpu = env_archcpu(env); >>> > >>> > + if (riscv_cpu_virt_enabled(env)) { >>> > + return write_vstimecmph(env, csrno, val); >>> > + } >>> > + >>> > env->stimecmp = deposit64(env->stimecmp, 32, 32, (uint64_t)val); >>> > riscv_timer_write_timecmp(cpu, env->stimer, env->stimecmp, 0, >>> MIP_STIP); >>> > >>> > @@ -1801,6 +1892,7 @@ static RISCVException rmw_mip64(CPURISCVState >>> *env, int csrno, >>> > if (csrno != CSR_HVIP) { >>> > gin = get_field(env->hstatus, HSTATUS_VGEIN); >>> > old_mip |= (env->hgeip & ((target_ulong)1 << gin)) ? >>> MIP_VSEIP : 0; >>> > + old_mip |= env->vstime_irq ? MIP_VSTIP : 0; >>> > } >>> > >>> > if (ret_val) { >>> > @@ -3661,6 +3753,12 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = { >>> > .min_priv_ver = >>> PRIV_VERSION_1_12_0 }, >>> > [CSR_STIMECMPH] = { "stimecmph", sstc, read_stimecmph, >>> write_stimecmph, >>> > .min_priv_ver = >>> PRIV_VERSION_1_12_0 }, >>> > + [CSR_VSTIMECMP] = { "vstimecmp", sstc_hmode, read_vstimecmp, >>> > + write_vstimecmp, >>> >>> Please align with last line. The same to other similar lines. >>> >>> >> Sure. I will fix that. >> >> >>> Regards, >>> >>> Weiwei Li >>> >>> > + .min_priv_ver = >>> PRIV_VERSION_1_12_0 }, >>> > + [CSR_VSTIMECMPH] = { "vstimecmph", sstc_hmode, read_vstimecmph, >>> > + write_vstimecmph, >>> > + .min_priv_ver = >>> PRIV_VERSION_1_12_0 }, >>> > >>> > /* Supervisor Protection and Translation */ >>> > [CSR_SATP] = { "satp", smode, read_satp, write_satp >>> }, >>> > diff --git a/target/riscv/machine.c b/target/riscv/machine.c >>> > index 622fface484e..4ba55705d147 100644 >>> > --- a/target/riscv/machine.c >>> > +++ b/target/riscv/machine.c >>> > @@ -92,6 +92,7 @@ static const VMStateDescription vmstate_hyper = { >>> > VMSTATE_UINTTL(env.hgeie, RISCVCPU), >>> > VMSTATE_UINTTL(env.hgeip, RISCVCPU), >>> > VMSTATE_UINT64(env.htimedelta, RISCVCPU), >>> > + VMSTATE_UINT64(env.vstimecmp, RISCVCPU), >>> > >>> > VMSTATE_UINTTL(env.hvictl, RISCVCPU), >>> > VMSTATE_UINT8_ARRAY(env.hviprio, RISCVCPU, 64), >>> > diff --git a/target/riscv/time_helper.c b/target/riscv/time_helper.c >>> > index f3fb5eac7b7b..8cce667dfd47 100644 >>> > --- a/target/riscv/time_helper.c >>> > +++ b/target/riscv/time_helper.c >>> > @@ -22,6 +22,14 @@ >>> > #include "time_helper.h" >>> > #include "hw/intc/riscv_aclint.h" >>> > >>> > +static void riscv_vstimer_cb(void *opaque) >>> > +{ >>> > + RISCVCPU *cpu = opaque; >>> > + CPURISCVState *env = &cpu->env; >>> > + env->vstime_irq = 1; >>> > + riscv_cpu_update_mip(cpu, MIP_VSTIP, BOOL_TO_MASK(1)); >>> > +} >>> > + >>> > static void riscv_stimer_cb(void *opaque) >>> > { >>> > RISCVCPU *cpu = opaque; >>> > @@ -47,10 +55,16 @@ void riscv_timer_write_timecmp(RISCVCPU *cpu, >>> QEMUTimer *timer, >>> > * If we're setting an stimecmp value in the "past", >>> > * immediately raise the timer interrupt >>> > */ >>> > + if (timer_irq == MIP_VSTIP) { >>> > + env->vstime_irq = 1; >>> > + } >>> > riscv_cpu_update_mip(cpu, timer_irq, BOOL_TO_MASK(1)); >>> > return; >>> > } >>> > >>> > + if (timer_irq == MIP_VSTIP) { >>> > + env->vstime_irq = 0; >>> > + } >>> > /* Clear the [V]STIP bit in mip */ >>> > riscv_cpu_update_mip(cpu, timer_irq, BOOL_TO_MASK(0)); >>> > >>> > @@ -95,4 +109,6 @@ void riscv_timer_init(RISCVCPU *cpu) >>> > env->stimer = timer_new_ns(QEMU_CLOCK_VIRTUAL, &riscv_stimer_cb, >>> cpu); >>> > env->stimecmp = 0; >>> > >>> > + env->vstimer = timer_new_ns(QEMU_CLOCK_VIRTUAL, >>> &riscv_vstimer_cb, cpu); >>> > + env->vstimecmp = 0; >>> > } >>> >>>