Message-ID: <c5a74dfe-68e2-48f1-9bbb-06db8e62ffea@arm.com>
Date: Tue, 13 May 2025 10:15:49 +0100
From: Suzuki K Poulose <suzuki.poulose@....com>
To: Catalin Marinas <catalin.marinas@....com>,
 Ryan Roberts <ryan.roberts@....com>
Cc: Will Deacon <will@...nel.org>,
 Mikołaj Lenczewski <miko.lenczewski@....com>,
 yang@...amperecomputing.com, corbet@....net, jean-philippe@...aro.org,
 robin.murphy@....com, joro@...tes.org, akpm@...ux-foundation.org,
 paulmck@...nel.org, mark.rutland@....com, joey.gouly@....com,
 maz@...nel.org, james.morse@....com, broonie@...nel.org,
 oliver.upton@...ux.dev, baohua@...nel.org, david@...hat.com,
 ioworker0@...il.com, jgg@...pe.ca, nicolinc@...dia.com, mshavit@...gle.com,
 jsnitsel@...hat.com, smostafa@...gle.com, kevin.tian@...el.com,
 linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, iommu@...ts.linux.dev
Subject: Re: [RESEND PATCH v6 1/3] arm64: Add BBM Level 2 cpu feature

On 12/05/2025 17:33, Catalin Marinas wrote:
> On Mon, May 12, 2025 at 02:35:01PM +0100, Ryan Roberts wrote:
>> On 12/05/2025 14:24, Suzuki K Poulose wrote:
>>> On 12/05/2025 14:07, Ryan Roberts wrote:
>>>> On 09/05/2025 17:04, Catalin Marinas wrote:
>>>>> On Fri, May 09, 2025 at 02:49:05PM +0100, Will Deacon wrote:
>>>>>> 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.
>>>>>>
>>>>>> Does that work? If not, then perhaps the cpufeature/cpuerrata code needs
>>>>>> some surgery for this.
>>>>>
>>>>> Ah, I should have read this thread in order. I think we can treat this
>>>>> as BBML2_NOABORT available as default based on ID regs and use the
>>>>> allow/deny-list as an erratum.
>>>>
>>>> Just to make sure I've understood all this, I think what you are both saying is
>>>> we can create a single capability called ARM64_HAS_NO_BBML2_NOABORT of type
>>>> ARM64_CPUCAP_LOCAL_CPU_ERRATUM. Each CPU will then check it has BBML2 and is in
>>>> the MIDR allow list; If any of those conditions are not met, the CPU is
>>>> considered to have ARM64_HAS_NO_BBML2_NOABORT.
>>>
>>> I guess we need two caps.
>>>
>>> 1. SYSTEM cap -> ARM64_HAS_BBML2. Based on the ID registers
>>> 2. An erratum -> ARM64_BBML2_ABORTS. Based on BBLM2==1 && !in_midr_list()
>>
>> I don't think we *need* two caps; I was suggesting to consider both of these
>> conditions for the single cap. You are suggesting to separate them. But I think
>> both approaches give the same result?
>>
>> I'm easy either way, but keen to understand why 2 caps are preferred?
> 
> I guess it's easier to reason about than a single, negated property but
> the result should be identical. With two properties we can easily
> implement the idreg override like nobbml2 since this works on the
> sanitised ID regs. But we could also implement this differently, no need
> to rely on the ID regs.
> 
> Stepping back a bit, we know that the MIDR allow-list implies
> BBML2_NOABORT (and at least BBML2 as in the ID regs). In theory, we need

Please be aware that BBML2_NOABORT midr list may not always imply BBLM2 
in ID registers (e.g., AmpereOne. But the plan is to fixup the per cpu
ID register - struct cpuinfo_arm64 - for such cores at early boot,
individually, before it is used for sanitisation of the system wide
copy).


> something like a SYSTEM_FEATURE which is the conjunction of all the
> early CPUs. However, such system-level cap is only checked after all the
> early CPUs booted _and_ only on the sanitised ID regs rather than MIDR.
> 
> We need a LOCAL_CPU feature behaviour to be called on each CPU but still
> have the conjunction of early CPUs, more like the system one. It should
> be permitted for late CPUs to have but not optional if already enabled.
> 
> So how about we introduce a WEAK_BOOT_CPU_FEATURE which gets enabled by
> the boot CPU if it has it _but_ cleared by any secondary early CPU if it
> doesn't (and never enabled by secondary CPUs). When the features are
> finalised, we know if all early CPUs had it. In combination with
> PERMITTED_FOR_LATE_CPU, we'd reject late CPUs that don't have it.

That could work, but it introduces this "clearing" a capability, which
we don't do at the moment.

We had an offline discussion about this some time ago, with Mark
Rutland. The best way to deal with this is to change the way we compute
capabilities. i.e.,


1. Each boot CPU run through all the capabilities and maintain a per-cpu
    copy of the state.
2. System wide capabilities can then be constructed from the all early
    boot CPU capability state (e.g., ANDing all the state from all CPUs
    for SCOPE_SYSTEM or ORing for LOCAL_CPU).

But this requires a drastic change to the infrastructure.

> 
> I think if we can get the above, it would be the cleaner option than
> trying to bend our minds around double negations like !NO_BBLM2_NOABORT.

Agree, every time I come back to the thread, I have to write down the
check and stare at it for a minute to agree with what it does. That said
it may be ideal solution for the short term. Or stick to what we do in
the patch currently, until we implement per-cpu capability proposal.

Cheers
Suzuki

> 

Powered by blists - more mailing lists