| Message ID | 20240209204539.4150550-2-farman@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: s390: Fix AR parameter in MEM_OP ioctl | expand |
On Mon, 2024-02-12 at 12:52 +0100, Heiko Carstens wrote: > > On Mon, Feb 12, 2024 at 11:21:30AM +0100, Heiko Carstens wrote: > > > > Or maybe a TIF flag with different semantics: "guest save area > > > > does > > > > not > > > > reflect current state - which is within registers". > > > > Something like the below; untested of course. Ooops, yeah. Christian suggested something similar in his first response to the RFC which I'd overlooked. > > But I guess there must be > > some arch specific vcpu flags, which can be used to achieve the > > same? Agreed. Putting something there probably makes sense to keep it in the KVM sphere > > > > diff --git a/arch/s390/include/asm/thread_info.h > > b/arch/s390/include/asm/thread_info.h > > index a674c7d25da5..b9ff8b125fb8 100644 > > --- a/arch/s390/include/asm/thread_info.h > > +++ b/arch/s390/include/asm/thread_info.h > > @@ -69,6 +69,7 @@ void arch_setup_new_exec(void); > > #define TIF_PATCH_PENDING 5 /* pending live patching update */ > > #define TIF_PGSTE 6 /* New mm's will use 4K page tables */ > > #define TIF_NOTIFY_SIGNAL 7 /* signal notifications exist */ > > +#define TIF_KVM_ACRS 8 /* access registers contain guest content > > */ > > #define TIF_ISOLATE_BP_GUEST 9 /* Run KVM guests with isolated BP > > */ > > #define TIF_PER_TRAP 10 /* Need to handle PER trap on exit to > > usermode */ > > > > diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c > > index 5bfcc50c1a68..b0ef242d2371 100644 > > --- a/arch/s390/kvm/gaccess.c > > +++ b/arch/s390/kvm/gaccess.c > > @@ -391,7 +391,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); > > + if (test_thread_flag(TIF_KVM_ACRS)) > > + save_access_regs(vcpu->run->s.regs.acrs); ...or WARN if not set, so that we know of the missing path. Will send this all as a v2. Thanks. > > alet.val = vcpu->run->s.regs.acrs[ar]; > > > > if (ar == 0 || alet.val == 0) { > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > > index ea63ac769889..3ee0913639d5 100644 > > --- a/arch/s390/kvm/kvm-s390.c > > +++ b/arch/s390/kvm/kvm-s390.c > > @@ -4951,6 +4951,7 @@ static void sync_regs(struct kvm_vcpu *vcpu) > > } > > save_access_regs(vcpu->arch.host_acrs); > > restore_access_regs(vcpu->run->s.regs.acrs); > > + set_thread_flag(TIF_KVM_ACRS); > > /* save host (userspace) fprs/vrs */ > > save_fpu_regs(); > > vcpu->arch.host_fpregs.fpc = current->thread.fpu.fpc; > > @@ -5020,6 +5021,7 @@ static void store_regs(struct kvm_vcpu *vcpu) > > kvm_run->s.regs.pfs = vcpu->arch.pfault_select; > > kvm_run->s.regs.pfc = vcpu->arch.pfault_compare; > > save_access_regs(vcpu->run->s.regs.acrs); > > + clear_thread_flag(TIF_KVM_ACRS); > > restore_access_regs(vcpu->arch.host_acrs); > > /* Save guest register state */ > > save_fpu_regs();
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index ea63ac769889..c2dfeea55dcf 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -5391,6 +5391,10 @@ static long kvm_s390_vcpu_mem_op(struct kvm_vcpu *vcpu, return -ENOMEM; } + /* Swap host/guest access registers in the event of a MEM_OP with AR specified */ + save_access_regs(vcpu->arch.host_acrs); + restore_access_regs(vcpu->run->s.regs.acrs); + acc_mode = mop->op == KVM_S390_MEMOP_LOGICAL_READ ? GACC_FETCH : GACC_STORE; if (mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY) { r = check_gva_range(vcpu, mop->gaddr, mop->ar, mop->size, @@ -5420,6 +5424,8 @@ static long kvm_s390_vcpu_mem_op(struct kvm_vcpu *vcpu, kvm_s390_inject_prog_irq(vcpu, &vcpu->arch.pgm); out_free: + save_access_regs(vcpu->run->s.regs.acrs); + restore_access_regs(vcpu->arch.host_acrs); vfree(tmpbuf); return r; }
The routine ar_translation() is called by get_vcpu_asce(), which is called by both the instruction intercept path (where the access registers had been loaded with the guest's values), and the MEM_OP ioctl (which hadn't). This means that any ALET the guest expects to be used would be ignored. Furthermore, the logic in ar_translation() will store the contents of the access registers back to the KVM_RUN struct. This unexpected change of AR values can lead to problems after invoking the MEM_OP, for example an ALET Specification Exception. 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. Suggested-by: Christian Borntraeger <borntraeger@linux.ibm.com> Signed-off-by: Eric Farman <farman@linux.ibm.com> --- arch/s390/kvm/kvm-s390.c | 6 ++++++ 1 file changed, 6 insertions(+)