Hi Julien,
> On 4 May 2022, at 09:20, Julien Grall <jul...@xen.org> wrote:
>
>
>
> On 04/05/2022 08:39, Bertrand Marquis wrote:
>> Hi Julien,
> Hi,
>
>>> On 3 May 2022, at 19:08, Julien Grall <jul...@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-48f9ed0b6...@suse.com/
Do I understand right that it is ok for you if I push one patch mentioning all
the commits done in Linux corresponding to the changes (instead of one patch
per commit) ?
>
>>>
>>>> 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.
So 3 patches if you confirm the previous point.
I am ok with that
>
>>>> Signed-off-by: Bertrand Marquis <bertrand.marq...@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.
Ok, there are currently 4 CONFIG_ not defined so I will add a patch for those
in my serie.
Cheers
Bertrand
>
> Cheers,
>
> --
> Julien Grall
