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

[Julien Grall <julien@xen.org>]

On 04/05/2022 08:39, Bertrand Marquis wrote:
> Hi Julien,
Hi,

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

Well... that's possible at the cost of everyone going through Linux to 
understand why the changes were made. This is not very scalable.

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

If we import the full file, then we will generally don't log all the 
commits. However, for smaller changes, we will always mention the commit 
backported. There are several examples on the ML:

  - 0435784cc75d ("xen/arm: smmuv1: Intelligent SMR allocation")
  - 9c432b876bf5 ("x86/mwait-idle: add SPR support")

We also recently introduced a tag "Origin:" to keep track of which 
commit was backported. If you want to understand the rationale, you can 
read this long thread:

https://lore.kernel.org/xen-devel/0ed245fa-58a7-a5f6-b82e-48f9ed0b6970@suse.com/

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

I think we need to differentiate the two request. The previous request 
is about logging which commits you backported. I would be open to have 
all of them in one patch so long we account the authors/tags properly.

For this request, this is mostly about avoid to mix multiple things 
together. Your commit message describes 3 distinct parts and therefore 
they should be split.

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

I am OK with that.

Cheers,

-- 
Julien Grall