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

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

On Thu, 2024-02-01 at 16:14 +0100, Heiko Carstens wrote:
> On Wed, Jan 31, 2024 at 09:58:32PM +0100, Eric Farman wrote:
> > 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.
> 
> What's the code path that can lead to this scenario?

When processing a KVM_RUN ioctl, the kernel is going to swap the
host/guest access registers in sync_regs, enter SIE, and then swap them
back in store_regs when it has to exit to userspace. So then on the
QEMU side it might look something like this:

kvm_arch_handle_exit
  handle_intercept
    handle_instruction
      handle_b2
        ioinst_handle_stsch
          s390_cpu_virt_mem_rw(ar=0xe, is_write=true)
            kvm_s390_mem_op

Where the interesting registers at that point are:

acr0           0x3ff               1023
acr1           0x33bff8c0          868219072
...
acr14          0x0                 0

Note ACR0/1 are already buggered up from an earlier pass.

The above carries us through the kernel this way:

kvm_arch_vcpu_ioctl(KVM_S390_MEM_OP)
  kvm_s390_vcpu_memsida_op
    kvm_s390_vcpu_mem_op
      write_guest_with_key
        access_guest_with_key
          get_vcpu_asce
            ar_translate
              save_access_regs(kvm_run)

> 
> >  arch/s390/kvm/gaccess.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
> > index 5bfcc50c1a68..9205496195a4 100644
> > --- a/arch/s390/kvm/gaccess.c
> > +++ b/arch/s390/kvm/gaccess.c
> > @@ -380,6 +380,7 @@ void ipte_unlock(struct kvm *kvm)
> >  static int ar_translation(struct kvm_vcpu *vcpu, union asce *asce,
> > u8 ar,
> >  			  enum gacc_mode mode)
> >  {
> > +	int acrs[NUM_ACRS];
> >  	union alet alet;
> >  	struct ale ale;
> >  	struct aste aste;
> > @@ -391,8 +392,8 @@ static int ar_translation(struct kvm_vcpu
> > *vcpu, union asce *asce, u8 ar,
> >  	if (ar >= NUM_ACRS)
> >  		return -EINVAL;
> >  
> > -	save_access_regs(vcpu->run->s.regs.acrs);
> > -	alet.val = vcpu->run->s.regs.acrs[ar];
> > +	save_access_regs(acrs);
> > +	alet.val = acrs[ar];
> 
> If the above is like you said, then this code would use the host
> access register contents for ar translation of the guest?
> 
> Or maybe I'm simply misunderstanding what you write.

Well regardless of this patch, I think it's using the contents of the
host registers today, isn't it? save_access_regs() does a STAM to put
the current registers into some bit of memory, so ar_translation() can
do regular logic against it. The above just changes WHERE that bit of
memory lives from something shared with another ioctl to something
local to ar_translation(). 

My original change just removed the save_access_regs() call entirely
and read the contents of the kvm_run struct since they were last saved
(see below). This "feels" better to me, and works for the scenario I
bumped into too. Maybe this is more appropriate?

---8<---

diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
index 5bfcc50c1a68..c5ed3b0b665a 100644
--- a/arch/s390/kvm/gaccess.c
+++ b/arch/s390/kvm/gaccess.c
@@ -391,7 +391,6 @@ static int ar_translation(struct kvm_vcpu *vcpu,
union asce *asce, u8 ar,
        if (ar >= NUM_ACRS)
                return -EINVAL;
 
-       save_access_regs(vcpu->run->s.regs.acrs);
        alet.val = vcpu->run->s.regs.acrs[ar];
 
        if (ar == 0 || alet.val == 0) {