[PATCH v2 0/2] KVM: s390: Fix AR parameter in MEM_OP ioctl

[PATCH v2 0/2] KVM: s390: Fix AR parameter in MEM_OP ioctl

[Eric Farman]

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(+)

-- 
2.40.1

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

[Eric Farman]

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/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 52664105a473..c86215eb4ca7 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -765,6 +765,7 @@ struct kvm_vcpu_arch {
 	__u64 cputm_start;
 	bool gs_enabled;
 	bool skey_enabled;
+	bool acrs_loaded;
 	struct kvm_s390_pv_vcpu pv;
 	union diag318_info diag318_info;
 };
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);
 	alet.val = vcpu->run->s.regs.acrs[ar];
 
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index ea63ac769889..c60ec561f7f1 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -3951,6 +3951,7 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
 				    KVM_SYNC_ARCH0 |
 				    KVM_SYNC_PFAULT |
 				    KVM_SYNC_DIAG318;
+	vcpu->arch.acrs_loaded = false;
 	kvm_s390_set_prefix(vcpu, 0);
 	if (test_kvm_facility(vcpu->kvm, 64))
 		vcpu->run->kvm_valid_regs |= KVM_SYNC_RICCB;
@@ -4951,6 +4952,7 @@ static void sync_regs(struct kvm_vcpu *vcpu)
 	}
 	save_access_regs(vcpu->arch.host_acrs);
 	restore_access_regs(vcpu->run->s.regs.acrs);
+	vcpu->arch.acrs_loaded = true;
 	/* save host (userspace) fprs/vrs */
 	save_fpu_regs();
 	vcpu->arch.host_fpregs.fpc = current->thread.fpu.fpc;
@@ -5021,6 +5023,7 @@ static void store_regs(struct kvm_vcpu *vcpu)
 	kvm_run->s.regs.pfc = vcpu->arch.pfault_compare;
 	save_access_regs(vcpu->run->s.regs.acrs);
 	restore_access_regs(vcpu->arch.host_acrs);
+	vcpu->arch.acrs_loaded = false;
 	/* Save guest register state */
 	save_fpu_regs();
 	vcpu->run->s.regs.fpc = current->thread.fpu.fpc;
@@ -5391,6 +5394,11 @@ static long kvm_s390_vcpu_mem_op(struct kvm_vcpu *vcpu,
 			return -ENOMEM;
 	}
 
+	/* 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;
 	vfree(tmpbuf);
 	return r;
 }
-- 
2.40.1

[PATCH v2 2/2] KVM: s390: selftests: memop: add a simple AR test

[Eric Farman]

There is a selftest that checks for an (expected) error when an
invalid AR is specified, but not one that exercises the AR path.

Add a simple test that mirrors the vanilla write/read test while
providing an AR. An AR that contains zero will direct the CPU to
use the primary address space normally used anyway. AR[1] is
selected for this test because the host AR[1] is usually non-zero,
and KVM needs to correctly swap those values.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 tools/testing/selftests/kvm/s390x/memop.c | 28 +++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/tools/testing/selftests/kvm/s390x/memop.c b/tools/testing/selftests/kvm/s390x/memop.c
index bb3ca9a5d731..be20c26ee545 100644
--- a/tools/testing/selftests/kvm/s390x/memop.c
+++ b/tools/testing/selftests/kvm/s390x/memop.c
@@ -375,6 +375,29 @@ static void test_copy(void)
 	kvm_vm_free(t.kvm_vm);
 }
 
+static void test_copy_access_register(void)
+{
+	struct test_default t = test_default_init(guest_copy);
+
+	HOST_SYNC(t.vcpu, STAGE_INITED);
+
+	prepare_mem12();
+	t.run->psw_mask &= ~(3UL << (63 - 17));
+	t.run->psw_mask |= 1UL << (63 - 17);  /* Enable AR mode */
+
+	CHECK_N_DO(MOP, t.vcpu, LOGICAL, WRITE, mem1, t.size,
+		   GADDR_V(mem1), AR(1));
+	HOST_SYNC(t.vcpu, STAGE_COPIED);
+
+	CHECK_N_DO(MOP, t.vcpu, LOGICAL, READ, mem2, t.size,
+		   GADDR_V(mem2), AR(1));
+	ASSERT_MEM_EQ(mem1, mem2, t.size);
+
+	t.run->psw_mask &= ~(3UL << (63 - 17));   /* Disable AR mode */
+
+	kvm_vm_free(t.kvm_vm);
+}
+
 static void set_storage_key_range(void *addr, size_t len, uint8_t key)
 {
 	uintptr_t _addr, abs, i;
@@ -1101,6 +1124,11 @@ int main(int argc, char *argv[])
 			.test = test_copy_key_fetch_prot_override,
 			.requirements_met = extension_cap > 0,
 		},
+		{
+			.name = "copy with access register mode",
+			.test = test_copy_access_register,
+			.requirements_met = true,
+		},
 		{
 			.name = "error checks with key",
 			.test = test_errors_key,
-- 
2.40.1

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

[Heiko Carstens]

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.

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

[Eric Farman]

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

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

[Eric Farman]

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.

Re: [PATCH v2 2/2] KVM: s390: selftests: memop: add a simple AR test

[Nina Schoetterl-Glausch]

On Thu, 2024-02-15 at 21:53 +0100, Eric Farman wrote:
> There is a selftest that checks for an (expected) error when an
> invalid AR is specified, but not one that exercises the AR path.
> 
> Add a simple test that mirrors the vanilla write/read test while
> providing an AR. An AR that contains zero will direct the CPU to
> use the primary address space normally used anyway. AR[1] is
> selected for this test because the host AR[1] is usually non-zero,
> and KVM needs to correctly swap those values.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>

Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>

> ---
>  tools/testing/selftests/kvm/s390x/memop.c | 28 +++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/s390x/memop.c b/tools/testing/selftests/kvm/s390x/memop.c
> index bb3ca9a5d731..be20c26ee545 100644
> --- a/tools/testing/selftests/kvm/s390x/memop.c
> +++ b/tools/testing/selftests/kvm/s390x/memop.c
> @@ -375,6 +375,29 @@ static void test_copy(void)
>  	kvm_vm_free(t.kvm_vm);
>  }
> 
> +static void test_copy_access_register(void)
> +{
> +	struct test_default t = test_default_init(guest_copy);
> +
> +	HOST_SYNC(t.vcpu, STAGE_INITED);
> +
> +	prepare_mem12();
> +	t.run->psw_mask &= ~(3UL << (63 - 17));
> +	t.run->psw_mask |= 1UL << (63 - 17);  /* Enable AR mode */
> +

I feel like part of the commit message should be a comment here

/*
 * Guest AR[1] should be zero, in which case the primary address space is used.
 * The host makes use of AR[1], its value must not be used for the memop.
 */
> +	CHECK_N_DO(MOP, t.vcpu, LOGICAL, WRITE, mem1, t.size,
> +		   GADDR_V(mem1), AR(1));
> +	HOST_SYNC(t.vcpu, STAGE_COPIED);
> +
> +	CHECK_N_DO(MOP, t.vcpu, LOGICAL, READ, mem2, t.size,
> +		   GADDR_V(mem2), AR(1));
> +	ASSERT_MEM_EQ(mem1, mem2, t.size);
> +
> +	t.run->psw_mask &= ~(3UL << (63 - 17));   /* Disable AR mode */

Any reason for this? It's not necessary since the vm is going to be destroyed.
> +
> +	kvm_vm_free(t.kvm_vm);
> +}
> +
>  static void set_storage_key_range(void *addr, size_t len, uint8_t key)
>  {
>  	uintptr_t _addr, abs, i;
> @@ -1101,6 +1124,11 @@ int main(int argc, char *argv[])
>  			.test = test_copy_key_fetch_prot_override,
>  			.requirements_met = extension_cap > 0,
>  		},
> +		{
> +			.name = "copy with access register mode",
> +			.test = test_copy_access_register,
> +			.requirements_met = true,
> +		},
>  		{
>  			.name = "error checks with key",
>  			.test = test_errors_key,

Re: [PATCH v2 2/2] KVM: s390: selftests: memop: add a simple AR test

[Eric Farman]

On Tue, 2024-02-20 at 14:34 +0100, Nina Schoetterl-Glausch wrote:
> On Thu, 2024-02-15 at 21:53 +0100, Eric Farman wrote:
> > There is a selftest that checks for an (expected) error when an
> > invalid AR is specified, but not one that exercises the AR path.
> > 
> > Add a simple test that mirrors the vanilla write/read test while
> > providing an AR. An AR that contains zero will direct the CPU to
> > use the primary address space normally used anyway. AR[1] is
> > selected for this test because the host AR[1] is usually non-zero,
> > and KVM needs to correctly swap those values.
> > 
> > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> 
> Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> 
> > ---
> >  tools/testing/selftests/kvm/s390x/memop.c | 28
> > +++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/kvm/s390x/memop.c
> > b/tools/testing/selftests/kvm/s390x/memop.c
> > index bb3ca9a5d731..be20c26ee545 100644
> > --- a/tools/testing/selftests/kvm/s390x/memop.c
> > +++ b/tools/testing/selftests/kvm/s390x/memop.c
> > @@ -375,6 +375,29 @@ static void test_copy(void)
> >  	kvm_vm_free(t.kvm_vm);
> >  }
> > 
> > +static void test_copy_access_register(void)
> > +{
> > +	struct test_default t = test_default_init(guest_copy);
> > +
> > +	HOST_SYNC(t.vcpu, STAGE_INITED);
> > +
> > +	prepare_mem12();
> > +	t.run->psw_mask &= ~(3UL << (63 - 17));
> > +	t.run->psw_mask |= 1UL << (63 - 17);  /* Enable AR mode */
> > +
> 
> I feel like part of the commit message should be a comment here
> 
> /*
>  * Guest AR[1] should be zero, in which case the primary address
> space is used.
>  * The host makes use of AR[1], its value must not be used for the
> memop.
>  */
> > +	CHECK_N_DO(MOP, t.vcpu, LOGICAL, WRITE, mem1, t.size,
> > +		   GADDR_V(mem1), AR(1));
> > +	HOST_SYNC(t.vcpu, STAGE_COPIED);
> > +
> > +	CHECK_N_DO(MOP, t.vcpu, LOGICAL, READ, mem2, t.size,
> > +		   GADDR_V(mem2), AR(1));
> > +	ASSERT_MEM_EQ(mem1, mem2, t.size);
> > +
> > +	t.run->psw_mask &= ~(3UL << (63 - 17));   /* Disable AR
> > mode */
> 
> Any reason for this? It's not necessary since the vm is going to be
> destroyed.

Nah. I think I was just putting it back the way I was, in case I did
slap another test on there, but never did.

> > +
> > +	kvm_vm_free(t.kvm_vm);
> > +}
> > +
> >  static void set_storage_key_range(void *addr, size_t len, uint8_t
> > key)
> >  {
> >  	uintptr_t _addr, abs, i;
> > @@ -1101,6 +1124,11 @@ int main(int argc, char *argv[])
> >  			.test = test_copy_key_fetch_prot_override,
> >  			.requirements_met = extension_cap > 0,
> >  		},
> > +		{
> > +			.name = "copy with access register mode",
> > +			.test = test_copy_access_register,
> > +			.requirements_met = true,
> > +		},
> >  		{
> >  			.name = "error checks with key",
> >  			.test = test_errors_key,
>