[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Julien Grall <julien@xxxxxxx>
  • From: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>
  • Date: Wed, 4 May 2022 09:49:54 +0000
  • Accept-language: en-GB, en-US
  • Arc-authentication-results: i=2; mx.microsoft.com 1; spf=pass (sender ip is 63.35.35.123) smtp.rcpttodomain=lists.xenproject.org smtp.mailfrom=arm.com; dmarc=pass (p=none sp=none pct=100) action=none header.from=arm.com; dkim=pass (signature was verified) header.d=armh.onmicrosoft.com; arc=pass (0 oda=1 ltdi=1 spf=[1,1,smtp.mailfrom=arm.com] dkim=[1,1,header.d=arm.com] dmarc=[1,1,header.from=arm.com])
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none
  • Arc-message-signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=v4yrrZoq72BY0aOgRp+XuKvQ60XUgDAQlKDmxu9cXok=; b=Bl/LRoLhzUZZv1pGG8MyrPv63Rv4ZWlbHkuOtwj51kGZSOv44GuO1yUxjUga77gafFdV+/CiBDQzeIetoLkPjyKiq5o9DV6pWwz+JIhJBQ+mjxV5NOhuDdCeNrJ6EbGA12TXcKi68ws4Gh3l+m3cycaUTTt59vVABbdZBHpIGoF8pPovR6pKrCu9P3j/RzLjWhJKzCtlmidScySSqFKbT/97WP99DMBAYqg4f0VBytSA+L7JdjFh/ZJyepeMjgqImEF/H8tqFoxoOb+8G4eeAhuEaRvk9lzS/u37+xfSItJYAqO+zlIjiZrXQlwpD54sOvrz/ntf5xPnjsJa0RWJ6g==
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=v4yrrZoq72BY0aOgRp+XuKvQ60XUgDAQlKDmxu9cXok=; b=fWocSSXb5G0f3pvuRa2nQDcfh47LAOup8JUrMPlCMoubCHy2X23IwLLJxlLHgRHjopUVzj0iuEp2FgYqNDLK6kJtukOuebmVc9iY9l047FSFAxZXnfKlnbfEgFH4JPgRvbziOqfAfWzEEyCm2AnCTqYHwIXwJDA4/Ms/C9PPVc+80/zDeWWYQxY0ZE5s/GYcgwygLnMMKbx3Bykfky3Pmqfdw3+PE0dX3hXMw4E/9F/XQ/nb9Z5P9v3ili9o5kNnqh6RM5qNgTF+s7VBsp+kA806Op5NPMaN/0NUZUxCN+pWwHbaev7nRMrxjaNEdIj8dwPIOHrEqRGfi1LKAEB3iQ==
  • Arc-seal: i=2; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=pass; b=CDRFsv4Df9OZYMtw+OVd5bCd2CeDxb7tEU0QRx9l6KSUsAGKNCMTkb2fi0kGaG1qhNWJigG91XHZuu/aVWeb25exk5bHc0YsAR5AwVjGFMZ4G6VfJA9xrCKLg1bBOj7TR78PxtfOKwfu0WiiGSmrpwJSNieGaZpo8y69oIAungc46Lgb29qcTG8akgOhvQz8dlTmr4r4pJ2TAcK0BUlgXzaVAZF5qN7IGgp6NGKHWT6+Ax841vVjwJKYM+/ycfl/VsIW9yp/sxu14Ycd/eh46BY8HLFs2qvRjYPJrXUW3yDF03YMpV/ObUX/pse6RqH3Oqyc7dVSheLZD5pu7x1RFw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=F5gifq9RU5WGkcaqMeYwDqW6FxM/mGWcZZGywNGmxCAv58pj8IctGxVIRNqu2mrCV5/v6YBCRGLrZMzygXykbQq030yDeHZFpFZ3U3h88F2UnFchl9Hf0qmwC6TX4p0Hjp+TNdnlo4jZDeGuicmNE1Lj8nhXVh3KvGH0hBaszXavIok6HzLccrsHIIBJwseyHtPAnJXhMNDB/K/ThpqFc6LV+a0SXmOKUXA4DG8eTSUmQvb5GACTxJiOTRG1yICnUNWmY1pm6TXsbwmo5ZxQOmgsiSPF+bUeBuJ8EO0FIae462sgB7ZXTtEjYrbhCaM+T87NLa/ojFYGMsK2R18mmg==
  • Authentication-results-original: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com;
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
  • Delivery-date: Wed, 04 May 2022 09:50:12 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Nodisclaimer: true
  • Original-authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com;
  • Thread-index: AQHYXtGhqgeBKorGIE+S3ZHm07cmGK0Nc+GAgADisYCAAAtXAIAAGQ0A
  • Thread-topic: [PATCH 1/3] xen/arm: Sync sysregs and cpuinfo with Linux 5.18-rc3

Hi Julien,

> On 4 May 2022, at 09:20, Julien Grall <julien@xxxxxxx> wrote:
> 
> 
> 
> On 04/05/2022 08:39, Bertrand Marquis wrote:
>> Hi Julien,
> Hi,
> 
>>> 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
> 
> 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@xxxxxxxx/

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




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.