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 <[email protected]> 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 <[email protected]> > > > --- > > > 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); > > >
