Re: [PATCH v8 3/3] target/riscv: Add vstimecmp support

[Atish Kumar Patra <atishp@rivosinc.com>]

[-- Attachment #1: Type: text/plain, Size: 15990 bytes --]

On Tue, Aug 9, 2022 at 6:33 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:

>
> 在 2022/8/10 上午3:34, Atish Kumar Patra 写道:
>
>
>
>
> 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.
>
> 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;
>>> >   }
>>>
>>>

[-- Attachment #2: Type: text/html, Size: 33157 bytes --]