Re: [PATCH 1/3] xen/arm: Sync sysregs and cpuinfo with Linux 5.18-rc3

[Julien Grall <julien@xen.org>]

Hi Bertrand,

On 03/05/2022 10:38, Bertrand Marquis wrote:
> Sync arm64 sysreg bit shift definitions with status of Linux kernel as
> of 5.18-rc3 version (linux commit b2d229d4ddb1).
> Sync ID registers sanitization with the status of Linux 5.18-rc3 and add
> sanitization of ISAR2 registers.
Please outline which specific commits you are actually backported. This 
would help to know what changed, why and also keep track of the autorships.

When possible, the changes should be separated to match each Linux 
commit we backport.

> Complete AA64ISAR2 and AA64MMFR1 with more fields.
> While there add a comment for MMFR bitfields as for other registers in
> the cpuinfo structure definition.

AFAICT, this patch is doing 3 different things that are somewhat related:
   - Sync cpufeature.c
   - Update the headers with unused defines
   - Complete the structure cpufeature.h

All those changes seem to be independent, so I think they should be done 
separately. This would help to keep the authorship right (your code vs 
Linux code).

> 
> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
> ---
>   xen/arch/arm/arm64/cpufeature.c          | 18 +++++-
>   xen/arch/arm/include/asm/arm64/sysregs.h | 76 ++++++++++++++++++++----
>   xen/arch/arm/include/asm/cpufeature.h    | 14 ++++-
>   3 files changed, 91 insertions(+), 17 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/cpufeature.c b/xen/arch/arm/arm64/cpufeature.c
> index 6e5d30dc7b..d9039d37b2 100644
> --- a/xen/arch/arm/arm64/cpufeature.c
> +++ b/xen/arch/arm/arm64/cpufeature.c
> @@ -143,6 +143,16 @@ static const struct arm64_ftr_bits ftr_id_aa64isar1[] = {
>   	ARM64_FTR_END,
>   };
>   
> +static const struct arm64_ftr_bits ftr_id_aa64isar2[] = {
> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_HIGHER_SAFE, ID_AA64ISAR2_CLEARBHB_SHIFT, 4, 0),
> +	ARM64_FTR_BITS(FTR_VISIBLE_IF_IS_ENABLED(CONFIG_ARM64_PTR_AUTH),
> +		       FTR_STRICT, FTR_EXACT, ID_AA64ISAR2_APA3_SHIFT, 4, 0),
> +	ARM64_FTR_BITS(FTR_VISIBLE_IF_IS_ENABLED(CONFIG_ARM64_PTR_AUTH),
So we are using CONFIG_ARM64_PTR_AUTH. But this is not defined in 
Kconfig. I realize there are more in cpufeature.c (somehow I didn't spot 
during preview), but I don't think this is right to define CONFIG_* 
without an associated entry in Kconfig.

In one hand, I think it would be odd to add an entry in Kconfig because 
Xen wouldn't properly work if selected. On the other hand, it is useful 
if when we will implement pointer authentification.

So maybe we should just add the Kconfig entry with a comment explaning 
why they are not selected. Any thoughts?

Cheers,

-- 
Julien Grall