Re: [PATCH v5 02/10] target/riscv: add rva22u64 profile definition

[Andrew Jones]

On Wed, Oct 25, 2023 at 08:44:51PM -0300, Daniel Henrique Barboza wrote:
> The rva22U64 profile, described in:
> 
> https://github.com/riscv/riscv-profiles/blob/main/profiles.adoc#rva22-profiles
> 
> Contains a set of CPU extensions aimed for 64-bit userspace
> applications. Enabling this set to be enabled via a single user flag
> makes it convenient to enable a predictable set of features for the CPU,
> giving users more predicability when running/testing their workloads.
> 
> QEMU implements all possible extensions of this profile. The exception
> is Zicbop (Cache-Block Prefetch Operations) that is not available since
> QEMU RISC-V does not implement a cache model.

I think we should add zicbop now. Its instructions won't cause illegal
instruction traps according to commit 59cb29d6a514 ("target/riscv: add
Zicbop cbo.prefetch{i, r, m} placeholder") and the instructions are
just hints anyway, so there's no need for QEMU to implement anything.
So, let's add the CPU property and its corresponding blocksize property,
and when zicbop is enabled add it to the ISA string and when its blocksize
isn't 64 bytes, disable zic64b.

> For this same reason all
> the so called 'synthetic extensions' described in the profile that are
> cache related are ignored (Za64rs, Zic64b, Ziccif, Ziccrse, Ziccamoa,
> Zicclsm).

I won't ack/nack the exclusion of these, since I don't know enough about
what they mean nor how TCG is or is not able to comply with what they
mean. So I'll take your word for it that, at most, we'd need to add them
to an ISA string to fully "support" them. OIOW, the current profile's only
"fault" regarding these "extensions" is it isn't describing them in some
way and fixing that wouldn't require any changes beyond extending the ISA
string.

> 
> An abstraction called RISCVCPUProfile is created to store the profile.
> 'ext_offsets' contains mandatory extensions that QEMU supports. Same
> thing with the 'misa_ext' mask. Optional extensions must be enabled
> manually in the command line if desired.
> 
> The design here is to use the common target/riscv/cpu.c file to store
> the profile declaration and export it to the accelerator files. Each
> accelerator is then responsible to expose it (or not) to users and how
> to enable the extensions.
> 
> Next patches will implement the profile for TCG and KVM.
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Acked-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  target/riscv/cpu.c | 31 +++++++++++++++++++++++++++++++
>  target/riscv/cpu.h | 12 ++++++++++++
>  2 files changed, 43 insertions(+)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 5095f093ba..d6ba0dff34 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1437,6 +1437,37 @@ Property riscv_cpu_options[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> +/*
> + * RVA22U64 defines some  'named features' or 'synthetic extensions'
                           ^ extra space

> + * that are cache related: Za64rs, Zic64b, Ziccif, Ziccrse, Ziccamoa
> + * and Zicclsm. We do not implement caching in QEMU so we'll consider
> + * all these named features as always enabled.
> + *
> + * There's no riscv,isa update for them (and for zic64b) at this

(nor for zic64b, despite it having a cfg offset)

> + * moment.
> + */
> +static RISCVCPUProfile RVA22U64 = {
> +    .name = "rva22u64",
> +    .misa_ext = RVI | RVM | RVA | RVF | RVD | RVC,
> +    .ext_offsets = {
> +        CPU_CFG_OFFSET(ext_zicsr), CPU_CFG_OFFSET(ext_zihintpause),
> +        CPU_CFG_OFFSET(ext_zba), CPU_CFG_OFFSET(ext_zbb),
> +        CPU_CFG_OFFSET(ext_zbs), CPU_CFG_OFFSET(ext_zfhmin),
> +        CPU_CFG_OFFSET(ext_zkt), CPU_CFG_OFFSET(ext_zicntr),
> +        CPU_CFG_OFFSET(ext_zihpm), CPU_CFG_OFFSET(ext_zicbom),
> +        CPU_CFG_OFFSET(ext_zicboz),
> +
> +        /* mandatory named features for this profile */
> +        CPU_CFG_OFFSET(zic64b),
> +
> +        RISCV_PROFILE_EXT_LIST_END
> +    }
> +};
> +
> +RISCVCPUProfile *riscv_profiles[] = {
> +    &RVA22U64, NULL,

I'd put the NULL on its own line, otherwise when we add the next profile
we'll need to modify the rva22u64 line too.

> +};
> +
>  static Property riscv_cpu_properties[] = {
>      DEFINE_PROP_BOOL("debug", RISCVCPU, cfg.debug, true),
>  
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index ee9abe61d6..d3d82c5a7a 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -68,6 +68,18 @@ const char *riscv_get_misa_ext_description(uint32_t bit);
>  
>  #define CPU_CFG_OFFSET(_prop) offsetof(struct RISCVCPUConfig, _prop)
>  
> +typedef struct riscv_cpu_profile {
> +    const char *name;
> +    uint32_t misa_ext;
> +    bool enabled;
> +    bool user_set;
> +    const int32_t ext_offsets[];
> +} RISCVCPUProfile;
> +
> +#define RISCV_PROFILE_EXT_LIST_END -1
> +
> +extern RISCVCPUProfile *riscv_profiles[];
> +
>  /* Privileged specification version */
>  enum {
>      PRIV_VERSION_1_10_0 = 0,
> -- 
> 2.41.0
>

Other than the nits and my wishy-washy-ness on the synthetic extensions,

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Thanks,
drew