mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Yang Shi <yang@os.amperecomputing.com>
To: Ryan Roberts <ryan.roberts@arm.com>,
	will@kernel.org, catalin.marinas@arm.com,
	Miko.Lenczewski@arm.com, scott@os.amperecomputing.com,
	cl@gentwo.org
Cc: linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Dev Jain <dev.jain@arm.com>
Subject: Re: [v3 PATCH 0/6] arm64: support FEAT_BBM level 2 and large block mapping when rodata=full
Date: Fri, 30 May 2025 14:21:11 -0700	[thread overview]
Message-ID: <12f987c5-0e7f-4930-945f-bf7a2e73f5c2@os.amperecomputing.com> (raw)
In-Reply-To: <1cc04c6b-ba0f-4e7f-ab85-46c364c66300@arm.com>



On 5/30/25 12:17 AM, Ryan Roberts wrote:
> On 29/05/2025 20:52, Yang Shi wrote:
>>>>> I just had another conversation about this internally, and there is another
>>>>> concern; we obviously don't want to modify the pgtables while other CPUs that
>>>>> don't support BBML2 could be accessing them. Even in stop_machine() this may be
>>>>> possible if the CPU stacks and task structure (for example) are allocated
>>>>> out of
>>>>> the linear map.
>>>>>
>>>>> So we need to be careful to follow the pattern used by kpti; all secondary CPUs
>>>>> need to switch to the idmap (which is installed in TTBR0) then install the
>>>>> reserved map in TTBR1, then wait for CPU 0 to repaint the linear map, then have
>>>>> the secondary CPUs switch TTBR1 back to swapper then switch back out of idmap.
>>>> So the below code should be ok?
>>>>
>>>> cpu_install_idmap()
>>>> Busy loop to wait for cpu 0 done
>>>> cpu_uninstall_idmap()
>>> Once you have installed the idmap, you'll need to call a function by its PA so
>>> you are actually executing out of the idmap. And you will need to be in assembly
>>> so you don't need the stack, and you'll need to switch TTBR1 to the reserved
>>> pgtable, so that the CPU has no access to the swapper pgtable (which CPU 0 is
>>> able to modify).
>>>
>>> You may well be able to reuse __idmap_kpti_secondary in proc.S, or lightly
>>> refactor it to work for both the existing idmap_kpti_install_ng_mappings case,
>>> and your case.
>> I'm wondering whether we really need idmap for repainting. I think repainting is
>> different from kpti. We just split linear map which is *not* used by kernel
>> itself, the mappings for kernel itself is intact, we don't touch it at all. So
>> as long as CPU 0 will not repaint the linear map until all other CPUs busy
>> looping in stop_machine fn, then we are fine.
> But *how* are the other CPUs busy looping? Are they polling a variable? Where
> does that variable live? The docs say that a high priority thread is run for
> each CPU. So there at least needs to be a task struct and a stack. There are
> some Kconfigs where the stack comes from the linear map, so if the variable that
> is polls is on its stack (or even on CPU 0's stack then that's a problem. If the
> scheduler runs and accesses the task struct which may be allocated from the
> linear map (e.g. via kmalloc), that's a problem.
>
> The point is that you have to understand all the details of stop_machine() to be
> confident that it is never accessing the linear map. And even if you can prove
> that today, there is nothing stopping from the implementation changing in future.
>
> But then you have non-architectural memory accesses too (i.e. speculative
> accesses). It's possible that the CPU does a speculative load, which causes the
> TLB to do a translation and cache a TLB entry to the linear map. Then CPU 0
> changes the pgtable and you have broken the BBM requirements from the secondary
> CPU's perspective.
>
> So personally I think the only truely safe way to solve this is to switch the
> secondary CPUs to the idmap, then install the reserved map in TTBR1. That way,
> the secondary CPUs can't see the swapper pgtable at all and CPU 0 is free to do
> what it likes.

OK, I agree it is safer to run the busy loop (wait for repainting done 
on boot CPU) in idmap address space.

IIUC I should just need map the flag polled by the secondary CPU in 
idmap so that both CPU 0 and secondary CPUs can access it. And have the 
wait function in .idmap.text section. I may not reuse kpti code because 
it is much simpler than kpti.

Thanks,
Yang

>
>> We can have two flags to control it. The first one should be a cpu mask, all
>> secondary CPUs set its own mask bit to tell CPU 0 it is in stop machine fn
>> (ready for repainting). The other flag is used by CPU 0 to tell all secondary
>> CPUs repainting is done, please resume. We need have the two flags in kernel
>> data section instead of stack.
>>
>> The code of fn is in kernel text section, the flags are in kernel data section.
>> I don't see how come fn (just doing simple busy loop) on secondary CPUs need to
>> access linear map while repainting the linear map. After repainting the TLB will
>> be flushed before letting secondary CPUs resume, so any access to linear map
>> address after that point should be safe too.
>>
>> Does it sound reasonable to you? Did I miss something?
> I think the potential for speculative access is the problem. Personally, I would
> follow the pattern laid out by kpti. Then you can more easily defend it by
> pointing to an established pattern.
>
> Thanks,
> Ryan
>
>> Thanks,
>> Yang
>>
>>> Thanks,
>>> Ryan
>>>
>>>>> Given CPU 0 supports BBML2, I think it can just update the linear map live,
>>>>> without needing to do the idmap dance?
>>>> Yes, I think so too.
>>>>
>>>> Thanks,
>>>> Yang
>>>>
>>>>> Thanks,
>>>>> Ryan
>>>>>
>>>>>
>>>>>> Thanks,
>>>>>> Ryan
>>>>>>


  reply	other threads:[~2025-05-30 21:21 UTC|newest]

Thread overview: 49+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-03-04 22:19 Yang Shi
2025-03-04 22:19 ` [v3 PATCH 1/6] arm64: Add BBM Level 2 cpu feature Yang Shi
2025-03-04 22:19 ` [v3 PATCH 2/6] arm64: cpufeature: add AmpereOne to BBML2 allow list Yang Shi
2025-03-14 10:58   ` Ryan Roberts
2025-03-17 17:50     ` Yang Shi
2025-03-04 22:19 ` [v3 PATCH 3/6] arm64: mm: make __create_pgd_mapping() and helpers non-void Yang Shi
2025-03-14 11:51   ` Ryan Roberts
2025-03-17 17:53     ` Yang Shi
2025-05-07  8:18       ` Ryan Roberts
2025-05-07 22:19         ` Yang Shi
2025-03-04 22:19 ` [v3 PATCH 4/6] arm64: mm: support large block mapping when rodata=full Yang Shi
2025-03-08  1:53   ` kernel test robot
2025-03-14 13:29   ` Ryan Roberts
2025-03-17 17:57     ` Yang Shi
2025-03-04 22:19 ` [v3 PATCH 5/6] arm64: mm: support split CONT mappings Yang Shi
2025-03-14 13:33   ` Ryan Roberts
2025-03-04 22:19 ` [v3 PATCH 6/6] arm64: mm: split linear mapping if BBML2 is not supported on secondary CPUs Yang Shi
2025-03-13 17:28 ` [v3 PATCH 0/6] arm64: support FEAT_BBM level 2 and large block mapping when rodata=full Yang Shi
2025-03-13 17:36   ` Ryan Roberts
2025-03-13 17:40     ` Yang Shi
2025-04-10 22:00       ` Yang Shi
2025-04-14 13:03         ` Ryan Roberts
2025-04-14 21:24           ` Yang Shi
2025-05-02 11:51             ` Ryan Roberts
2025-05-05 21:39               ` Yang Shi
2025-05-07  7:58                 ` Ryan Roberts
2025-05-07 21:16                   ` Yang Shi
2025-05-28  0:00                     ` Yang Shi
2025-05-28  3:47                       ` Dev Jain
2025-05-28 13:13                       ` Ryan Roberts
2025-05-28 15:18                         ` Yang Shi
2025-05-28 17:12                           ` Yang Shi
2025-05-29  8:48                             ` Ryan Roberts
2025-05-29 15:33                               ` Ryan Roberts
2025-05-29 17:35                                 ` Yang Shi
2025-05-29 18:30                                   ` Ryan Roberts
2025-05-29 19:52                                     ` Yang Shi
2025-05-30  7:17                                       ` Ryan Roberts
2025-05-30 21:21                                         ` Yang Shi [this message]
2025-05-29  7:36                           ` Ryan Roberts
2025-05-29 16:37                             ` Yang Shi
2025-05-29 17:01                               ` Ryan Roberts
2025-05-29 17:50                                 ` Yang Shi
2025-05-29 18:34                                   ` Ryan Roberts
2025-05-29 20:52                                     ` Yang Shi
2025-05-30  7:59                                       ` Ryan Roberts
2025-05-30 17:18                                         ` Yang Shi
2025-06-02 10:47                                           ` Ryan Roberts
2025-06-02 20:55                                             ` Yang Shi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=12f987c5-0e7f-4930-945f-bf7a2e73f5c2@os.amperecomputing.com \
    --to=yang@os.amperecomputing.com \
    --cc=Miko.Lenczewski@arm.com \
    --cc=catalin.marinas@arm.com \
    --cc=cl@gentwo.org \
    --cc=dev.jain@arm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=ryan.roberts@arm.com \
    --cc=scott@os.amperecomputing.com \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®