Re: [PATCH for-9.0 1/7] target/riscv: implement svade

[Andrew Jones]

On Thu, Nov 23, 2023 at 04:15:26PM -0300, Daniel Henrique Barboza wrote:
> 'svade' is a RVA22S64 profile requirement, a profile we're going to add
> shortly. It is a named feature (i.e. not a formal extension, not defined
> in riscv,isa DT at this moment) defined in [1] as:
> 
> "Page-fault exceptions are raised when a page is accessed when A bit is
> clear, or written when D bit is clear.".
> 
> As far as the spec goes, 'svade' is one of the two distinct modes of
> handling PTE_A and PTE_D. The other way, i.e. update PTE_A/PTE_D when
> they're cleared, is defined by the 'svadu' extension. Checking
> cpu_helper.c, get_physical_address(), we can verify that QEMU is
> compliant with that: we will update PTE_A/PTE_D if 'svadu' is enabled,
> or throw a page-fault exception if 'svadu' isn't enabled.
> 
> So, as far as we're concerned, 'svade' translates to 'svadu must be
> disabled'.
> 
> We'll implement it like 'zic64b': an internal flag that profiles can
> enable. The flag will not be exposed to users.
> 
> [1] https://github.com/riscv/riscv-profiles/blob/main/profiles.adoc
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/cpu.c         | 1 +
>  target/riscv/cpu_cfg.h     | 1 +
>  target/riscv/tcg/tcg-cpu.c | 9 +++++++++
>  3 files changed, 11 insertions(+)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 3a230608cb..59b131c1fc 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1445,6 +1445,7 @@ const RISCVCPUMultiExtConfig 
> riscv_cpu_experimental_exts[] = {
>  };
>  
>  const RISCVCPUMultiExtConfig riscv_cpu_named_features[] = {
> +    MULTI_EXT_CFG_BOOL("svade", svade, true),
>      MULTI_EXT_CFG_BOOL("zic64b", zic64b, true),
>  
>      DEFINE_PROP_END_OF_LIST(),
> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> index 90f18eb601..46b06db68b 100644
> --- a/target/riscv/cpu_cfg.h
> +++ b/target/riscv/cpu_cfg.h
> @@ -116,6 +116,7 @@ struct RISCVCPUConfig {
>      bool ext_smepmp;
>      bool rvv_ta_all_1s;
>      bool rvv_ma_all_1s;
> +    bool svade;
>      bool zic64b;
>  
>      uint32_t mvendorid;
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 8fa8d61142..ddf37b25f3 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -188,6 +188,9 @@ static void riscv_cpu_enable_named_feat(RISCVCPU *cpu, 
> uint32_t feat_offset)
>          cpu->cfg.cbop_blocksize = 64;
>          cpu->cfg.cboz_blocksize = 64;
>          break;
> +    case CPU_CFG_OFFSET(svade):
> +        cpu->cfg.ext_svadu = false;
> +        break;
>      default:
>          g_assert_not_reached();
>      }
> @@ -383,8 +386,14 @@ static void riscv_cpu_validate_zic64b(RISCVCPU *cpu)
>                        cpu->cfg.cboz_blocksize == 64;
>  }
>  
> +static void riscv_cpu_validate_svade(RISCVCPU *cpu)

Another use of the word "validate" that doesn't seem right to me (and the
use in riscv_cpu_validate_zic64b too). We should probably double check all
our uses of that word in function names. This function, for example,
doesn't check ("validate") anything it just does an assignment ("set"), so
riscv_cpu_set_svade() would seem more appropriate. (And, we don't really
even need the function, IMO).

> +{
> +    cpu->cfg.svade = !cpu->cfg.ext_svadu;
> +}
> +
>  static void riscv_cpu_validate_named_features(RISCVCPU *cpu)
>  {
> +    riscv_cpu_validate_svade(cpu);
>      riscv_cpu_validate_zic64b(cpu);
>  }
>  
> -- 
> 2.41.0
>

Otherwise,

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