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