Re: [PATCH v2 13/18] hw/misc: riscv_worldguard: Add API to enable WG extension of CPU

[Jim Shu <jim.shu@sifive.com>]

Hi Daniel and Chao,

I will replace qemu_get_cpu() with cpu_by_arch_id().
However, cpu_by_arch_id() can still return NULL if the hartid is
invalid, so I will handle that case with a clean exit.

I will fix these in the v3 patch.


Thanks,
Jim

On Wed, Aug 13, 2025 at 11:10 AM Chao Liu <chao.liu@yeah.net> wrote:
>
> > On 4/17/25 7:52 AM, Jim Shu wrote:
> > > riscv_worldguard_apply_cpu() could enable WG CPU extension and set WG
> > > callback to CPUs. It is used by machine code after realizing global WG
> > > device.
> > >
> > > Signed-off-by: Jim Shu <jim.shu@sifive.com>
> > > ---
> > >   hw/misc/riscv_worldguard.c         | 87 ++++++++++++++++++++++++++++++
> > >   include/hw/misc/riscv_worldguard.h |  1 +
> > >   2 files changed, 88 insertions(+)
> > >
> > > diff --git a/hw/misc/riscv_worldguard.c b/hw/misc/riscv_worldguard.c
> > > index b02bd28d02..1a910f4cf3 100644
> > > --- a/hw/misc/riscv_worldguard.c
> > > +++ b/hw/misc/riscv_worldguard.c
> > > @@ -92,6 +92,93 @@ uint32_t mem_attrs_to_wid(MemTxAttrs attrs)
> > >       }
> > >   }
> > >
> > > +static void riscv_cpu_wg_reset(CPURISCVState *env)
> > > +{
> > > +    CPUState *cs = env_cpu(env);
> > > +    RISCVCPU *cpu = RISCV_CPU(cs);
> > > +    uint32_t mlwid, slwid, mwiddeleg;
> > > +    uint32_t trustedwid;
> > > +
> > > +    if (!riscv_cpu_cfg(env)->ext_smwg) {
> > > +        return;
> > > +    }
> > > +
> > > +    if (worldguard_config == NULL) {
> > > +        /*
> > > +         * Note: This reset is dummy now and WG CSRs will be reset again
> > > +         * after worldguard_config is realized.
> > > +         */
> > > +        return;
> > > +    }
> > > +
> > > +    trustedwid = worldguard_config->trustedwid;
> > > +    if (trustedwid == NO_TRUSTEDWID) {
> > > +        trustedwid = worldguard_config->nworlds - 1;
> > > +    }
> > > +
> > > +    /* Reset mlwid, slwid, mwiddeleg CSRs */
> > > +    if (worldguard_config->hw_bypass) {
> > > +        /* HW bypass mode */
> > > +        mlwid = trustedwid;
> > > +    } else {
> > > +        mlwid = 0;
> > > +    }
> > > +    slwid = 0;
> > > +    mwiddeleg = 0;
> > > +
> > > +    env->mlwid = mlwid;
> > > +    if (riscv_cpu_cfg(env)->ext_sswg) {
> > > +        env->slwid = slwid;
> > > +        env->mwiddeleg = mwiddeleg;
> > > +    }
> > > +
> > > +    /* Check mwid, mwidlist config */
> > > +    if (worldguard_config != NULL) {
> > > +        uint32_t valid_widlist = MAKE_64BIT_MASK(0, worldguard_config->nworlds);
> > > +
> > > +        /* CPU use default mwid / mwidlist config if not set */
> > > +        if (cpu->cfg.mwidlist == UINT32_MAX) {
> > > +            /* mwidlist contains all WIDs */
> > > +            cpu->cfg.mwidlist = valid_widlist;
> > > +        }
> > > +        if (cpu->cfg.mwid == UINT32_MAX) {
> > > +            cpu->cfg.mwid = trustedwid;
> > > +        }
> > > +
> > > +        /* Check if mwid/mwidlist HW config is valid in NWorld. */
> > > +        g_assert((cpu->cfg.mwidlist & ~valid_widlist) == 0);
> > > +        g_assert(cpu->cfg.mwid < worldguard_config->nworlds);
> > > +    }
> > > +}
> > > +
> > > +/*
> > > + * riscv_worldguard_apply_cpu - Enable WG extension of CPU
> > > + *
> > > + * Note: This API should be used after global WG device is created
> > > + * (riscv_worldguard_realize()).
> > > + */
> > > +void riscv_worldguard_apply_cpu(uint32_t hartid)
> > > +{
> > > +    /* WG global config should exist */
> > > +    g_assert(worldguard_config);
> >
> > We usually add g_asserts() after the variable declarations.
> >
> > > +
> > > +    CPUState *cpu = qemu_get_cpu(hartid);
>
> arm_get_cpu() uses CPUState::cpu_index to obtain the corresponding CPUState pointer.
>
> However, CPUState::cpu_index and the RISC-V HART index are not necessarily strictly
> one-to-one (for instance, when the hartid base is non-zero or when hartids are
> discontinuous).
>
> Typically, we use arm_get_cpu() at the accelerators, rather than in hw/code.
>
> A better approach is to use cpu_by_arch_id() instead of qemu_get_cpu(),
> in RISC-V cpu_by_arch_id() uses the hartid.
>
> e.g.
>
>      CPUState *cpu = cpu_by_arch_id(hartid);
>
>
> Thanks,
>
> Chao
>
> > > +    RISCVCPU *rcpu = RISCV_CPU(cpu);
> > > +    CPURISCVState *env = cpu ? cpu_env(cpu) : NULL;
> > > +
> > > +    rcpu->cfg.ext_smwg = true;
> > > +    if (riscv_has_ext(env, RVS) && riscv_has_ext(env, RVU)) {
> > > +        rcpu->cfg.ext_sswg = true;
> > > +    }
> >
> > riscv_has_ext() will segfault if env == NULL, and you're creating a code
> > path where this might happen:
> >
> > > +    CPURISCVState *env = cpu ? cpu_env(cpu) : NULL;
> >
> > In fact, cpu == NULL will explode on you earlier via this macro:
> >
> > > +    RISCVCPU *rcpu = RISCV_CPU(cpu);
> >
> > You can either handle cpu == NULL with a clean exit before using 'cpu' to assign
> > stuff or g_assert(cpu != NULL) for a more rude exit. But with this code as is
> > you're gambling with segfaults.
> >
> >
> > Thanks,
> >
> > Daniel
> >
> >
> > > +
> > > +    /* Set machine specific WorldGuard callback */
> > > +    env->wg_reset = riscv_cpu_wg_reset;
> > > +    env->wid_to_mem_attrs = wid_to_mem_attrs;
> > > +
> > > +    /* Reset WG CSRs in CPU */
> > > +    env->wg_reset(env);
> > > +}
> > > +
> > >   bool could_access_wgblocks(MemTxAttrs attrs, const char *wgblock)
> > >   {
> > >       uint32_t wid = mem_attrs_to_wid(attrs);
> > > diff --git a/include/hw/misc/riscv_worldguard.h b/include/hw/misc/riscv_worldguard.h
> > > index 8a533a0517..211a72e438 100644
> > > --- a/include/hw/misc/riscv_worldguard.h
> > > +++ b/include/hw/misc/riscv_worldguard.h
> > > @@ -48,6 +48,7 @@ extern struct RISCVWorldGuardState *worldguard_config;
> > >
> > >   DeviceState *riscv_worldguard_create(uint32_t nworlds, uint32_t trustedwid,
> > >                                        bool hw_bypass, bool tz_compat);
> > > +void riscv_worldguard_apply_cpu(uint32_t hartid);
> > >
> > >   uint32_t mem_attrs_to_wid(MemTxAttrs attrs);
> > >   bool could_access_wgblocks(MemTxAttrs attrs, const char *wgblock);
> >
>