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?
