On Tue, Aug 9, 2022 at 6:33 PM Weiwei Li <liwei...@iscas.ac.cn> wrote:
>
> 在 2022/8/10 上午3:34, Atish Kumar Patra 写道:
>
>
>
>
> 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.
>
> 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;
>>> > }
>>>
>>>
