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

[Brendan Jackman <jackmanb@google.com>]

> > OK, sounds like I need to rewrite this explanation! It's only been

> > read before by people who already knew how this thing worked so this
> > might take a few attempts to make it clear.
> > 
> > Maybe the best way to make it clear is to explain this with reference
> > to KVM. At a super high level, That looks like:
> > 
> > ioctl(KVM_RUN) {
> >     enter_from_user_mode()
> >     while !need_userspace_handling() {
> >         asi_enter();  // part 1
> >         vmenter();  // part 2
> >         asi_relax(); // part 3
> >     }
> >     asi _exit(); // part 4b
> >     exit_to_user_mode()
> > }
> > 
> > So part 4a is just referring to continuation of the loop.
> > 
> > This explanation was written when that was the only user of this API
> > so it was probably clearer, now we have userspace it seems a bit odd.
> > 
> > With my pseudocode above, does it make more sense? If so I'll try to
> > think of a better way to explain it.
> 
> Well, it is still confusing. I would expect to see:
> 
> ioctl(KVM_RUN) {
>     enter_from_user_mode()
>     while !need_userspace_handling() {
>         asi_enter();  // part 1
>         vmenter();  // part 2
>         asi_exit(); // part 3
>     }
>     asi_switch(); // part 4b
>     exit_to_user_mode()
> }
> 
> Because then it is ballanced: you enter the restricted address space, do stuff
> and then you exit it without switching address space. But then you need to
> switch address space so you have to do asi_exit or asi_switch or wnatnot. And
> that's still unbalanced.
> 
> So from *only* looking at the usage, it'd be a lot more balanced if all calls
> were paired:
> 
> ioctl(KVM_RUN) {
>     enter_from_user_mode()
>     asi_switch_to();			<-------+
>     while !need_userspace_handling() {		|
>         asi_enter();  // part 1		<---+	|
>         vmenter();  // part 2		    |	|
>         asi_exit(); // part 3		<---+	|
>     }						|
>     asi_switch_back(); // part 4b	<-------+
>     exit_to_user_mode()
> }
> 
> (look at me doing ascii paintint :-P)
> 
> Naming is awful but it should illustrate what I mean:
> 
> 	asi_switch_to
> 	  asi_enter
> 	  asi_exit
> 	asi_switch_back
> 
> Does that make more sense?

Yeah I see what you mean. I think the issues are:

1. We're mixing up two different aspects in the API:

   a. Starting and finishing "critical sections" (i.e. the region
      between asi_enter() and asi_relax())

   b. Actually triggering address space transitions.

2. There is a fundamental asymmetry at play here: asi_enter() and
   asi_exit() can both be NOPs (when we're already in the relevant
   address space), and asi_enter() being a NOP is really the _whole
   point of ASI_.

   The ideal world is where asi_exit() is very very rare, so
   asi_enter() is almost always a NOP.

So we could disentangle part 1 by just rejigging things as you suggest,
and I think the naming would be like:

asi_enter
  asi_start_critical
  asi_end_critical
asi_exit

But the issue with that is that asi_start_critical() _must_ imply
asi_enter() (otherwise if we get an NMI between asi_enter() and
asi_start_critical(), and that causes a #PF, we will start the
critical section in the wrong address space and ASI won't do its job).
So, we are somewhat forced to mix up a. and b. from above.

BTW, there is another thing complicating this picture a little: ASI
"clients" (really just meaning KVM code at this point) are not not
really supposed to care at all about the actual address space, the fact
that they currently have to call asi_exit() in part 4b is just a
temporary thing to simplify the initial implementation. It has a
performance cost (not enormous, serious KVM platforms try pretty hard
to avoid returning to user space, but it does still matter) so
Google's internal version has already got rid of it and that's where I
expect this thing to evolve too. But for now it just lets us keep
things simple since e.g. we never have to think about context
switching in the restricted address space.

With that in mind, what if it looked like this:

ioctl(KVM_RUN) {
    enter_from_user_mode()
    while !need_userspace_handling()
	// This implies asi_enter(), but this code "doesn't care"
	// about that.
        asi_start_critical();
        vmenter();
        asi_end_critical();
    }
    // TODO: This is temporary, it should not be needed.
    asi_exit();
    exit_to_user_mode()
}

Once the asi_exit() call disappears, it will be symmetrical from the
"client API"'s point of view. And while we still mix up address space
switching with critical section boundaries, the address space
switching is "just an implementation detail" and not really visible as
part of the API.

> Documentation/process/email-clients.rst

I have now setup Mutt. But, for now I am replying with plan vim +
git-send-email, because I also sent this RFC to a ridiculous CC list
(I just blindly used the get_maintainers.pl output, I don't know why I
thought that was a reasonable approach) and it turns out this is the
easiest way to trim it in a reply! Hopefully I can get the headers
right...