Re: [RFC PATCH] KVM: s390: remove extra copy of access registers into KVM_RUN

[Eric Farman <farman@linux.ibm.com>]

On Thu, 2024-02-08 at 14:51 +0100, Christian Borntraeger wrote:
> 
> 
> Am 08.02.24 um 13:37 schrieb Janosch Frank:
> > On 2/8/24 12:50, Christian Borntraeger wrote:
> > > Am 31.01.24 um 21:58 schrieb Eric Farman:
> > > > The routine ar_translation() is called by get_vcpu_asce(),
> > > > which is
> > > > called from a handful of places, such as an interception that
> > > > is
> > > > being handled during KVM_RUN processing. In that case, the
> > > > access
> > > > registers of the vcpu had been saved to a host_acrs struct and
> > > > then
> > > > the guest access registers loaded from the KVM_RUN struct prior
> > > > to
> > > > entering SIE. Saving them back to KVM_RUN at this point doesn't
> > > > do
> > > > any harm, since it will be done again at the end of the KVM_RUN
> > > > loop when the host access registers are restored.
> > > > 
> > > > But that's not the only path into this code. The MEM_OP ioctl
> > > > can
> > > > be used while specifying an access register, and will arrive
> > > > here.
> > > > 
> > > > Linux itself doesn't use the access registers for much, but it
> > > > does
> > > > squirrel the thread local storage variable into ACRs 0 and 1 in
> > > > copy_thread() [1]. This means that the MEM_OP ioctl may copy
> > > > non-zero access registers (the upper- and lower-halves of the
> > > > TLS
> > > > pointer) to the KVM_RUN struct, which will end up getting
> > > > propogated
> > > > to the guest once KVM_RUN ioctls occur. Since these are almost
> > > > certainly invalid as far as an ALET goes, an ALET Specification
> > > > Exception would be triggered if it were attempted to be used.
> > > > 
> > > > [1] arch/s390/kernel/process.c:169
> > > > 
> > > > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> > > > ---
> > > > 
> > > > Notes:
> > > >       I've gone back and forth about whether the correct fix is
> > > >       to simply remove the save_access_regs() call and inspect
> > > >       the contents from the most recent KVM_RUN directly,
> > > > versus
> > > >       storing the contents locally. Both work for me but I've
> > > >       opted for the latter, as it continues to behave the same
> > > >       as it does today but without the implicit use of the
> > > >       KVM_RUN space. As it is, this is (was) the only reference
> > > >       to vcpu->run in this file, which stands out since the
> > > >       routines are used by other callers.
> > > >       Curious about others' thoughts.
> > > 
> > > Given the main idea that we have the guest ARs loaded in the kvm
> > > module
> > > when running a guest and that the kernel does not use those. This
> > > avoids
> > > saving/restoring the ARs for all the fast path exits.
> > > The MEM_OP is indeed a separate path.
> > > So what about making this slightly slower by doing something like
> > > this
> > > (untested, white space damaged)

This idea seems to work fine for the case I was puzzling over.

> > 
> > We could fence AR loading/storing via the the PSW address space
> > bits for more performance and not do a full sync/store regs here.
> 
> Hmm, we would then add a conditional branch which also is not ideal.
> Maybe just load/restore the ARs instead of the full sync/save_reg
> dance?

This might work too. I'll give that a try later today.