Re: [PATCH v2 1/2] KVM: s390: load guest access registers in MEM_OP ioctl

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

On Fri, 2024-02-16 at 11:33 -0500, Eric Farman wrote:
> On Fri, 2024-02-16 at 10:40 +0100, Heiko Carstens wrote:
> > On Thu, Feb 15, 2024 at 09:53:43PM +0100, Eric Farman wrote:
> > > The routine ar_translation() can be reached by both the
> > > instruction
> > > intercept path (where the access registers had been loaded with
> > > the
> > > guest register contents), and the MEM_OP ioctls (which hadn't).
> > > This latter case means that any ALET the guest expects to be used
> > > would be ignored.
> > > 
> > > Fix this by swapping the host/guest access registers around the
> > > MEM_OP ioctl, in the same way that the KVM_RUN ioctl does with
> > > sync_regs()/store_regs(). The full register swap isn't needed
> > > here,
> > > since only the access registers are used in this interface.
> > > 
> > > Introduce a boolean in the kvm_vcpu_arch struct to indicate the
> > > guest ARs have been loaded into the registers. This permits a
> > > warning to be emitted if entering this path without a proper
> > > register setup.
> > > 
> > > Suggested-by: Christian Borntraeger <borntraeger@linux.ibm.com>
> > > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> > > ---
> > >  arch/s390/include/asm/kvm_host.h |  1 +
> > >  arch/s390/kvm/gaccess.c          |  2 ++
> > >  arch/s390/kvm/kvm-s390.c         | 11 +++++++++++
> > >  3 files changed, 14 insertions(+)
> > ...
> > > diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
> > > index 5bfcc50c1a68..33587bb4c9e8 100644
> > > --- a/arch/s390/kvm/gaccess.c
> > > +++ b/arch/s390/kvm/gaccess.c
> > > @@ -391,6 +391,8 @@ static int ar_translation(struct kvm_vcpu
> > > *vcpu, union asce *asce, u8 ar,
> > >  	if (ar >= NUM_ACRS)
> > >  		return -EINVAL;
> > >  
> > > +	WARN_ON_ONCE(!vcpu->arch.acrs_loaded);
> > > +
> > >  	save_access_regs(vcpu->run->s.regs.acrs);
> > 
> > Why not simply:
> > 
> > 	if (vcpu->arch.acrs_loaded)
> > 		save_access_regs(vcpu->run->s.regs.acrs);
> > 
> > ?
> > 
> > This will always work, and the WARN_ON_ONCE() would not be needed.
> > Besides
> > that: _if_ the WARN_ON_ONCE() would trigger, damage would have
> > happened
> > already: host registers would have been made visible to the guest.
> > 
> > Or did I miss anything?
> 
> You're right that the suggestion to skip the save_access_regs() call
> in
> this way would get the ALET out of the guest correctly, but the
> actual
> CPU AR hadn't yet been loaded with the guest contents. Thus, the data
> copy would be done with the host access register rather than the
> guest's, which is why I needed to add those two extra hunks to do an
> AR
> swap around the MEM_OP interface. Without that, the selftest in patch
> 2
> continues to fail.

Scratch that. I applied this onto some other in-progress code, so the
statement about a failing test isn't valid. You're not missing
anything, and the hunks around MEM_OP aren't needed. I'll send v3.