diff mbox series

[kvm-unit-tests,v3,1/3] s390x: Test TEID values in storage key test

Message ID 20220523132406.1820550-2-scgl@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series More skey instr. emulation test | expand

Commit Message

Janis Schoetterl-Glausch May 23, 2022, 1:24 p.m. UTC 
On a protection exception, test that the Translation-Exception
Identification (TEID) values are correct given the circumstances of the
particular test.
The meaning of the TEID values is dependent on the installed
suppression-on-protection facility.

Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
---
 s390x/skey.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 69 insertions(+), 6 deletions(-)

Comments

Claudio Imbrenda May 24, 2022, 3:09 p.m. UTC | #1 
On Mon, 23 May 2022 15:24:04 +0200
Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:

> On a protection exception, test that the Translation-Exception
> Identification (TEID) values are correct given the circumstances of the
> particular test.
> The meaning of the TEID values is dependent on the installed
> suppression-on-protection facility.
> 
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> ---
>  s390x/skey.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 69 insertions(+), 6 deletions(-)
> 
> diff --git a/s390x/skey.c b/s390x/skey.c
> index 42bf598c..5e234cde 100644
> --- a/s390x/skey.c
> +++ b/s390x/skey.c
> @@ -8,6 +8,7 @@
>   *  Janosch Frank <frankja@linux.vnet.ibm.com>
>   */
>  #include <libcflat.h>
> +#include <asm/arch_def.h>
>  #include <asm/asm-offsets.h>
>  #include <asm/interrupt.h>
>  #include <vmalloc.h>
> @@ -158,6 +159,68 @@ static void test_test_protection(void)
>  	report_prefix_pop();
>  }
>  
> +enum access {
> +	ACC_STORE = 1,
> +	ACC_FETCH = 2,
> +	ACC_UPDATE = 3,
> +};
> +
> +enum protection {
> +	PROT_STORE = 1,
> +	PROT_FETCH_STORE = 3,
> +};
> +
> +static void check_key_prot_exc(enum access access, enum protection prot)
> +{
> +	union teid teid;
> +	int access_code;
> +
> +	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
> +	report_prefix_push("TEID");
> +	teid.val = lowcore.trans_exc_id;
> +	switch (get_supp_on_prot_facility()) {
> +	case SOP_NONE:
> +	case SOP_BASIC:
> +		break;

for basic you should check for sop_teid_predictable and sop_acc_list

> +	case SOP_ENHANCED_1:
> +		report(!teid.esop1_acc_list_or_dat, "valid protection code");

actually, both values of esop1_acc_list_or_dat are wrong, since we're
expecting neither an access list nor a dat exception.

you need to check for esop1_teid_predictable instead (which you need to
add, see comment in that patchseries)

> +		break;
> +	case SOP_ENHANCED_2:
> +		switch (teid_esop2_prot_code(teid)) {
> +		case PROT_KEY:
> +			access_code = teid.acc_exc_f_s;

is the f/s feature guaranteed to be present when we have esop2?

can the f/s feature be present with esop1 or basic sop?

> +
> +			switch (access_code) {
> +			case 0:
> +				report_pass("valid access code");
> +				break;
> +			case 1:
> +			case 2:
> +				report((access & access_code) && (prot & access_code),
> +				       "valid access code");
> +				break;
> +			case 3:
> +				/*
> +				 * This is incorrect in that reserved values
> +				 * should be ignored, but kvm should not return
> +				 * a reserved value and having a test for that
> +				 * is more valuable.
> +				 */
> +				report_fail("valid access code");
> +				break;
> +			}
> +			/* fallthrough */
> +		case PROT_KEY_LAP:
> +			report_pass("valid protection code");
> +			break;
> +		default:
> +			report_fail("valid protection code");
> +		}
> +		break;
> +	}
> +	report_prefix_pop();
> +}
> +
>  /*
>   * Perform STORE CPU ADDRESS (STAP) instruction while temporarily executing
>   * with access key 1.
> @@ -199,7 +262,7 @@ static void test_store_cpu_address(void)
>  	expect_pgm_int();
>  	*out = 0xbeef;
>  	store_cpu_address_key_1(out);
> -	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
> +	check_key_prot_exc(ACC_STORE, PROT_STORE);
>  	report(*out == 0xbeef, "no store occurred");
>  	report_prefix_pop();
>  
> @@ -210,7 +273,7 @@ static void test_store_cpu_address(void)
>  	expect_pgm_int();
>  	*out = 0xbeef;
>  	store_cpu_address_key_1(out);
> -	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
> +	check_key_prot_exc(ACC_STORE, PROT_STORE);
>  	report(*out == 0xbeef, "no store occurred");
>  	report_prefix_pop();
>  
> @@ -228,7 +291,7 @@ static void test_store_cpu_address(void)
>  	expect_pgm_int();
>  	*out = 0xbeef;
>  	store_cpu_address_key_1(out);
> -	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
> +	check_key_prot_exc(ACC_STORE, PROT_STORE);
>  	report(*out == 0xbeef, "no store occurred");
>  	report_prefix_pop();
>  
> @@ -314,7 +377,7 @@ static void test_set_prefix(void)
>  	set_storage_key(pagebuf, 0x28, 0);
>  	expect_pgm_int();
>  	set_prefix_key_1(prefix_ptr);
> -	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
> +	check_key_prot_exc(ACC_FETCH, PROT_FETCH_STORE);
>  	report(get_prefix() == old_prefix, "did not set prefix");
>  	report_prefix_pop();
>  
> @@ -327,7 +390,7 @@ static void test_set_prefix(void)
>  	install_page(root, virt_to_pte_phys(root, pagebuf), 0);
>  	set_prefix_key_1((uint32_t *)0);
>  	install_page(root, 0, 0);
> -	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
> +	check_key_prot_exc(ACC_FETCH, PROT_FETCH_STORE);
>  	report(get_prefix() == old_prefix, "did not set prefix");
>  	report_prefix_pop();
>  
> @@ -351,7 +414,7 @@ static void test_set_prefix(void)
>  	install_page(root, virt_to_pte_phys(root, pagebuf), 0);
>  	set_prefix_key_1((uint32_t *)&mem_all[2048]);
>  	install_page(root, 0, 0);
> -	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
> +	check_key_prot_exc(ACC_FETCH, PROT_FETCH_STORE);
>  	report(get_prefix() == old_prefix, "did not set prefix");
>  	report_prefix_pop();
>
Janis Schoetterl-Glausch June 8, 2022, 5:03 p.m. UTC | #2 
On Tue, 2022-05-24 at 17:09 +0200, Claudio Imbrenda wrote:
> On Mon, 23 May 2022 15:24:04 +0200
> Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:
> 
> > On a protection exception, test that the Translation-Exception
> > Identification (TEID) values are correct given the circumstances of the
> > particular test.
> > The meaning of the TEID values is dependent on the installed
> > suppression-on-protection facility.
> > 
> > Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> > ---
> >  s390x/skey.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 69 insertions(+), 6 deletions(-)
> > 
> > diff --git a/s390x/skey.c b/s390x/skey.c
> > index 42bf598c..5e234cde 100644
> > --- a/s390x/skey.c
> > +++ b/s390x/skey.c
> > @@ -8,6 +8,7 @@

[...]

> > +		break;
> > +	case SOP_ENHANCED_2:
> > +		switch (teid_esop2_prot_code(teid)) {
> > +		case PROT_KEY:
> > +			access_code = teid.acc_exc_f_s;
> 
> is the f/s feature guaranteed to be present when we have esop2?

That's how I understand it. For esop1 the PoP explicitly states that
the facility is a prerequisite, for esop2 it doesn't.
> 
> can the f/s feature be present with esop1 or basic sop?

esop1: yes, basic: no.
The way I read it, in the case of esop1 the bits are only meaningful
for DAT and access list exceptions, i.e. when the TEID is not
unpredictable.
> 
> > +
> > +			switch (access_code) {
> > +			case 0:
> > +				report_pass("valid access code");
> > +				break;
> > +			case 1:
> > +			case 2:
> > +				report((access & access_code) && (prot & access_code),
> > +				       "valid access code");
> > +				break;
> > +			case 3:
> > +				/*
> > +				 * This is incorrect in that reserved values
> > +				 * should be ignored, but kvm should not return
> > +				 * a reserved value and having a test for that
> > +				 * is more valuable.
> > +				 */
> > +				report_fail("valid access code");
> > +				break;
> > +			}
> > +			/* fallthrough */
> > +		case PROT_KEY_LAP:
> > +			report_pass("valid protection code");
> > +			break;
> > +		default:
> > +			report_fail("valid protection code");
> > +		}
> > +		break;
> > +	}
> > +	report_prefix_pop();
> > +}
> > +

[...]
Claudio Imbrenda June 9, 2022, 7:44 a.m. UTC | #3 
On Wed, 08 Jun 2022 19:03:23 +0200
Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:

[...]

> > > +		break;
> > > +	case SOP_ENHANCED_2:
> > > +		switch (teid_esop2_prot_code(teid)) {
> > > +		case PROT_KEY:
> > > +			access_code = teid.acc_exc_f_s;  
> > 
> > is the f/s feature guaranteed to be present when we have esop2?  
> 
> That's how I understand it. For esop1 the PoP explicitly states that
> the facility is a prerequisite, for esop2 it doesn't.
> > 
> > can the f/s feature be present with esop1 or basic sop?  
> 
> esop1: yes, basic: no.
> The way I read it, in the case of esop1 the bits are only meaningful
> for DAT and access list exceptions, i.e. when the TEID is not
> unpredictable.

I see, makes sense

maybe add a comment :)

> >   
> > > +
> > > +			switch (access_code) {
> > > +			case 0:
> > > +				report_pass("valid access code");
> > > +				break;
> > > +			case 1:
> > > +			case 2:
> > > +				report((access & access_code) && (prot & access_code),
> > > +				       "valid access code");
> > > +				break;
> > > +			case 3:
> > > +				/*
> > > +				 * This is incorrect in that reserved values
> > > +				 * should be ignored, but kvm should not return
> > > +				 * a reserved value and having a test for that
> > > +				 * is more valuable.
> > > +				 */
> > > +				report_fail("valid access code");
> > > +				break;
> > > +			}
> > > +			/* fallthrough */
> > > +		case PROT_KEY_LAP:
> > > +			report_pass("valid protection code");
> > > +			break;
> > > +		default:
> > > +			report_fail("valid protection code");
> > > +		}
> > > +		break;
> > > +	}
> > > +	report_prefix_pop();
> > > +}
> > > +  
> 
> [...]
diff mbox series

Patch

diff --git a/s390x/skey.c b/s390x/skey.c
index 42bf598c..5e234cde 100644
--- a/s390x/skey.c
+++ b/s390x/skey.c
@@ -8,6 +8,7 @@ 
  *  Janosch Frank <frankja@linux.vnet.ibm.com>
  */
 #include <libcflat.h>
+#include <asm/arch_def.h>
 #include <asm/asm-offsets.h>
 #include <asm/interrupt.h>
 #include <vmalloc.h>
@@ -158,6 +159,68 @@  static void test_test_protection(void)
 	report_prefix_pop();
 }
 
+enum access {
+	ACC_STORE = 1,
+	ACC_FETCH = 2,
+	ACC_UPDATE = 3,
+};
+
+enum protection {
+	PROT_STORE = 1,
+	PROT_FETCH_STORE = 3,
+};
+
+static void check_key_prot_exc(enum access access, enum protection prot)
+{
+	union teid teid;
+	int access_code;
+
+	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
+	report_prefix_push("TEID");
+	teid.val = lowcore.trans_exc_id;
+	switch (get_supp_on_prot_facility()) {
+	case SOP_NONE:
+	case SOP_BASIC:
+		break;
+	case SOP_ENHANCED_1:
+		report(!teid.esop1_acc_list_or_dat, "valid protection code");
+		break;
+	case SOP_ENHANCED_2:
+		switch (teid_esop2_prot_code(teid)) {
+		case PROT_KEY:
+			access_code = teid.acc_exc_f_s;
+
+			switch (access_code) {
+			case 0:
+				report_pass("valid access code");
+				break;
+			case 1:
+			case 2:
+				report((access & access_code) && (prot & access_code),
+				       "valid access code");
+				break;
+			case 3:
+				/*
+				 * This is incorrect in that reserved values
+				 * should be ignored, but kvm should not return
+				 * a reserved value and having a test for that
+				 * is more valuable.
+				 */
+				report_fail("valid access code");
+				break;
+			}
+			/* fallthrough */
+		case PROT_KEY_LAP:
+			report_pass("valid protection code");
+			break;
+		default:
+			report_fail("valid protection code");
+		}
+		break;
+	}
+	report_prefix_pop();
+}
+
 /*
  * Perform STORE CPU ADDRESS (STAP) instruction while temporarily executing
  * with access key 1.
@@ -199,7 +262,7 @@  static void test_store_cpu_address(void)
 	expect_pgm_int();
 	*out = 0xbeef;
 	store_cpu_address_key_1(out);
-	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
+	check_key_prot_exc(ACC_STORE, PROT_STORE);
 	report(*out == 0xbeef, "no store occurred");
 	report_prefix_pop();
 
@@ -210,7 +273,7 @@  static void test_store_cpu_address(void)
 	expect_pgm_int();
 	*out = 0xbeef;
 	store_cpu_address_key_1(out);
-	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
+	check_key_prot_exc(ACC_STORE, PROT_STORE);
 	report(*out == 0xbeef, "no store occurred");
 	report_prefix_pop();
 
@@ -228,7 +291,7 @@  static void test_store_cpu_address(void)
 	expect_pgm_int();
 	*out = 0xbeef;
 	store_cpu_address_key_1(out);
-	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
+	check_key_prot_exc(ACC_STORE, PROT_STORE);
 	report(*out == 0xbeef, "no store occurred");
 	report_prefix_pop();
 
@@ -314,7 +377,7 @@  static void test_set_prefix(void)
 	set_storage_key(pagebuf, 0x28, 0);
 	expect_pgm_int();
 	set_prefix_key_1(prefix_ptr);
-	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
+	check_key_prot_exc(ACC_FETCH, PROT_FETCH_STORE);
 	report(get_prefix() == old_prefix, "did not set prefix");
 	report_prefix_pop();
 
@@ -327,7 +390,7 @@  static void test_set_prefix(void)
 	install_page(root, virt_to_pte_phys(root, pagebuf), 0);
 	set_prefix_key_1((uint32_t *)0);
 	install_page(root, 0, 0);
-	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
+	check_key_prot_exc(ACC_FETCH, PROT_FETCH_STORE);
 	report(get_prefix() == old_prefix, "did not set prefix");
 	report_prefix_pop();
 
@@ -351,7 +414,7 @@  static void test_set_prefix(void)
 	install_page(root, virt_to_pte_phys(root, pagebuf), 0);
 	set_prefix_key_1((uint32_t *)&mem_all[2048]);
 	install_page(root, 0, 0);
-	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
+	check_key_prot_exc(ACC_FETCH, PROT_FETCH_STORE);
 	report(get_prefix() == old_prefix, "did not set prefix");
 	report_prefix_pop();