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

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

[Eric Farman]

Hi Christian, Janosch, Heiko,

Here is a new version for the AR/MEM_OP issue I'm attempting to address.
(Thank you, Heiko, for the offline chat!)

Changes:
v3:
	[HC] Drop the AR swap in MEM_OP path
	[HC] Remove WARN and don't do save_access_regs on !bool
v2: https://lore.kernel.org/r/20240215205344.2562020-1-farman@linux.ibm.com/
	[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: fix access register usage in ioctls
  KVM: s390: selftests: memop: add a simple AR test

 arch/s390/include/asm/kvm_host.h          |  1 +
 arch/s390/kvm/gaccess.c                   |  3 ++-
 arch/s390/kvm/kvm-s390.c                  |  3 +++
 tools/testing/selftests/kvm/s390x/memop.c | 28 +++++++++++++++++++++++
 4 files changed, 34 insertions(+), 1 deletion(-)

-- 
2.40.1

[PATCH v3 1/2] KVM: s390: fix access register usage in ioctls

[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).
Since this routine saves the current registers to vcpu->run,
this routine erroneously saves host registers into the guest space.

Introduce a boolean in the kvm_vcpu_arch struct to indicate whether
the registers contain guest contents. If they do (the instruction
intercept path), the save can be performed and the AR translation
is done just as it is today. If they don't (the MEM_OP path), the
AR can be read from vcpu->run without stashing the current contents.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 arch/s390/include/asm/kvm_host.h | 1 +
 arch/s390/kvm/gaccess.c          | 3 ++-
 arch/s390/kvm/kvm-s390.c         | 3 +++
 3 files changed, 6 insertions(+), 1 deletion(-)

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..b4c805092021 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 (vcpu->arch.acrs_loaded)
+		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..61092f0e0a66 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;
-- 
2.40.1

Re: [PATCH v3 1/2] KVM: s390: fix access register usage in ioctls

[Heiko Carstens]

On Fri, Feb 16, 2024 at 10:36:15PM +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).
> Since this routine saves the current registers to vcpu->run,
> this routine erroneously saves host registers into the guest space.
> 
> Introduce a boolean in the kvm_vcpu_arch struct to indicate whether
> the registers contain guest contents. If they do (the instruction
> intercept path), the save can be performed and the AR translation
> is done just as it is today. If they don't (the MEM_OP path), the
> AR can be read from vcpu->run without stashing the current contents.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  arch/s390/include/asm/kvm_host.h | 1 +
>  arch/s390/kvm/gaccess.c          | 3 ++-
>  arch/s390/kvm/kvm-s390.c         | 3 +++
>  3 files changed, 6 insertions(+), 1 deletion(-)

Reviewed-by: Heiko Carstens <hca@linux.ibm.com>

Re: [PATCH v3 1/2] KVM: s390: fix access register usage in ioctls

[Nina Schoetterl-Glausch]

On Fri, 2024-02-16 at 22:36 +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).
> Since this routine saves the current registers to vcpu->run,
> this routine erroneously saves host registers into the guest space.
> 
> Introduce a boolean in the kvm_vcpu_arch struct to indicate whether
> the registers contain guest contents. If they do (the instruction
> intercept path), the save can be performed and the AR translation
> is done just as it is today. If they don't (the MEM_OP path), the
> AR can be read from vcpu->run without stashing the current contents.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>

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

> ---
>  arch/s390/include/asm/kvm_host.h | 1 +
>  arch/s390/kvm/gaccess.c          | 3 ++-
>  arch/s390/kvm/kvm-s390.c         | 3 +++
>  3 files changed, 6 insertions(+), 1 deletion(-)
> 
> 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;

Not sure how descriptive that name is.
Maybe add a comment.

>  	struct kvm_s390_pv_vcpu pv;
>  	union diag318_info diag318_info;
>  };

[...]

Re: [PATCH v3 1/2] KVM: s390: fix access register usage in ioctls

[Christian Borntraeger]

Am 16.02.24 um 22:36 schrieb 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).
> Since this routine saves the current registers to vcpu->run,
> this routine erroneously saves host registers into the guest space.
> 
> Introduce a boolean in the kvm_vcpu_arch struct to indicate whether
> the registers contain guest contents. If they do (the instruction
> intercept path), the save can be performed and the AR translation
> is done just as it is today. If they don't (the MEM_OP path), the
> AR can be read from vcpu->run without stashing the current contents.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>

Acked-by: Christian Borntraeger <borntraeger@linux.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>
> ---
>   arch/s390/include/asm/kvm_host.h | 1 +
>   arch/s390/kvm/gaccess.c          | 3 ++-
>   arch/s390/kvm/kvm-s390.c         | 3 +++
>   3 files changed, 6 insertions(+), 1 deletion(-)
> 
> 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..b4c805092021 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 (vcpu->arch.acrs_loaded)
> +		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..61092f0e0a66 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;

Re: [PATCH v3 1/2] KVM: s390: fix access register usage in ioctls

[Janosch Frank]

On 2/20/24 15:15, Christian Borntraeger wrote:
> 
> 
> Am 16.02.24 um 22:36 schrieb 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).
>> Since this routine saves the current registers to vcpu->run,
>> this routine erroneously saves host registers into the guest space.
>>
>> Introduce a boolean in the kvm_vcpu_arch struct to indicate whether
>> the registers contain guest contents. If they do (the instruction
>> intercept path), the save can be performed and the AR translation
>> is done just as it is today. If they don't (the MEM_OP path), the
>> AR can be read from vcpu->run without stashing the current contents.
>>
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> 

Just spoke with Christian since that's a mixed signal.

To move this over the s390 tree:
> Acked-by: Christian Borntraeger <borntraeger@linux.ibm.com>

For the Patch in general:
> Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>

Re: [PATCH v3 1/2] KVM: s390: fix access register usage in ioctls

[Eric Farman]

On Tue, 2024-02-20 at 15:19 +0100, Janosch Frank wrote:
> On 2/20/24 15:15, Christian Borntraeger wrote:
> > 
> > 
> > Am 16.02.24 um 22:36 schrieb 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).
> > > Since this routine saves the current registers to vcpu->run,
> > > this routine erroneously saves host registers into the guest
> > > space.
> > > 
> > > Introduce a boolean in the kvm_vcpu_arch struct to indicate
> > > whether
> > > the registers contain guest contents. If they do (the instruction
> > > intercept path), the save can be performed and the AR translation
> > > is done just as it is today. If they don't (the MEM_OP path), the
> > > AR can be read from vcpu->run without stashing the current
> > > contents.
> > > 
> > > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> > 
> 
> Just spoke with Christian since that's a mixed signal.
> 
> To move this over the s390 tree:
> > Acked-by: Christian Borntraeger <borntraeger@linux.ibm.com>
> 
> For the Patch in general:
> > Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>
> 

Shall I send a v4 with the tags, and comments Nina suggested, then?

[PATCH v3 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 v3 2/2] KVM: s390: selftests: memop: add a simple AR test

[Christian Borntraeger]

Am 16.02.24 um 22:36 schrieb 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>

Acked-by: Christian Borntraeger <borntraeger@linux.ibm.com>