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

[Bertrand Marquis <Bertrand.Marquis@arm.com>]

Hi Julien,

> On 3 May 2022, at 19:08, Julien Grall <julien@xen.org> wrote:
> 
> 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.

As those are exactly identical to the linux tree, one can easily use git blame on the linux source tree to find those information if it is needed.

I checked a bit and this is not something that was required before (for example when the cpufeature was introduced).

> 
>> 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).

This and the previous request to split using linux commit will actually end up in 10 patches or more.

In the current, the change can easily be checked doing a diff with the mentioned Linux version, so I am not really thrilled to make it more complex.

Please confirm that all this is really what you want.

> 
>> 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?

This is really right and a very good catch.

I think it would make sense to introduce those in Kconfig in order to keep the code equivalent to Linux.

So I would suggest here to add hidden entries like this:

ARM64_PTR_AUTH
	def_bool n
	depends on ARM64
        help
          Pointer authentication support.
          This feature is not supported by Xen.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall