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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

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