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

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

[Eric Farman]

Hi Christian, Janosch, Heiko,

Here is a pair of patches to replace the RFC I recently sent [1].

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

[1] 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/kvm/kvm-s390.c                  |  6 +++++
 tools/testing/selftests/kvm/s390x/memop.c | 28 +++++++++++++++++++++++
 2 files changed, 34 insertions(+)

-- 
2.40.1

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

[Eric Farman]

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

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;
 }
-- 
2.40.1

[PATCH v1 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 v1 1/2] KVM: s390: load guest access registers in MEM_OP ioctl

[Heiko Carstens]

On Fri, Feb 09, 2024 at 09:45:38PM +0100, Eric Farman wrote:
> 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(+)
> 
> 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);

I guess we will end up with more and more of such constructs until nobody
knows when which register contents are loaded. I would higly prefer a TIF flag
which indicates if the access registers contain the host or guest register
contents, and actual users grab the required content from the correct location
- or better: always take it from guest save area, and write to the guest save
area if the to be invented TIF flag indicates that access registers contain
guest registers...

Or maybe a TIF flag with different semantics: "guest save area does not
reflect current state - which is within registers".

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

[Heiko Carstens]

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. But I guess there must be
some arch specific vcpu flags, which can be used to achieve the same?

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

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

[Eric Farman]

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