diff mbox series

[2/3] selftests: kvm: s390: Add uc_skey VM test case

Message ID 20240815154529.628087-3-schlameuss@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series selftests: kvm: s390: Add ucontrol memory selftests | expand

Commit Message

Christoph Schlameuss Aug. 15, 2024, 3:45 p.m. UTC
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(+)

Comments

Janosch Frank Aug. 16, 2024, 2:36 p.m. UTC | #1
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?
Christoph Schlameuss Aug. 19, 2024, 4 p.m. UTC | #2
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.
Janosch Frank Aug. 23, 2024, 7:59 a.m. UTC | #3
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?
Christoph Schlameuss Aug. 30, 2024, 3:51 p.m. UTC | #4
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 mbox series

Patch

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;