[RESEND RFC PATCH v1 2/5] arm64: Add BBM Level 2 cpu feature

[Ryan Roberts]

On 12/12/2024 14:26, Marc Zyngier wrote:
> On Thu, 12 Dec 2024 10:55:45 +0000,
> Ryan Roberts <ryan.roberts at arm.com> wrote:
>>
>> On 12/12/2024 08:25, Marc Zyngier wrote:
>>>> +
>>>> +	local_flush_tlb_all();
>>>
>>> The elephant in the room: if TLBs are in such a sorry state, what
>>> guarantees we can make it this far?
>>
>> I'll leave Miko to respond to your other comments, but I wanted to address this
>> one, since it's pretty fundamental. We went around this loop internally and
>> concluded that what we are doing is architecturally sound.
>>
>> The expectation is that a conflict abort can only be generated as a result of
>> the change in patch 4 (and patch 5). That change makes it possible for the TLB
>> to end up with a multihit. But crucially that can only happen for user space
>> memory because that change only operates on user memory. And while the TLB may
>> detect the conflict at any time, the conflict abort is only permitted to be
>> reported when an architectural access is prevented by the conflict. So we never
>> do anything that would allow a conflict for a kernel memory access and a user
>> memory conflict abort can never be triggered as a result of accessing kernel memory.
>>
>> Copy/pasting comment from AlexC on the topic, which explains it better than I can:
>>
>> """
>> The intent is certainly that in cases where the hardware detects a TLB conflict
>> abort, it is only permitted to report it (by generating an exception) if it
>> applies to an access that is being attempted architecturally. ... that property
>> can be built from the following two properties:
>>
>> 1. The TLB conflict can only be reported as an Instruction Abort or a Data Abort
>>
>> 2. Those two exception types must be reported synchronously and precisely.
>> """
> 
> I totally agree with this. The issue is that nothing says that the
> abort is in any way related to userspace.
> 
>>>
>>> I honestly don't think you can reliably handle a TLB Conflict abort in
>>> the same translation regime as the original fault, given that we don't
>>> know the scope of that fault. You are probably making an educated
>>> guess that it is good enough on the CPUs you know of, but I don't see
>>> anything in the architecture that indicates the "blast radius" of a
>>> TLB conflict.
>>
>> OK, so I'm claiming that the blast radius is limited to the region of memory
>> that we are operating on in contpte_collapse() in patch 4. Perhaps we need to go
>> re-read the ARM and come back with the specific statements that led us to that
>> conclusion?

>From the ARM:
"""
RFCPSG: If level 1 or level 2 is supported and the Contiguous bit in a set of
Block descriptors or Page descriptors is changed, then a TLB conflict abort can
be generated because multiple translation table entries might exist within a TLB
that translates the same IA.
"""

Although I guess it's not totally explicit, I've interpretted that as saying
that conflicting TLB entries can only arise for the IA range for which the
contiguous bits have been modified in the translation tables.

Given we are only fiddling with the contiguous bits for user space mappings in
this way, that's why I'm asserting we will only get a conflict abort for user
space mappings... assuming the absence of kernel bugs, anyway...

> 
> But we don't know for sure what caused this conflict by the time we
> arrive in the handler. It could equally be because we have a glaring
> bug somewhere on the kernel side, even if you are *now* only concerned
> with userspace.

OK I see what you are saying; previously a conflict abort would have led to
calling do_bad(), which returns 1, which causes do_mem_abort() to either kill
the kernel or the process depending on the origin of the abort. (although if it
came from kernel due to bug, we're just hoping that the conflict doesn't affect
the path through the handler). With this change, we always assume we can fix it
with the TLBI.

How about this change to ensure we still die for issues originating from the kernel?

if (!user_mode(regs) || !system_supports_bbml2())
		return do_bad(far, esr, regs);

> 
> If anything, this should absolutely check for FAR_EL1 and assert that
> this is indeed caused by such change.

I'm not really sure how we would check this reliably? Without patch 5, the
problem is somewhat constrained; we could have as many changes in flight as
there are CPUs so we could keep a list of all the {mm_struct, VA-range} that are
being modified. But if patch 5 is confirmed to be architecturally sound, then
there is no "terminating tlbi" so there is no bound on the set of {mm_struct,
VA-range}'s that could legitimately cause a conflict abort.

Thanks,
Ryan

> 
> Thanks,
> 
> 	M.
>