Re: [PATCH RFC v2 03/29] mm: asi: Introduce ASI core API

[Brendan Jackman <jackmanb@google.com>]

On Tue Mar 18, 2025 at 12:50 AM UTC, Junaid Shahid wrote:
> On 3/17/25 4:40 AM, Brendan Jackman wrote:
> > 
> > I don't understand having both asi_[un]lock() _and_
> > asi_{start,enter}_critical_region(). The only reason we need the
> > critical section concept is for the purposes of the NMI glue code you
> > mentioned in part 1, and that setup must happen before the switch into
> > the restricted address space.
> > 
> > Also, I don't think we want part 5 inside the asi_lock()->asi_unlock()
> > region. That seems like the region betwen part 5 and 6, we are in the
> > unrestricted address space, but the NMI entry code is still set up to
> > return to the restricted address space on exception return. I think
> > that would actually be harmless, but it doesn't achieve anything.
> > 
> > The more I talk about it, the more convinced I am that the proper API
> > should only have two elements, one that says "I'm about to run
> > untrusted code" and one that says "I've finished running untrusted
> > code". But...
> > 
> >> 1. you can do empty calls to keep the interface balanced and easy to use
> >>
> >> 2. once you can remove asi_exit(), you should be able to replace all in-tree
> >>     users in one atomic change so that they're all switched to the new,
> >>     simplified interface
> > 
> > Then what about if we did this:
> > 
> > /*
> >   * Begin a region where ASI restricted address spaces _may_ be used.
> >   *
> >   * Preemption must be off throughout this region.
> >   */
> > static inline void asi_start(void)
> > {
> > 	/*
> > 	 * Cannot currently context switch in the restricted adddress
> > 	 * space.
> > 	 */
> > 	lockdep_assert_preemption_disabled();
>
> I assume that this limitation is just for the initial version in this RFC, 
> right? 

Well I think we also wanna get ASI in-tree with this limitation,
otherwise the initial series will be too big and complex. But yea,
it's a temporary thing for sure. Maybe resolving that would be the
highest-priority issue once ASI is merged.

> But even in that case, I think this should be in asi_start_critical() 
> below, not asi_start(), since IIRC the KVM run loop does contain preemptible 
> code as well. And we would need an explicit asi_exit() in the context switch 
> code like we had in an earlier RFC.

Oh. Yeah. In my proposal below I had totally forgotten we had
asi_exit() in the context_switch() path (it is there in this patch).

So we only need the asi_exit() in the KVM code in order to avoid
actually hitting e.g. exit_to_user_mode() in the restricted address
space.

But... we can just put an asi_exit() there explicitly instead of
dumping all this weirdness into the "core API" and the KVM codebase.

So... I think all we really need is asi_start_critical() and
asi_end_critical()? And make everything else happen as part of the
normal functioning of the entry and context-switching logic. Am I
forgetting something else?