Re: [PATCH v16 71/99] target/arm: cpu-sve: new module

[Richard Henderson]

On 6/4/21 8:52 AM, Alex Bennée wrote:
> +#ifndef CPU_SVE_H
> +#define CPU_SVE_H
> +
> +/* note: SVE is an AARCH64-only option, only include this for TARGET_AARCH64 */

This seems unnecessary.

> @@ -201,13 +202,9 @@ typedef struct {
>   
>   #ifdef TARGET_AARCH64
>   # define ARM_MAX_VQ    16
> -void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp);
> -void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp);
>   #else
>   # define ARM_MAX_VQ    1
> -static inline void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp) { }
> -static inline void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp) { }
> -#endif
> +#endif /* TARGET_AARCH64 */

Hmm.  I'm not sure about removing the stubs, but see below.

> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 2fef8ca471..6db37b42d1 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -23,6 +23,7 @@
>   #include "target/arm/idau.h"
>   #include "qapi/error.h"
>   #include "cpu.h"
> +#include "cpu-sve.h"

Not following the advice given above, which I agree with.

> +#ifdef TARGET_AARCH64
>       if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
>           arm_cpu_sve_finalize(cpu, &local_err);
>           if (local_err != NULL) {
> @@ -838,6 +840,7 @@ void arm_cpu_finalize_features(ARMCPU *cpu, Error **errp)
>               }
>           }
>       }
> +#endif /* TARGET_AARCH64 */

New ifdefs, which isn't great.  But I also see that it's more or less a 1-1 
swap.  One ifdef here, or one ifdef in the header to create the stub.  So I 
guess it's a draw.

So, modulo the comment at the top,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~