On Tue, Aug 9, 2022 at 12:01 AM Weiwei Li <liweiwei@iscas.ac.cn> wrote:

>
> 在 2022/8/9 上午1:20, Atish Kumar Patra 写道:
>
>
>
> On Sun, Aug 7, 2022 at 6:50 PM Weiwei Li <liweiwei@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 <atishp@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;
>> >   }
>>
>>