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@xxxxxxxxxxxxx> > > > Signed-off-by: Eric Farman <farman@xxxxxxxxxxxxx> > > > --- > > > 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.
