linux-kernel - Re: [PATCH v1 1/3] arm64: Add BBM Level 2 cpu feature

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <ac84727f-0f81-4e63-a822-3fbc2d30eada@os.amperecomputing.com>
Date: Thu, 20 Feb 2025 12:01:25 -0800
From: Yang Shi <yang@...amperecomputing.com>
To: Mikołaj Lenczewski <miko.lenczewski@....com>
Cc: ryan.roberts@....com, catalin.marinas@....com, will@...nel.org,
 joey.gouly@....com, broonie@...nel.org, mark.rutland@....com,
 james.morse@....com, yangyicong@...ilicon.com, robin.murphy@....com,
 anshuman.khandual@....com, maz@...nel.org, liaochang1@...wei.com,
 akpm@...ux-foundation.org, david@...hat.com, baohua@...nel.org,
 ioworker0@...il.com, oliver.upton@...ux.dev,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/3] arm64: Add BBM Level 2 cpu feature




On 2/20/25 2:34 AM, Mikołaj Lenczewski wrote:
> On Wed, Feb 19, 2025 at 05:25:00PM -0800, Yang Shi wrote:
>>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>>> index d561cf3b8ac7..8c337bd95ef7 100644
>>> --- a/arch/arm64/kernel/cpufeature.c
>>> +++ b/arch/arm64/kernel/cpufeature.c
>>> @@ -2176,6 +2176,31 @@ static bool hvhe_possible(const struct arm64_cpu_capabilities *entry,
>>>    	return arm64_test_sw_feature_override(ARM64_SW_FEATURE_OVERRIDE_HVHE);
>>>    }
>>>    
>>> +static bool has_bbml2_noconflict(const struct arm64_cpu_capabilities *entry,
>>> +				 int scope)
>>> +{
>>> +	if (!IS_ENABLED(CONFIG_ARM64_ENABLE_BBML2))
>>> +		return false;
>>> +
>>> +	/* We want to allow usage of bbml2 in as wide a range of kernel contexts
>>> +	 * as possible. This list is therefore an allow-list of known-good
>>> +	 * implementations that both support bbml2 and additionally, fulfil the
>>> +	 * extra constraint of never generating TLB conflict aborts when using
>>> +	 * the relaxed bbml2 semantics (such aborts make use of bbml2 in certain
>>> +	 * kernel contexts difficult to prove safe against recursive aborts).
>>> +	 */
>>> +	static const struct midr_range supports_bbml2_without_abort_list[] = {
>>> +		MIDR_REV_RANGE(MIDR_CORTEX_X4, 0, 3, 0xf),
>>> +		MIDR_REV_RANGE(MIDR_NEOVERSE_V3, 0, 2, 0xf),
>>> +		{}
>>> +	};
>>> +
>>> +	if (!is_midr_in_range_list(read_cpuid_id(), supports_bbml2_without_abort_list))
>>> +		return false;
>>> +
>>> +	return true;
>>> +}
>> Hi Miko,
>>
>> Thanks for cc'ing me this series. I and Ryan discussed about how to
>> advertise BBML2 properly in my thread
>> (https://lore.kernel.org/linux-arm-kernel/4c44cf6e-98de-47bb-b430-2b1331114904@os.amperecomputing.com/).
>> IIUC, this may not work as expected.
>>
>> The boot cpu initializes the boot_cpu_data, then the secondary cpus need
>> to update it, the "sanitized" register value will be generated. For
>> example, TLB range capability is determined by ISAR0_EL1. If all the
>> cpus have this feature, the "sanitized" register value will show true
>> otherwise it will show false.
>>
>> BBML2 can be determined by MMFR2_EL1. If we can rely on it then system
>> feature does work. But the problem is some implementations may have
>> MMFR2_EL1 set, but they may not be able to handle TLB conflict. We can't
>> rely on it solely so we check MIDR in .matches callback instead of
>> MMFR2_EL1. But system feature .matches callback is just called once on
>> boot CPU because it is supposed to read the sanitized register value. So
>> you actually just checked the MIDR on boot CPU in .matches callback if I
>> read the code correctly.
>>
>> I'm not quite familiar with cpufeature details, if I'm wrong please feel
>> free to correct me.
>>
>> Yang
> Hi Yang,
>
> Thank you for taking the time to review this patch series.
>
> Thank you also for the spot. I am very much not an expert on
> cpufeatures, but I think you are correct. IIUC, currently the .matches
> check would go through as long as the the boot CPU executing the
> .matches function has the correct MIDR value, and as long as
> CONFIG_ARM64_ENABLE_BBML2 is set.
>
> If, as you point out, another CPU has a MIDR that is not on this list
> and which was not checked (because .matches only executes on a single
> boot CPU), then .matches should still go through (and we could run into
> problems when said other CPU executes any BBML2 aware code).
>
> Please let me know if I have understood what you are saying correctly.

Yes, that is exactly what I meant.

>
>  From re-reading `include/asm/cpufeature.h`, making it a SCOPE_LOCAL_CPU
> feature seems to be close to what we want. We want to have each CPU test
> its cpuid against the allowed MIDR list, and the feature overall to
> only be considered present if *all* boot cpus returned true (not as
> SCOPE_LOCAL_CPU puts it "... detect if at least one matches.").

Yeah, LOCAL_CPU feature may work, but I'm not sure whether it can 
satisfy this usecase in the current shape.

>
> I will see if this can be hacked around with the current system, and if
> not, we might have to extend the current cpucaps scopes / machinery like
> you suggest in your patch series comments.

Thank you for looking into it.

Yang

>