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

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

On Tue, 2024-02-06 at 16:47 +0100, Heiko Carstens wrote:
> On Thu, Feb 01, 2024 at 11:56:26AM -0500, Eric Farman wrote:
> > 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:
> > > 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)
> 
> ...
> 
> > 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(). 
> 
> This seems to be true; but there are also other code paths which can
> reach ar_translation() where the access register contents actually do
> belong to the guest (e.g. intercept handling).

Right, the trouble is that both scenarios end up here. Storing the
access registers here in the intercept path "doesn't hurt" because
it'll be done again when we clean up after the SIE exit (store_regs()),
and the original patch keeps the behavior the same for the memop case
without disrupting the kvm_run space. I agree this behavior doesn't
seem right.

> 
> > 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);
> 
> I guess this and your previous patch are both not correct.

Yay? :)

>  There is
> different handling required depending on if current access register
> contents belong to the host or guest (both seems to be possible),
> when
> the function is entered.
> 
> But anyway, I'll leave that up to Janosch and Claudio, just had a
> quick
> look at your patch :)
> 

Thanks for the quick look. Will wait for Janosch and Claudio to get a
couple cycles.