Re: [RESEND PATCH v6 1/3] arm64: Add BBM Level 2 cpu feature

[Ryan Roberts <ryan.roberts@arm.com>]

On 09/05/2025 15:28, Will Deacon wrote:
> On Fri, May 09, 2025 at 03:16:01PM +0100, Ryan Roberts wrote:
>> On 09/05/2025 14:49, Will Deacon wrote:
>>> On Tue, May 06, 2025 at 03:52:59PM +0100, Ryan Roberts wrote:
>>>> On 06/05/2025 15:25, Will Deacon wrote:
>>>>> On Mon, Apr 28, 2025 at 03:35:14PM +0000, Mikołaj Lenczewski wrote:
>>>>>> +static bool has_bbml2_noabort(const struct arm64_cpu_capabilities *caps, int scope)
>>>>>> +{
>>>>>> +	if (!IS_ENABLED(CONFIG_ARM64_BBML2_NOABORT))
>>>>>> +		return false;
>>>>>> +
>>>>>> +	if (scope & SCOPE_SYSTEM) {
>>>>>> +		int cpu;
>>>>>> +
>>>>>> +		/*
>>>>>> +		 * We are a boot CPU, and must verify that all enumerated boot
>>>>>> +		 * CPUs have MIDR values within our allowlist. Otherwise, we do
>>>>>> +		 * not allow the BBML2 feature to avoid potential faults when
>>>>>> +		 * the insufficient CPUs access memory regions using BBML2
>>>>>> +		 * semantics.
>>>>>> +		 */
>>>>>> +		for_each_online_cpu(cpu) {
>>>>>> +			if (!cpu_has_bbml2_noabort(cpu_read_midr(cpu)))
>>>>>> +				return false;
>>>>>> +		}
>>>>>
>>>>> This penalises large homogeneous systems and it feels unnecessary given
>>>>> that we have the ability to check this per-CPU. 
>>
>> In case you didn't spot it, cpu_read_midr() is not actually reading a remote
>> cpu's midr. It's reading the cached midr in a per-cpu data structure. So I don't
>> think this will be very expensive in reality. And it's only run once during boot...
>>
>> static inline unsigned int cpu_read_midr(int cpu)
>> {
>> 	WARN_ON_ONCE(!cpu_online(cpu));
>>
>> 	return per_cpu(cpu_data, cpu).reg_midr;
>> }
> 
> I know it's not reading a remote MIDR, that would be crazy :)
> 
> But iterating over per-cpu variables sucks and we should be able to avoid
> it because this code already knows how to detect features locally.
> 
>>
>>> Can you use
>>>>> ARM64_CPUCAP_BOOT_CPU_FEATURE instead of ARM64_CPUCAP_SYSTEM_FEATURE
>>>>> to solve this?
>>>>
>>>> We are trying to solve for the case where the boot CPU has BBML2 but a secondary
>>>> CPU doesn't. (e.g. hetrogeneous system where boot CPU is big and secondary is
>>>> little and does not advertise the feature. I can't remember if we proved there
>>>> are real systems with this config - I have vague recollection that we did but my
>>>> memory is poor...).
>>>>
>>>> My understanding is that for ARM64_CPUCAP_BOOT_CPU_FEATURE, "If the boot CPU
>>>> has enabled this feature already, then every late CPU must have it". So that
>>>> would exclude any secondary CPUs without BBML2 from coming online?
>>>
>>> Damn, yes, you're right. However, it still feels horribly hacky to iterate
>>> over the online CPUs in has_bbml2_noabort() -- the cpufeature framework
>>> has the ability to query features locally and we should be able to use
>>> that. We're going to want that should the architecture eventually decide
>>> on something like BBML3 for this.
>>
>> For BBML3, we're looking for a minimum value in the BBM field of the FFMR, and
>> in that case the framework can handle it nicely with
>> ARM64_CPUCAP_SYSTEM_FEATURE. But the framework doesn't have any support for the
>> case where we need to look at all the midrs. So Suzuki came up with this method.
>>
>> If/when we have BBML3 I was thinking we could retrospectively treat the CPUs in
>> the midr list as having an erratum that they don't advertise BBML3 when they
>> should (since the semantics are basically the same I expect/hope/have been
>> trying to ensure), so we can just implement workarounds to make it look like
>> they do have BBML3, then the framework does it's thing. Perhaps we can live with
>> this hack until we get to that point?
> 
> I think if you want to go down that route, then this needs to be detected
> locally on each CPU.

Yes that's my point; once we have BBML3 it will be detected locally for each CPU
because the framework can handle that for MMFR fields. But until we get there,
we are stuck with a midr list.

> 
>>> What we have with BBML2_NOABORT seems similar to an hwcap in that we only
>>> support the capability if all CPUs have it (rejecting late CPUs without it
>>> in that case) but we can live without it if not all of the early CPUs
>>> have it. Unlikely hwcaps, though, we shouldn't be advertising this to
>>> userspace and we can't derive the capability solely from the sanitised
>>> system registers.
>>
>> Agreed.
>>
>>>
>>> I wonder if we could treat it like an erratum in some way instead? That
>>> is, invert things so that CPUs which _don't_ have BBML2_NOABORT are
>>> considered to have a "BBM_CONFLICT_ABORT" erratum (which we obviously
>>> wouldn't shout about). Then we should be able to say:
>>>
>>>   - If any of the early CPUs don't have BBML2_NOABORT, then the erratum
>>>     would be enabled and we wouln't elide BBM.
>>>
>>>   - If a late CPU doesn't have BBML2_NOABORT then it can't come online
>>>     if the erratum isn't already enabled.
>>
>> That's exactly the policy that this cludge provides. But it's using the midr to
>> check if the CPU has BBML2_NOABORT. I'm not sure I follow your point about a
>> "BBM_CONFLICT_ABORT" erratum?
> 
> I was hoping that it would mean that each CPU can independently determine
> whether or not they have the erratum and then enable it as soon as they
> detect it. That way, there's no need to iterate over all the early cores.

OK, I don't understand the framework well enough to fully understand your
suggestion; I'll talk to Suzuki and have a dig through the code.

Thanks for the review!
Ryan

> 
>> I'm also at a massive disadvantage because I find the whole cpufeatures
>> framework impenetrable :)
>>
>> I'll ping Suzuki to see if he can chime in here.
> 
> Thanks,
> 
> Will