Hi Janosch,
Here is a new (final?) version for the AR/MEM_OP issue I'm attempting to address. Hopefully they can be picked up to whatever tree makes sense.
Changes: v4: [HC,CB,NSG] Add tags (thank you!) [NSG] Add commentary v3: https://lore.kernel.org/r/20240216213616.3819805-1-farman@linux.ibm.com/ [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 | 2 ++ arch/s390/kvm/gaccess.c | 3 ++- arch/s390/kvm/kvm-s390.c | 3 +++ tools/testing/selftests/kvm/s390x/memop.c | 31 +++++++++++++++++++++++ 4 files changed, 38 insertions(+), 1 deletion(-)
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.
Reviewed-by: Heiko Carstens hca@linux.ibm.com Reviewed-by: Nina Schoetterl-Glausch nsg@linux.ibm.com Acked-by: Christian Borntraeger borntraeger@linux.ibm.com Reviewed-by: Christian Borntraeger borntraeger@linux.ibm.com Signed-off-by: Eric Farman farman@linux.ibm.com --- arch/s390/include/asm/kvm_host.h | 2 ++ arch/s390/kvm/gaccess.c | 3 ++- arch/s390/kvm/kvm-s390.c | 3 +++ 3 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h index 52664105a473..aee2d3a6254b 100644 --- a/arch/s390/include/asm/kvm_host.h +++ b/arch/s390/include/asm/kvm_host.h @@ -765,6 +765,8 @@ struct kvm_vcpu_arch { __u64 cputm_start; bool gs_enabled; bool skey_enabled; + /* Indicator if the access registers have been loaded from guest */ + 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;
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.
Reviewed-by: Nina Schoetterl-Glausch nsg@linux.ibm.com Acked-by: Christian Borntraeger borntraeger@linux.ibm.com Signed-off-by: Eric Farman farman@linux.ibm.com --- tools/testing/selftests/kvm/s390x/memop.c | 31 +++++++++++++++++++++++ 1 file changed, 31 insertions(+)
diff --git a/tools/testing/selftests/kvm/s390x/memop.c b/tools/testing/selftests/kvm/s390x/memop.c index bb3ca9a5d731..b6da8f71ea19 100644 --- a/tools/testing/selftests/kvm/s390x/memop.c +++ b/tools/testing/selftests/kvm/s390x/memop.c @@ -375,6 +375,32 @@ 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 */ + + /* + * Primary address space gets used if an access register + * contains zero. The host makes use of AR[1] so is a good + * candidate to ensure the guest AR (of zero) is used. + */ + 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); + + 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 +1127,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,
On 2/20/24 22:12, Eric Farman wrote:
Hi Janosch,
Here is a new (final?) version for the AR/MEM_OP issue I'm attempting to address. Hopefully they can be picked up to whatever tree makes sense.
I've got good and bad news for you :)
You need to re-base this patch set on Heiko's feature branch once my kvm fpu patch is on there since the current version runs into conflicts with Heiko's fpu rework. We'll contact you once that's the case. The patch made it onto devel yesterday evening and I assumed you'd wait a bit until sending a new version but I was mistaken.
Apart from that, there's not much to do. Drop Christian's Acks, as I said, they were solely for the question if we'll move your patches via Heiko's repo. Ack + rev-by doesn't make sense anyway.
Tanks for taking on this problem and fixing it!
On Wed, Feb 21, 2024 at 08:49:58AM +0100, Janosch Frank wrote:
On 2/20/24 22:12, Eric Farman wrote:
Hi Janosch,
Here is a new (final?) version for the AR/MEM_OP issue I'm attempting to address. Hopefully they can be picked up to whatever tree makes sense.
I've got good and bad news for you :)
You need to re-base this patch set on Heiko's feature branch once my kvm fpu patch is on there since the current version runs into conflicts with Heiko's fpu rework. We'll contact you once that's the case. The patch made it onto devel yesterday evening and I assumed you'd wait a bit until sending a new version but I was mistaken.
I resolved the trivial merge conflict - no need to resend anything.
Series applied, thanks!
On Wed, 2024-02-21 at 12:26 +0100, Heiko Carstens wrote:
On Wed, Feb 21, 2024 at 08:49:58AM +0100, Janosch Frank wrote:
On 2/20/24 22:12, Eric Farman wrote:
Hi Janosch,
Here is a new (final?) version for the AR/MEM_OP issue I'm attempting to address. Hopefully they can be picked up to whatever tree makes sense.
I've got good and bad news for you :)
You need to re-base this patch set on Heiko's feature branch once my kvm fpu patch is on there since the current version runs into conflicts with Heiko's fpu rework. We'll contact you once that's the case. The patch made it onto devel yesterday evening and I assumed you'd wait a bit until sending a new version but I was mistaken.
I resolved the trivial merge conflict - no need to resend anything.
Series applied, thanks!
Ah, sorry about that, I didn't notice the other fpu stuff go through. Thank you, Heiko!
linux-kselftest-mirror@lists.linaro.org
-
Eric Farman -
Heiko Carstens -
Janosch Frank