On Tue, Aug 9, 2022 at 12:01 AM Weiwei Li <liwei...@iscas.ac.cn> wrote:
>
> 在 2022/8/9 上午1:20, Atish Kumar Patra 写道:
>
>
>
> On Sun, Aug 7, 2022 at 6:50 PM Weiwei Li <liwei...@iscas.ac.cn> 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 <ati...@rivosinc.com>
>> > ---
>> > 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.
>
> 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;
>> > }
>>
>>
