| Message ID | 20240215205344.2562020-1-farman@linux.ibm.com |
|---|---|
| Headers | show |
| Series | KVM: s390: Fix AR parameter in MEM_OP ioctl | expand |
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. If the WARN triggers, damage will be done if the ARs get copied back to the vcpu->run space (I don't believe any damage has occurred at the time of the WARN). That's what's happening today and I'd like to address, but there's no indication of what's happened. Perhaps I need to combine the two ideas? Do the WARN, but remove the save_access_regs() call since it gets done again once the registers are swapped back. Or keep it, and dig out the RFC code that stores the current ARs into a temporary variable instead? Thanks, Eric
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.
Hi Christian, Janosch, Heiko, Here is a new version for the AR/MEM_OP issue I'm attempting to address, with Heiko's feedback to v1. Patch 1 performs the host/guest access register swap that Christian suggested (instead of a full sync_reg/store_reg process). Patch 2 provides a selftest patch that exercises this scenario. Applying patch 2 without patch 1 fails in the following way: [eric@host linux]# ./tools/testing/selftests/kvm/s390x/memop TAP version 13 1..16 ok 1 simple copy ok 2 generic error checks ok 3 copy with storage keys ok 4 cmpxchg with storage keys ok 5 concurrently cmpxchg with storage keys ok 6 copy with key storage protection override ok 7 copy with key fetch protection ok 8 copy with key fetch protection override ==== Test Assertion Failure ==== s390x/memop.c:186: !r pid=5720 tid=5720 errno=4 - Interrupted system call 1 0x00000000010042af: memop_ioctl at memop.c:186 (discriminator 3) 2 0x0000000001006697: test_copy_access_register at memop.c:400 (discriminator 2) 3 0x0000000001002aaf: main at memop.c:1181 4 0x000003ffaec33a5b: ?? ??:0 5 0x000003ffaec33b5d: ?? ??:0 6 0x0000000001002ba9: _start at ??:? KVM_S390_MEM_OP failed, rc: 40 errno: 4 (Interrupted system call) Thoughts on this approach? Thanks, Eric Changes: v2: [HC] Add a flag to indicate access registers have been loaded v1: https://lore.kernel.org/r/20240209204539.4150550-1-farman@linux.ibm.com/ [CB] Store access registers around memop ioctl [JF] Add a kernel selftest RFC: https://lore.kernel.org/r/20240131205832.2179029-1-farman@linux.ibm.com/ Eric Farman (2): KVM: s390: load guest access registers in MEM_OP ioctl KVM: s390: selftests: memop: add a simple AR test arch/s390/include/asm/kvm_host.h | 1 + arch/s390/kvm/gaccess.c | 2 ++ arch/s390/kvm/kvm-s390.c | 11 +++++++++ tools/testing/selftests/kvm/s390x/memop.c | 28 +++++++++++++++++++++++ 4 files changed, 42 insertions(+)