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

[Junaid Shahid <junaids@google.com>]

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? 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.

> 
> 	/*
> 	 * (Actually, this doesn't do anything besides assert, it's
> 	 * just to help the API make sense).
> 	 */
> }
> 
> /*
>   * End a region begun by asi_start(). After this, the CPU cannot be in
>   * the restricted address space until the next asi_start().
>   */
> static inline void asi_end(void)
> {
> 	/* Leave the restricted address space if we're in it. */
> 	...
> }
> 
> /*
>   * About to run untrusted code, begin a region that _must_ run in the
>   * restricted address space.
>   */
> void asi_start_critical(void);
> 
> /* End a region begun by asi_start_critical(). */
> void asi_end_critical(void);
> 
> ioctl(KVM_RUN) {
>      enter_from_user_mode()
>      asi_start()
>      while !need_userspace_handling()
>          asi_start_critical();
>          vmenter();
>          asi_end_critical();
>      }
>      asi_end()
>      exit_to_user_mode()
> }
> 
> Then the API is balanced, and we have a clear migration path towards
> the two-element API, i.e. we need to just remove asi_start() and
> asi_end(). It also better captures the point about the temporary
> simplification: basically the reason why the API is currently
> overcomplicated is: if totally arbitrary parts of the kernel can find
> themselves in the restricted address space, we have more work to do.
> (It's totally possible, but we don't wanna block initial submission on
> that work). The simplification is about demarcating what code is and
> isn't affected by ASI, so having this "region" kinda helps with that.
> Although, because NMIs can also be affected it's a bit of a fuzzy
> demarcation...

Not just NMIs, but other IRQs can also be in the restricted address space even 
in this initial version. But that is of course still significantly less in scope 
than the general case, so the demarcation of process-context code via 
asi_start()/asi_end() does indeed seem useful.

Thanks,
Junaid