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?
- /* Swap host/guest access registers */
- save_access_regs(vcpu->arch.host_acrs);
- restore_access_regs(vcpu->run->s.regs.acrs);
- vcpu->arch.acrs_loaded = true;
- 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 +5428,9 @@ 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);
- vcpu->arch.acrs_loaded = false;
... these two hunks wouldn't be required if the code above would be changed like I proposed.