| Message ID | 20240815154529.628087-3-schlameuss@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] selftests: kvm: s390: Add uc_map_unmap VM test case | expand |
On 8/15/24 5:45 PM, Christoph Schlameuss wrote: > Add a test case manipulating s390 storage keys from within the ucontrol > VM. > > Signed-off-by: Christoph Schlameuss <schlameuss@linux.ibm.com> > --- > .../selftests/kvm/s390x/ucontrol_test.c | 76 +++++++++++++++++++ > 1 file changed, 76 insertions(+) > > diff --git a/tools/testing/selftests/kvm/s390x/ucontrol_test.c b/tools/testing/selftests/kvm/s390x/ucontrol_test.c > index 41306bb52f29..5f8815a80544 100644 > --- a/tools/testing/selftests/kvm/s390x/ucontrol_test.c > +++ b/tools/testing/selftests/kvm/s390x/ucontrol_test.c > @@ -79,6 +79,32 @@ asm("test_mem_asm:\n" > " j 0b\n" > ); > > +/* Test program manipulating storage keys */ > +extern char test_skey_asm[]; > +asm("test_skey_asm:\n" > + "xgr %r0, %r0\n" > + > + "0:\n" > + " ahi %r0,1\n" > + " st %r1,0(%r5,%r6)\n" > + > + " iske %r1,%r6\n" > + " ahi %r0,1\n" > + " diag 0,0,0x44\n" > + > + " sske %r1,%r6\n" Might want to add a xgr here so you're sure that you're not reading your own values if iske fails. > + " iske %r1,%r6\n" > + " ahi %r0,1\n" > + " diag 0,0,0x44\n" > + > + " rrbe %r1,%r6\n" > + " iske %r1,%r6\n" > + " ahi %r0,1\n" > + " diag 0,0,0x44\n" > + > + " j 0b\n" > +); > + > FIXTURE(uc_kvm) > { > struct kvm_s390_sie_block *sie_block; > @@ -345,6 +371,56 @@ static void uc_assert_diag44(FIXTURE_DATA(uc_kvm) * self) > TEST_ASSERT_EQ(0x440000, sie_block->ipb); > } > > +TEST_F(uc_kvm, uc_skey) > +{ > + u64 test_vaddr = self->base_gpa + VM_MEM_SIZE - (SZ_1M / 2); > + struct kvm_sync_regs *sync_regs = &self->run->s.regs; > + struct kvm_run *run = self->run; > + u8 skeyvalue = 0x34; > + > + /* copy test_skey_asm to code_hva / code_gpa */ > + TH_LOG("copy code %p to vm mapped memory %p / %p", > + &test_skey_asm, (void *)self->code_hva, (void *)self->code_gpa); > + memcpy((void *)self->code_hva, &test_skey_asm, PAGE_SIZE); > + > + /* set register content for test_skey_asm to access not mapped memory */ > + sync_regs->gprs[1] = skeyvalue; > + sync_regs->gprs[5] = self->base_gpa; > + sync_regs->gprs[6] = test_vaddr; > + run->kvm_dirty_regs |= KVM_SYNC_GPRS; > + > + self->sie_block->ictl |= ICTL_OPEREXC | ICTL_PINT; > + self->sie_block->cpuflags &= ~CPUSTAT_KSS; So you don't want KVM to initialize skeys? Or am I missing a ucontrol skey interaction? What about the ICTLs if KSS is not available on the machine?
On Fri Aug 16, 2024 at 4:36 PM CEST, Janosch Frank wrote: > On 8/15/24 5:45 PM, Christoph Schlameuss wrote: > > Add a test case manipulating s390 storage keys from within the ucontrol > > VM. > > > > Signed-off-by: Christoph Schlameuss <schlameuss@linux.ibm.com> > > --- > > .../selftests/kvm/s390x/ucontrol_test.c | 76 +++++++++++++++++++ > > 1 file changed, 76 insertions(+) > > > > diff --git a/tools/testing/selftests/kvm/s390x/ucontrol_test.c b/tools/testing/selftests/kvm/s390x/ucontrol_test.c > > index 41306bb52f29..5f8815a80544 100644 > > --- a/tools/testing/selftests/kvm/s390x/ucontrol_test.c > > +++ b/tools/testing/selftests/kvm/s390x/ucontrol_test.c > > @@ -79,6 +79,32 @@ asm("test_mem_asm:\n" > > " j 0b\n" > > ); > > > > +/* Test program manipulating storage keys */ > > +extern char test_skey_asm[]; > > +asm("test_skey_asm:\n" > > + "xgr %r0, %r0\n" > > + > > + "0:\n" > > + " ahi %r0,1\n" > > + " st %r1,0(%r5,%r6)\n" > > + > > + " iske %r1,%r6\n" > > + " ahi %r0,1\n" > > + " diag 0,0,0x44\n" > > + > > + " sske %r1,%r6\n" > > Might want to add a xgr here so you're sure that you're not reading your > own values if iske fails. > Good point. Will change the r1 value here. > > + " iske %r1,%r6\n" > > + " ahi %r0,1\n" > > + " diag 0,0,0x44\n" > > + > > + " rrbe %r1,%r6\n" > > + " iske %r1,%r6\n" > > + " ahi %r0,1\n" > > + " diag 0,0,0x44\n" > > + > > + " j 0b\n" > > +); > > + > > FIXTURE(uc_kvm) > > { > > struct kvm_s390_sie_block *sie_block; > > @@ -345,6 +371,56 @@ static void uc_assert_diag44(FIXTURE_DATA(uc_kvm) * self) > > TEST_ASSERT_EQ(0x440000, sie_block->ipb); > > } > > > > +TEST_F(uc_kvm, uc_skey) > > +{ > > + u64 test_vaddr = self->base_gpa + VM_MEM_SIZE - (SZ_1M / 2); > > + struct kvm_sync_regs *sync_regs = &self->run->s.regs; > > + struct kvm_run *run = self->run; > > + u8 skeyvalue = 0x34; > > + > > + /* copy test_skey_asm to code_hva / code_gpa */ > > + TH_LOG("copy code %p to vm mapped memory %p / %p", > > + &test_skey_asm, (void *)self->code_hva, (void *)self->code_gpa); > > + memcpy((void *)self->code_hva, &test_skey_asm, PAGE_SIZE); > > + > > + /* set register content for test_skey_asm to access not mapped memory */ > > + sync_regs->gprs[1] = skeyvalue; > > + sync_regs->gprs[5] = self->base_gpa; > > + sync_regs->gprs[6] = test_vaddr; > > + run->kvm_dirty_regs |= KVM_SYNC_GPRS; > > + > > + self->sie_block->ictl |= ICTL_OPEREXC | ICTL_PINT; > > + self->sie_block->cpuflags &= ~CPUSTAT_KSS; > > So you don't want KVM to initialize skeys? > Or am I missing a ucontrol skey interaction? > > What about the ICTLs if KSS is not available on the machine? This is explicitly disabling KSS, not enabling it. Doing that explicitly might not strictly be necessary but I thought this does provide some clarity about the state.
On 8/19/24 6:00 PM, Christoph Schlameuss wrote: > On Fri Aug 16, 2024 at 4:36 PM CEST, Janosch Frank wrote: >> On 8/15/24 5:45 PM, Christoph Schlameuss wrote: [...] >>> +TEST_F(uc_kvm, uc_skey) >>> +{ >>> + u64 test_vaddr = self->base_gpa + VM_MEM_SIZE - (SZ_1M / 2); >>> + struct kvm_sync_regs *sync_regs = &self->run->s.regs; >>> + struct kvm_run *run = self->run; >>> + u8 skeyvalue = 0x34; >>> + >>> + /* copy test_skey_asm to code_hva / code_gpa */ >>> + TH_LOG("copy code %p to vm mapped memory %p / %p", >>> + &test_skey_asm, (void *)self->code_hva, (void *)self->code_gpa); >>> + memcpy((void *)self->code_hva, &test_skey_asm, PAGE_SIZE); >>> + >>> + /* set register content for test_skey_asm to access not mapped memory */ >>> + sync_regs->gprs[1] = skeyvalue; >>> + sync_regs->gprs[5] = self->base_gpa; >>> + sync_regs->gprs[6] = test_vaddr; >>> + run->kvm_dirty_regs |= KVM_SYNC_GPRS; >>> + >>> + self->sie_block->ictl |= ICTL_OPEREXC | ICTL_PINT; >>> + self->sie_block->cpuflags &= ~CPUSTAT_KSS; >> >> So you don't want KVM to initialize skeys? >> Or am I missing a ucontrol skey interaction? >> >> What about the ICTLs if KSS is not available on the machine? > > This is explicitly disabling KSS, not enabling it. > Doing that explicitly might not strictly be necessary but I thought this does > provide some clarity about the state. > The 3 skey ICTLs and KSS are used by KVM to get an intercept on the first skey instruction that the guest issues. KVM uses that intercept to initialize the keys and setup skey handling since it's an edge case because Linux doesn't use skeys. If KSS is available KVM will not set the skey ICTLs but KSS is a "recent" addition (my guess would be ~z13). So if you want to disable skey intercepts regardless of the machine you need to clear all 4 bits. Are you sure that disabling KSS makes sense and does what you think it does?
On Fri Aug 23, 2024 at 9:59 AM CEST, Janosch Frank wrote: > On 8/19/24 6:00 PM, Christoph Schlameuss wrote: > > On Fri Aug 16, 2024 at 4:36 PM CEST, Janosch Frank wrote: > >> On 8/15/24 5:45 PM, Christoph Schlameuss wrote: > [...] > >>> +TEST_F(uc_kvm, uc_skey) > >>> +{ > >>> + u64 test_vaddr = self->base_gpa + VM_MEM_SIZE - (SZ_1M / 2); > >>> + struct kvm_sync_regs *sync_regs = &self->run->s.regs; > >>> + struct kvm_run *run = self->run; > >>> + u8 skeyvalue = 0x34; > >>> + > >>> + /* copy test_skey_asm to code_hva / code_gpa */ > >>> + TH_LOG("copy code %p to vm mapped memory %p / %p", > >>> + &test_skey_asm, (void *)self->code_hva, (void *)self->code_gpa); > >>> + memcpy((void *)self->code_hva, &test_skey_asm, PAGE_SIZE); > >>> + > >>> + /* set register content for test_skey_asm to access not mapped memory */ > >>> + sync_regs->gprs[1] = skeyvalue; > >>> + sync_regs->gprs[5] = self->base_gpa; > >>> + sync_regs->gprs[6] = test_vaddr; > >>> + run->kvm_dirty_regs |= KVM_SYNC_GPRS; > >>> + > >>> + self->sie_block->ictl |= ICTL_OPEREXC | ICTL_PINT; > >>> + self->sie_block->cpuflags &= ~CPUSTAT_KSS; > >> > >> So you don't want KVM to initialize skeys? > >> Or am I missing a ucontrol skey interaction? > >> > >> What about the ICTLs if KSS is not available on the machine? > > > > This is explicitly disabling KSS, not enabling it. > > Doing that explicitly might not strictly be necessary but I thought this does > > provide some clarity about the state. > > > > The 3 skey ICTLs and KSS are used by KVM to get an intercept on the > first skey instruction that the guest issues. KVM uses that intercept to > initialize the keys and setup skey handling since it's an edge case > because Linux doesn't use skeys. > > If KSS is available KVM will not set the skey ICTLs but KSS is a > "recent" addition (my guess would be ~z13). So if you want to disable > skey intercepts regardless of the machine you need to clear all 4 bits. > > Are you sure that disabling KSS makes sense and does what you think it does? I did revisit the normal skey initialization. It is as you say triggered by the first KSS intercept. But this is where it actually differs in uncontrol VMs. kvm_handle_sie_intercept() would normally call kvm_s390_skey_check_enable(). But in the ucontrol case it exists early and sets KVM_EXIT_S390_SIEIC with the KSS intercept code. So I think the best coverage we can produce here is to mimic that within the tests userspace code. I will restore the KSS interception and handle it in the next patch version.
diff --git a/tools/testing/selftests/kvm/s390x/ucontrol_test.c b/tools/testing/selftests/kvm/s390x/ucontrol_test.c index 41306bb52f29..5f8815a80544 100644 --- a/tools/testing/selftests/kvm/s390x/ucontrol_test.c +++ b/tools/testing/selftests/kvm/s390x/ucontrol_test.c @@ -79,6 +79,32 @@ asm("test_mem_asm:\n" " j 0b\n" ); +/* Test program manipulating storage keys */ +extern char test_skey_asm[]; +asm("test_skey_asm:\n" + "xgr %r0, %r0\n" + + "0:\n" + " ahi %r0,1\n" + " st %r1,0(%r5,%r6)\n" + + " iske %r1,%r6\n" + " ahi %r0,1\n" + " diag 0,0,0x44\n" + + " sske %r1,%r6\n" + " iske %r1,%r6\n" + " ahi %r0,1\n" + " diag 0,0,0x44\n" + + " rrbe %r1,%r6\n" + " iske %r1,%r6\n" + " ahi %r0,1\n" + " diag 0,0,0x44\n" + + " j 0b\n" +); + FIXTURE(uc_kvm) { struct kvm_s390_sie_block *sie_block; @@ -345,6 +371,56 @@ static void uc_assert_diag44(FIXTURE_DATA(uc_kvm) * self) TEST_ASSERT_EQ(0x440000, sie_block->ipb); } +TEST_F(uc_kvm, uc_skey) +{ + u64 test_vaddr = self->base_gpa + VM_MEM_SIZE - (SZ_1M / 2); + struct kvm_sync_regs *sync_regs = &self->run->s.regs; + struct kvm_run *run = self->run; + u8 skeyvalue = 0x34; + + /* copy test_skey_asm to code_hva / code_gpa */ + TH_LOG("copy code %p to vm mapped memory %p / %p", + &test_skey_asm, (void *)self->code_hva, (void *)self->code_gpa); + memcpy((void *)self->code_hva, &test_skey_asm, PAGE_SIZE); + + /* set register content for test_skey_asm to access not mapped memory */ + sync_regs->gprs[1] = skeyvalue; + sync_regs->gprs[5] = self->base_gpa; + sync_regs->gprs[6] = test_vaddr; + run->kvm_dirty_regs |= KVM_SYNC_GPRS; + + self->sie_block->ictl |= ICTL_OPEREXC | ICTL_PINT; + self->sie_block->cpuflags &= ~CPUSTAT_KSS; + /* DAT disabled + 64 bit mode */ + run->psw_mask = 0x0000000180000000ULL; + run->psw_addr = self->code_gpa; + + ASSERT_EQ(0, uc_run_once(self)); + ASSERT_EQ(false, uc_handle_exit(self)); + ASSERT_EQ(2, sync_regs->gprs[0]); + /* assert initial skey (ACC = 0, R & C = 1) */ + ASSERT_EQ(0x06, sync_regs->gprs[1]); + uc_assert_diag44(self); + + sync_regs->gprs[1] = skeyvalue; + run->kvm_dirty_regs |= KVM_SYNC_GPRS; + ASSERT_EQ(0, uc_run_once(self)); + ASSERT_EQ(false, uc_handle_exit(self)); + ASSERT_EQ(3, sync_regs->gprs[0]); + ASSERT_EQ(skeyvalue, sync_regs->gprs[1]); + uc_assert_diag44(self); + + sync_regs->gprs[1] = skeyvalue; + run->kvm_dirty_regs |= KVM_SYNC_GPRS; + ASSERT_EQ(0, uc_run_once(self)); + ASSERT_EQ(false, uc_handle_exit(self)); + ASSERT_EQ(4, sync_regs->gprs[0]); + /* assert R reset but rest of skey unchanged*/ + ASSERT_EQ(skeyvalue & 0xfa, sync_regs->gprs[1]); + ASSERT_EQ(0x00, sync_regs->gprs[1] & 0x04); + uc_assert_diag44(self); +} + TEST_F(uc_kvm, uc_map_unmap) { struct kvm_sync_regs *sync_regs = &self->run->s.regs;
Add a test case manipulating s390 storage keys from within the ucontrol VM. Signed-off-by: Christoph Schlameuss <schlameuss@linux.ibm.com> --- .../selftests/kvm/s390x/ucontrol_test.c | 76 +++++++++++++++++++ 1 file changed, 76 insertions(+)