Re: [RFC PATCH 3/4] xen/arm: Sanitize cpuinfo ID registers fields

From: Bertrand Marquis <Bertrand.Marquis@arm.com>
To: Julien Grall <julien@xen.org>
Cc: Xen-devel <xen-devel@lists.xenproject.org>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Ian Jackson <iwj@xenproject.org>, Jan Beulich <jbeulich@suse.com>,
	Wei Liu <wl@xen.org>
Subject: Re: [RFC PATCH 3/4] xen/arm: Sanitize cpuinfo ID registers fields
Date: Mon, 12 Jul 2021 16:29:51 +0000	[thread overview]
Message-ID: <D4992248-BED1-4766-8872-1BFEB04068FE@arm.com> (raw)
In-Reply-To: <08d7e35e-6785-9ce9-2e8b-1bbf65e4b47f@xen.org>

Hi Julien,

> On 12 Jul 2021, at 12:13, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 12/07/2021 12:03, Bertrand Marquis wrote:
>> Hi Julien,
> 
> Hi Bertrand,
> 
>>> On 12 Jul 2021, at 11:16, Julien Grall <julien@xen.org> wrote:
>>> 
>>> Hi Bertrand,
>>> 
>>> On 29/06/2021 18:08, Bertrand Marquis wrote:
>>>> Define a sanitize_cpu function to be called on secondary cores to
>>>> sanitize the cpuinfo structure from the boot CPU.
>>>> The safest value is taken when possible and the system is marked tainted
>>>> if we encounter values which are incompatible with each other.
>>>> Call the sanitize_cpu function on all secondary cores and remove the
>>>> code disabling secondary cores if they are not the same as the boot core
>>>> as we are now able to handle this use case.
>>>> This is only supported on arm64 so cpu_sanitize is an empty static
>>>> inline on arm32.
>>>> This patch also removes the code imported from Linux that will not be
>>>> used by Xen.
>>>> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
>>>> ---
>>>>  xen/arch/arm/arm64/cpusanitize.c | 236 ++++++++++++++++++++++++-------
>>>>  xen/arch/arm/smpboot.c           |   5 +-
>>>>  xen/include/asm-arm/cpufeature.h |   9 ++
>>>>  xen/include/xen/lib.h            |   1 +
>>>>  4 files changed, 199 insertions(+), 52 deletions(-)
>>>> diff --git a/xen/arch/arm/arm64/cpusanitize.c b/xen/arch/arm/arm64/cpusanitize.c
>>>> index 4cc8378c14..744006ca7c 100644
>>>> --- a/xen/arch/arm/arm64/cpusanitize.c
>>>> +++ b/xen/arch/arm/arm64/cpusanitize.c
>>>> @@ -97,10 +97,6 @@ struct arm64_ftr_bits {
>>>>  		.width = 0,				\
>>>>  	}
>>>>  -static void cpu_enable_cnp(struct arm64_cpu_capabilities const *cap);
>>>> -
>>>> -static bool __system_matches_cap(unsigned int n);
>>>> -
>>>>  /*
>>>>   * NOTE: Any changes to the visibility of features should be kept in
>>>>   * sync with the documentation of the CPU feature register ABI.
>>>> @@ -277,31 +273,6 @@ static const struct arm64_ftr_bits ftr_id_aa64mmfr2[] = {
>>>>  	ARM64_FTR_END,
>>>>  };
>>>>  -static const struct arm64_ftr_bits ftr_ctr[] = {
>>>> -	ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_EXACT, 31, 1, 1), /* RES1 */
>>>> -	ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, CTR_DIC_SHIFT, 1, 1),
>>>> -	ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, CTR_IDC_SHIFT, 1, 1),
>>>> -	ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_HIGHER_OR_ZERO_SAFE, CTR_CWG_SHIFT, 4, 0),
>>>> -	ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_HIGHER_OR_ZERO_SAFE, CTR_ERG_SHIFT, 4, 0),
>>>> -	ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, CTR_DMINLINE_SHIFT, 4, 1),
>>>> -	/*
>>>> -	 * Linux can handle differing I-cache policies. Userspace JITs will
>>>> -	 * make use of *minLine.
>>>> -	 * If we have differing I-cache policies, report it as the weakest - VIPT.
>>>> -	 */
>>>> -	ARM64_FTR_BITS(FTR_VISIBLE, FTR_NONSTRICT, FTR_EXACT, CTR_L1IP_SHIFT, 2, ICACHE_POLICY_VIPT),	/* L1Ip */
>>>> -	ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, CTR_IMINLINE_SHIFT, 4, 0),
>>>> -	ARM64_FTR_END,
>>>> -};
>>> 
>>> I don't think this is should be dropped. Xen will need to know the safest cacheline size that can be used for cache maintenance instructions.
>> I will surround those entries by #if 0 instead
> 
> But, why can't this just be sanitized as the other registers? If this is just a matter of missing structure in Xen, then we should add one.
> 
> The same goes for the other 2 below.

The point of the RFC was to validate the way to go and do the base.

Those require changes out of the cpuinfo and are on my todo list to.

Regards
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall