[kvm-unit-tests PATCH v3 0/3] s390x: Add misaligned instruction tests

[kvm-unit-tests PATCH v3 0/3] s390x: Add misaligned instruction tests

[Nina Schoetterl-Glausch]

Instructions on s390 must be halfword aligned.
Add two tests for that.
These currently fail when using TCG.

v3 -> v2:
 * pick up R-b (thanks Janosch)
 * use br instead of bcr (thanks Claudio)
 * use text section instead of rodata for ex target (thanks Claudio)
 * fix label position (thanks Claudio)

v1 -> v2:
 * rebase
 * use PSW macros
 * simplify odd psw test (thanks Claudio)
 * rename some identifiers
 * pick up R-b (thanks Claudio)

Nina Schoetterl-Glausch (3):
  s390x/spec_ex: Use PSW macro
  s390x/spec_ex: Add test introducing odd address into PSW
  s390x/spec_ex: Add test of EXECUTE with odd target address

 s390x/spec_ex.c | 85 +++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 76 insertions(+), 9 deletions(-)

Range-diff against v2:
1:  d82f4fb6 ! 1:  6ae1eb7d s390x/spec_ex: Use PSW macro
    @@ Commit message
         Replace explicit psw definition by PSW macro.
         No functional change intended.
     
    +    Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
         Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
     
      ## s390x/spec_ex.c ##
2:  e537797f ! 2:  a0d02438 s390x/spec_ex: Add test introducing odd address into PSW
    @@ s390x/spec_ex.c: static int psw_bit_12_is_1(void)
     +"	. = . + 1\n"
     +"misaligned_code:\n"
     +"	larl	%r0,0\n"
    -+"	bcr	0xf,%r1\n"
    ++"	br	%r1\n"
     +);
     +
     +static int psw_odd_address(void)
3:  dc552880 ! 3:  e771deeb s390x/spec_ex: Add test of EXECUTE with odd target address
    @@ s390x/spec_ex.c: static int short_psw_bit_12_is_0(void)
     +	uint64_t pre_target_addr;
     +	int to = 0, from = 0x0dd;
     +
    -+	asm volatile ( ".pushsection .rodata\n"
    -+		"pre_odd_ex_target:\n"
    ++	asm volatile ( ".pushsection .text.ex_odd\n"
     +		"	.balign	2\n"
    ++		"pre_odd_ex_target:\n"
     +		"	. = . + 1\n"
     +		"	lr	%[to],%[from]\n"
     +		"	.popsection\n"

base-commit: e3c5c3ef2524c58023073c0fadde2e8ae3c04ec6
-- 
2.39.1

[kvm-unit-tests PATCH v3 1/3] s390x/spec_ex: Use PSW macro

[Nina Schoetterl-Glausch]

Replace explicit psw definition by PSW macro.
No functional change intended.

Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
---
 s390x/spec_ex.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/s390x/spec_ex.c b/s390x/spec_ex.c
index 42ecaed3..2adc5996 100644
--- a/s390x/spec_ex.c
+++ b/s390x/spec_ex.c
@@ -105,10 +105,7 @@ static int check_invalid_psw(void)
 /* For normal PSWs bit 12 has to be 0 to be a valid PSW*/
 static int psw_bit_12_is_1(void)
 {
-	struct psw invalid = {
-		.mask = BIT(63 - 12),
-		.addr = 0x00000000deadbeee
-	};
+	struct psw invalid = PSW(BIT(63 - 12), 0x00000000deadbeee);
 
 	expect_invalid_psw(invalid);
 	load_psw(invalid);
@@ -118,10 +115,7 @@ static int psw_bit_12_is_1(void)
 /* A short PSW needs to have bit 12 set to be valid. */
 static int short_psw_bit_12_is_0(void)
 {
-	struct psw invalid = {
-		.mask = BIT(63 - 12),
-		.addr = 0x00000000deadbeee
-	};
+	struct psw invalid = PSW(BIT(63 - 12), 0x00000000deadbeee);
 	struct short_psw short_invalid = {
 		.mask = 0x0,
 		.addr = 0xdeadbeee
-- 
2.39.1

[kvm-unit-tests PATCH v3 2/3] s390x/spec_ex: Add test introducing odd address into PSW

[Nina Schoetterl-Glausch]

Instructions on s390 must be halfword aligned.
Introducing an odd instruction address into the PSW leads to a
specification exception when attempting to execute the instruction at
the odd address.
Add a test for this.

Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
---
 s390x/spec_ex.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 1 deletion(-)

diff --git a/s390x/spec_ex.c b/s390x/spec_ex.c
index 2adc5996..83b8c58e 100644
--- a/s390x/spec_ex.c
+++ b/s390x/spec_ex.c
@@ -88,12 +88,23 @@ static void expect_invalid_psw(struct psw psw)
 	invalid_psw_expected = true;
 }
 
+static void clear_invalid_psw(void)
+{
+	expected_psw = PSW(0, 0);
+	invalid_psw_expected = false;
+}
+
 static int check_invalid_psw(void)
 {
 	/* Since the fixup sets this to false we check for false here. */
 	if (!invalid_psw_expected) {
+		/*
+		 * Early exception recognition: pgm_int_id == 0.
+		 * Late exception recognition: psw address has been
+		 *	incremented by pgm_int_id (unpredictable value)
+		 */
 		if (expected_psw.mask == invalid_psw.mask &&
-		    expected_psw.addr == invalid_psw.addr)
+		    expected_psw.addr == invalid_psw.addr - lowcore.pgm_int_id)
 			return 0;
 		report_fail("Wrong invalid PSW");
 	} else {
@@ -112,6 +123,42 @@ static int psw_bit_12_is_1(void)
 	return check_invalid_psw();
 }
 
+extern char misaligned_code[];
+asm (  ".balign	2\n"
+"	. = . + 1\n"
+"misaligned_code:\n"
+"	larl	%r0,0\n"
+"	br	%r1\n"
+);
+
+static int psw_odd_address(void)
+{
+	struct psw odd = PSW_WITH_CUR_MASK((uint64_t)&misaligned_code);
+	uint64_t executed_addr;
+
+	expect_invalid_psw(odd);
+	fixup_psw.mask = extract_psw_mask();
+	asm volatile ( "xr	%%r0,%%r0\n"
+		"	larl	%%r1,0f\n"
+		"	stg	%%r1,%[fixup_addr]\n"
+		"	lpswe	%[odd_psw]\n"
+		"0:	lr	%[executed_addr],%%r0\n"
+	: [fixup_addr] "=&T" (fixup_psw.addr),
+	  [executed_addr] "=d" (executed_addr)
+	: [odd_psw] "Q" (odd)
+	: "cc", "%r0", "%r1"
+	);
+
+	if (!executed_addr) {
+		return check_invalid_psw();
+	} else {
+		assert(executed_addr == odd.addr);
+		clear_invalid_psw();
+		report_fail("did not execute unaligned instructions");
+		return 1;
+	}
+}
+
 /* A short PSW needs to have bit 12 set to be valid. */
 static int short_psw_bit_12_is_0(void)
 {
@@ -170,6 +217,7 @@ struct spec_ex_trigger {
 static const struct spec_ex_trigger spec_ex_triggers[] = {
 	{ "psw_bit_12_is_1", &psw_bit_12_is_1, false, &fixup_invalid_psw },
 	{ "short_psw_bit_12_is_0", &short_psw_bit_12_is_0, false, &fixup_invalid_psw },
+	{ "psw_odd_address", &psw_odd_address, false, &fixup_invalid_psw },
 	{ "bad_alignment", &bad_alignment, true, NULL },
 	{ "not_even", &not_even, true, NULL },
 	{ NULL, NULL, false, NULL },
-- 
2.39.1

[kvm-unit-tests PATCH v3 3/3] s390x/spec_ex: Add test of EXECUTE with odd target address

[Nina Schoetterl-Glausch]

The EXECUTE instruction executes the instruction at the given target
address. This address must be halfword aligned, otherwise a
specification exception occurs.
Add a test for this.

Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
---
 s390x/spec_ex.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/s390x/spec_ex.c b/s390x/spec_ex.c
index 83b8c58e..5fa05dba 100644
--- a/s390x/spec_ex.c
+++ b/s390x/spec_ex.c
@@ -177,6 +177,30 @@ static int short_psw_bit_12_is_0(void)
 	return 0;
 }
 
+static int odd_ex_target(void)
+{
+	uint64_t pre_target_addr;
+	int to = 0, from = 0x0dd;
+
+	asm volatile ( ".pushsection .text.ex_odd\n"
+		"	.balign	2\n"
+		"pre_odd_ex_target:\n"
+		"	. = . + 1\n"
+		"	lr	%[to],%[from]\n"
+		"	.popsection\n"
+
+		"	larl	%[pre_target_addr],pre_odd_ex_target\n"
+		"	ex	0,1(%[pre_target_addr])\n"
+		: [pre_target_addr] "=&a" (pre_target_addr),
+		  [to] "+d" (to)
+		: [from] "d" (from)
+	);
+
+	assert((pre_target_addr + 1) & 1);
+	report(to != from, "did not perform ex with odd target");
+	return 0;
+}
+
 static int bad_alignment(void)
 {
 	uint32_t words[5] __attribute__((aligned(16)));
@@ -218,6 +242,7 @@ static const struct spec_ex_trigger spec_ex_triggers[] = {
 	{ "psw_bit_12_is_1", &psw_bit_12_is_1, false, &fixup_invalid_psw },
 	{ "short_psw_bit_12_is_0", &short_psw_bit_12_is_0, false, &fixup_invalid_psw },
 	{ "psw_odd_address", &psw_odd_address, false, &fixup_invalid_psw },
+	{ "odd_ex_target", &odd_ex_target, true, NULL },
 	{ "bad_alignment", &bad_alignment, true, NULL },
 	{ "not_even", &not_even, true, NULL },
 	{ NULL, NULL, false, NULL },
-- 
2.39.1

Re: [kvm-unit-tests PATCH v3 2/3] s390x/spec_ex: Add test introducing odd address into PSW

[Janosch Frank]

On 3/15/23 16:54, Nina Schoetterl-Glausch wrote:
> Instructions on s390 must be halfword aligned.
> Introducing an odd instruction address into the PSW leads to a
> specification exception when attempting to execute the instruction at
> the odd address.
> Add a test for this.
> 
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>

Acked-by: Janosch Frank <frankja@linux.ibm.com>

Some nits below.

> ---
>   s390x/spec_ex.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 49 insertions(+), 1 deletion(-)
> 
> diff --git a/s390x/spec_ex.c b/s390x/spec_ex.c
> index 2adc5996..83b8c58e 100644
> --- a/s390x/spec_ex.c
> +++ b/s390x/spec_ex.c
> @@ -88,12 +88,23 @@ static void expect_invalid_psw(struct psw psw)
>   	invalid_psw_expected = true;
>   }
>   
> +static void clear_invalid_psw(void)
> +{
> +	expected_psw = PSW(0, 0);
> +	invalid_psw_expected = false;
> +}
> +
>   static int check_invalid_psw(void)
>   {
>   	/* Since the fixup sets this to false we check for false here. */
>   	if (!invalid_psw_expected) {
> +		/*
> +		 * Early exception recognition: pgm_int_id == 0.
> +		 * Late exception recognition: psw address has been
> +		 *	incremented by pgm_int_id (unpredictable value)
> +		 */
>   		if (expected_psw.mask == invalid_psw.mask &&
> -		    expected_psw.addr == invalid_psw.addr)
> +		    expected_psw.addr == invalid_psw.addr - lowcore.pgm_int_id)
>   			return 0;
>   		report_fail("Wrong invalid PSW");
>   	} else {
> @@ -112,6 +123,42 @@ static int psw_bit_12_is_1(void)
>   	return check_invalid_psw();
>   }
>   
> +extern char misaligned_code[];
> +asm (  ".balign	2\n"

Is the double space intended?
Looking at the file itself some asm blocks have no space before the "(" 
and some have one.

> +"	. = . + 1\n"
> +"misaligned_code:\n"
> +"	larl	%r0,0\n"
> +"	br	%r1\n"
> +);

Any reason this is not indented?

> +
> +static int psw_odd_address(void)
> +{
> +	struct psw odd = PSW_WITH_CUR_MASK((uint64_t)&misaligned_code);
> +	uint64_t executed_addr;
> +
> +	expect_invalid_psw(odd);
> +	fixup_psw.mask = extract_psw_mask();
> +	asm volatile ( "xr	%%r0,%%r0\n"

While it will likely never make a difference I'd still use xgr here 
instead of xr.

> +		"	larl	%%r1,0f\n"
> +		"	stg	%%r1,%[fixup_addr]\n"
> +		"	lpswe	%[odd_psw]\n"
> +		"0:	lr	%[executed_addr],%%r0\n"
> +	: [fixup_addr] "=&T" (fixup_psw.addr),
> +	  [executed_addr] "=d" (executed_addr)
> +	: [odd_psw] "Q" (odd)
> +	: "cc", "%r0", "%r1"
> +	);
> +
> +	if (!executed_addr) {
> +		return check_invalid_psw();
> +	} else {
> +		assert(executed_addr == odd.addr);
> +		clear_invalid_psw();
> +		report_fail("did not execute unaligned instructions");
> +		return 1;
> +	}
> +}
> +
>   /* A short PSW needs to have bit 12 set to be valid. */
>   static int short_psw_bit_12_is_0(void)
>   {
> @@ -170,6 +217,7 @@ struct spec_ex_trigger {
>   static const struct spec_ex_trigger spec_ex_triggers[] = {
>   	{ "psw_bit_12_is_1", &psw_bit_12_is_1, false, &fixup_invalid_psw },
>   	{ "short_psw_bit_12_is_0", &short_psw_bit_12_is_0, false, &fixup_invalid_psw },
> +	{ "psw_odd_address", &psw_odd_address, false, &fixup_invalid_psw },
>   	{ "bad_alignment", &bad_alignment, true, NULL },
>   	{ "not_even", &not_even, true, NULL },
>   	{ NULL, NULL, false, NULL },

Re: [kvm-unit-tests PATCH v3 3/3] s390x/spec_ex: Add test of EXECUTE with odd target address

[Janosch Frank]

On 3/15/23 16:54, Nina Schoetterl-Glausch wrote:
> The EXECUTE instruction executes the instruction at the given target
> address. This address must be halfword aligned, otherwise a
> specification exception occurs.
> Add a test for this.
> 
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>

Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

> ---
>   s390x/spec_ex.c | 25 +++++++++++++++++++++++++
>   1 file changed, 25 insertions(+)
> 
> diff --git a/s390x/spec_ex.c b/s390x/spec_ex.c
> index 83b8c58e..5fa05dba 100644
> --- a/s390x/spec_ex.c
> +++ b/s390x/spec_ex.c
> @@ -177,6 +177,30 @@ static int short_psw_bit_12_is_0(void)
>   	return 0;
>   }
>   
> +static int odd_ex_target(void)
> +{
> +	uint64_t pre_target_addr;
> +	int to = 0, from = 0x0dd;
> +
> +	asm volatile ( ".pushsection .text.ex_odd\n"
> +		"	.balign	2\n"
> +		"pre_odd_ex_target:\n"
> +		"	. = . + 1\n"
> +		"	lr	%[to],%[from]\n"
> +		"	.popsection\n"
> +
> +		"	larl	%[pre_target_addr],pre_odd_ex_target\n"
> +		"	ex	0,1(%[pre_target_addr])\n"
> +		: [pre_target_addr] "=&a" (pre_target_addr),
> +		  [to] "+d" (to)
> +		: [from] "d" (from)
> +	);
> +
> +	assert((pre_target_addr + 1) & 1);
> +	report(to != from, "did not perform ex with odd target");
> +	return 0;
> +}
> +
>   static int bad_alignment(void)
>   {
>   	uint32_t words[5] __attribute__((aligned(16)));
> @@ -218,6 +242,7 @@ static const struct spec_ex_trigger spec_ex_triggers[] = {
>   	{ "psw_bit_12_is_1", &psw_bit_12_is_1, false, &fixup_invalid_psw },
>   	{ "short_psw_bit_12_is_0", &short_psw_bit_12_is_0, false, &fixup_invalid_psw },
>   	{ "psw_odd_address", &psw_odd_address, false, &fixup_invalid_psw },
> +	{ "odd_ex_target", &odd_ex_target, true, NULL },
>   	{ "bad_alignment", &bad_alignment, true, NULL },
>   	{ "not_even", &not_even, true, NULL },
>   	{ NULL, NULL, false, NULL },

Re: [kvm-unit-tests PATCH v3 2/3] s390x/spec_ex: Add test introducing odd address into PSW

[Nina Schoetterl-Glausch]

On Fri, 2023-03-17 at 10:26 +0100, Janosch Frank wrote:
> On 3/15/23 16:54, Nina Schoetterl-Glausch wrote:
> > Instructions on s390 must be halfword aligned.
> > Introducing an odd instruction address into the PSW leads to a
> > specification exception when attempting to execute the instruction at
> > the odd address.
> > Add a test for this.
> > 
> > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> 
> Acked-by: Janosch Frank <frankja@linux.ibm.com>
> 
> Some nits below.
> 
> > ---
> >   s390x/spec_ex.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 49 insertions(+), 1 deletion(-)
> > 
> > diff --git a/s390x/spec_ex.c b/s390x/spec_ex.c
> > index 2adc5996..83b8c58e 100644
> > --- a/s390x/spec_ex.c
> > +++ b/s390x/spec_ex.c
> > @@ -88,12 +88,23 @@ static void expect_invalid_psw(struct psw psw)
> >   	invalid_psw_expected = true;
> >   }
> >   
> > +static void clear_invalid_psw(void)
> > +{
> > +	expected_psw = PSW(0, 0);
> > +	invalid_psw_expected = false;
> > +}
> > +
> >   static int check_invalid_psw(void)
> >   {
> >   	/* Since the fixup sets this to false we check for false here. */
> >   	if (!invalid_psw_expected) {
> > +		/*
> > +		 * Early exception recognition: pgm_int_id == 0.
> > +		 * Late exception recognition: psw address has been
> > +		 *	incremented by pgm_int_id (unpredictable value)
> > +		 */
> >   		if (expected_psw.mask == invalid_psw.mask &&
> > -		    expected_psw.addr == invalid_psw.addr)
> > +		    expected_psw.addr == invalid_psw.addr - lowcore.pgm_int_id)
> >   			return 0;
> >   		report_fail("Wrong invalid PSW");
> >   	} else {
> > @@ -112,6 +123,42 @@ static int psw_bit_12_is_1(void)
> >   	return check_invalid_psw();
> >   }
> >   
> > +extern char misaligned_code[];
> > +asm (  ".balign	2\n"
> 
> Is the double space intended?

Yes, so stuff lines up.
> Looking at the file itself some asm blocks have no space before the "(" 
> and some have one.

In spec_ex.c? Where?

> 
> > +"	. = . + 1\n"
> > +"misaligned_code:\n"
> > +"	larl	%r0,0\n"
> > +"	br	%r1\n"
> > +);
> 
> Any reason this is not indented?

You mean the whole asm block, so it looks more like a function body to the misaligned_code symbol?
I'm indifferent about it, can do that if you think it's nicer.

> 
> > +
> > +static int psw_odd_address(void)
> > +{
> > +	struct psw odd = PSW_WITH_CUR_MASK((uint64_t)&misaligned_code);
> > +	uint64_t executed_addr;
> > +
> > +	expect_invalid_psw(odd);
> > +	fixup_psw.mask = extract_psw_mask();
> > +	asm volatile ( "xr	%%r0,%%r0\n"
> 
> While it will likely never make a difference I'd still use xgr here 
> instead of xr.

Yes, needs xgr.
> 
> > +		"	larl	%%r1,0f\n"
> > +		"	stg	%%r1,%[fixup_addr]\n"
> > +		"	lpswe	%[odd_psw]\n"
> > +		"0:	lr	%[executed_addr],%%r0\n"
> > +	: [fixup_addr] "=&T" (fixup_psw.addr),
> > +	  [executed_addr] "=d" (executed_addr)
> > +	: [odd_psw] "Q" (odd)
> > +	: "cc", "%r0", "%r1"
> > +	);
> > +
> > +	if (!executed_addr) {
> > +		return check_invalid_psw();
> > +	} else {
> > +		assert(executed_addr == odd.addr);
> > +		clear_invalid_psw();
> > +		report_fail("did not execute unaligned instructions");
> > +		return 1;
> > +	}
> > +}
> > +
> >   /* A short PSW needs to have bit 12 set to be valid. */
> >   static int short_psw_bit_12_is_0(void)
> >   {
> > @@ -170,6 +217,7 @@ struct spec_ex_trigger {
> >   static const struct spec_ex_trigger spec_ex_triggers[] = {
> >   	{ "psw_bit_12_is_1", &psw_bit_12_is_1, false, &fixup_invalid_psw },
> >   	{ "short_psw_bit_12_is_0", &short_psw_bit_12_is_0, false, &fixup_invalid_psw },
> > +	{ "psw_odd_address", &psw_odd_address, false, &fixup_invalid_psw },
> >   	{ "bad_alignment", &bad_alignment, true, NULL },
> >   	{ "not_even", &not_even, true, NULL },
> >   	{ NULL, NULL, false, NULL },
> 

Re: [kvm-unit-tests PATCH v3 2/3] s390x/spec_ex: Add test introducing odd address into PSW

[Janosch Frank]

On 3/17/23 11:51, Nina Schoetterl-Glausch wrote:
> On Fri, 2023-03-17 at 10:26 +0100, Janosch Frank wrote:
>> On 3/15/23 16:54, Nina Schoetterl-Glausch wrote:
>>> Instructions on s390 must be halfword aligned.
>>> Introducing an odd instruction address into the PSW leads to a
>>> specification exception when attempting to execute the instruction at
>>> the odd address.
>>> Add a test for this.
>>>
>>> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
>>
>> Acked-by: Janosch Frank <frankja@linux.ibm.com>
>>
>> Some nits below.
>>
>>> ---
>>>    s390x/spec_ex.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++-
>>>    1 file changed, 49 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/s390x/spec_ex.c b/s390x/spec_ex.c
>>> index 2adc5996..83b8c58e 100644
>>> --- a/s390x/spec_ex.c
>>> +++ b/s390x/spec_ex.c
>>> @@ -88,12 +88,23 @@ static void expect_invalid_psw(struct psw psw)
>>>    	invalid_psw_expected = true;
>>>    }
>>>    
>>> +static void clear_invalid_psw(void)
>>> +{
>>> +	expected_psw = PSW(0, 0);
>>> +	invalid_psw_expected = false;
>>> +}
>>> +
>>>    static int check_invalid_psw(void)
>>>    {
>>>    	/* Since the fixup sets this to false we check for false here. */
>>>    	if (!invalid_psw_expected) {
>>> +		/*
>>> +		 * Early exception recognition: pgm_int_id == 0.
>>> +		 * Late exception recognition: psw address has been
>>> +		 *	incremented by pgm_int_id (unpredictable value)
>>> +		 */
>>>    		if (expected_psw.mask == invalid_psw.mask &&
>>> -		    expected_psw.addr == invalid_psw.addr)
>>> +		    expected_psw.addr == invalid_psw.addr - lowcore.pgm_int_id)
>>>    			return 0;
>>>    		report_fail("Wrong invalid PSW");
>>>    	} else {
>>> @@ -112,6 +123,42 @@ static int psw_bit_12_is_1(void)
>>>    	return check_invalid_psw();
>>>    }
>>>    
>>> +extern char misaligned_code[];
>>> +asm (  ".balign	2\n"
>>
>> Is the double space intended?
> 
> Yes, so stuff lines up.
ahhh, right.

>> Looking at the file itself some asm blocks have no space before the "("
>> and some have one.
> 
> In spec_ex.c? Where?

Should have said: "after the (" but seems like the point doesn't matter 
anyway just fixup the xgr.

> 
>>
>>> +"	. = . + 1\n"
>>> +"misaligned_code:\n"
>>> +"	larl	%r0,0\n"
>>> +"	br	%r1\n"
>>> +);
>>
>> Any reason this is not indented?
> 
> You mean the whole asm block, so it looks more like a function body to the misaligned_code symbol?
> I'm indifferent about it, can do that if you think it's nicer.
> 
>>
>>> +
>>> +static int psw_odd_address(void)
>>> +{
>>> +	struct psw odd = PSW_WITH_CUR_MASK((uint64_t)&misaligned_code);
>>> +	uint64_t executed_addr;
>>> +
>>> +	expect_invalid_psw(odd);
>>> +	fixup_psw.mask = extract_psw_mask();
>>> +	asm volatile ( "xr	%%r0,%%r0\n"
>>
>> While it will likely never make a difference I'd still use xgr here
>> instead of xr.
> 
> Yes, needs xgr.
>>
>>> +		"	larl	%%r1,0f\n"
>>> +		"	stg	%%r1,%[fixup_addr]\n"
>>> +		"	lpswe	%[odd_psw]\n"
>>> +		"0:	lr	%[executed_addr],%%r0\n"
>>> +	: [fixup_addr] "=&T" (fixup_psw.addr),
>>> +	  [executed_addr] "=d" (executed_addr)
>>> +	: [odd_psw] "Q" (odd)
>>> +	: "cc", "%r0", "%r1"
>>> +	);
>>> +
>>> +	if (!executed_addr) {
>>> +		return check_invalid_psw();
>>> +	} else {
>>> +		assert(executed_addr == odd.addr);
>>> +		clear_invalid_psw();
>>> +		report_fail("did not execute unaligned instructions");
>>> +		return 1;
>>> +	}
>>> +}
>>> +
>>>    /* A short PSW needs to have bit 12 set to be valid. */
>>>    static int short_psw_bit_12_is_0(void)
>>>    {
>>> @@ -170,6 +217,7 @@ struct spec_ex_trigger {
>>>    static const struct spec_ex_trigger spec_ex_triggers[] = {
>>>    	{ "psw_bit_12_is_1", &psw_bit_12_is_1, false, &fixup_invalid_psw },
>>>    	{ "short_psw_bit_12_is_0", &short_psw_bit_12_is_0, false, &fixup_invalid_psw },
>>> +	{ "psw_odd_address", &psw_odd_address, false, &fixup_invalid_psw },
>>>    	{ "bad_alignment", &bad_alignment, true, NULL },
>>>    	{ "not_even", &not_even, true, NULL },
>>>    	{ NULL, NULL, false, NULL },
>>
> 

Re: [kvm-unit-tests PATCH v3 3/3] s390x/spec_ex: Add test of EXECUTE with odd target address

[Thomas Huth]

On 15/03/2023 16.54, Nina Schoetterl-Glausch wrote:
> The EXECUTE instruction executes the instruction at the given target
> address. This address must be halfword aligned, otherwise a
> specification exception occurs.
> Add a test for this.
> 
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> ---
>   s390x/spec_ex.c | 25 +++++++++++++++++++++++++
>   1 file changed, 25 insertions(+)
> 
> diff --git a/s390x/spec_ex.c b/s390x/spec_ex.c
> index 83b8c58e..5fa05dba 100644
> --- a/s390x/spec_ex.c
> +++ b/s390x/spec_ex.c
> @@ -177,6 +177,30 @@ static int short_psw_bit_12_is_0(void)
>   	return 0;
>   }
>   
> +static int odd_ex_target(void)
> +{
> +	uint64_t pre_target_addr;
> +	int to = 0, from = 0x0dd;
> +
> +	asm volatile ( ".pushsection .text.ex_odd\n"
> +		"	.balign	2\n"
> +		"pre_odd_ex_target:\n"
> +		"	. = . + 1\n"
> +		"	lr	%[to],%[from]\n"
> +		"	.popsection\n"
> +
> +		"	larl	%[pre_target_addr],pre_odd_ex_target\n"
> +		"	ex	0,1(%[pre_target_addr])\n"
> +		: [pre_target_addr] "=&a" (pre_target_addr),
> +		  [to] "+d" (to)
> +		: [from] "d" (from)
> +	);
> +
> +	assert((pre_target_addr + 1) & 1);
> +	report(to != from, "did not perform ex with odd target");
> +	return 0;
> +}

Can this be triggered with KVM, or is this just a test for TCG?
In the latter case, Ilya also added a test for this to QEMU's TCG test suite:

  https://lists.gnu.org/archive/html/qemu-devel/2023-03/msg04872.html

... so if this is only about TCG, it should already be covered there.

  Thomas

Re: [kvm-unit-tests PATCH v3 3/3] s390x/spec_ex: Add test of EXECUTE with odd target address

[Thomas Huth]

On 17/03/2023 15.09, Thomas Huth wrote:
> On 15/03/2023 16.54, Nina Schoetterl-Glausch wrote:
>> The EXECUTE instruction executes the instruction at the given target
>> address. This address must be halfword aligned, otherwise a
>> specification exception occurs.
>> Add a test for this.
>>
>> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
>> ---
>>   s390x/spec_ex.c | 25 +++++++++++++++++++++++++
>>   1 file changed, 25 insertions(+)
>>
>> diff --git a/s390x/spec_ex.c b/s390x/spec_ex.c
>> index 83b8c58e..5fa05dba 100644
>> --- a/s390x/spec_ex.c
>> +++ b/s390x/spec_ex.c
>> @@ -177,6 +177,30 @@ static int short_psw_bit_12_is_0(void)
>>       return 0;
>>   }
>> +static int odd_ex_target(void)
>> +{
>> +    uint64_t pre_target_addr;
>> +    int to = 0, from = 0x0dd;
>> +
>> +    asm volatile ( ".pushsection .text.ex_odd\n"
>> +        "    .balign    2\n"
>> +        "pre_odd_ex_target:\n"
>> +        "    . = . + 1\n"
>> +        "    lr    %[to],%[from]\n"
>> +        "    .popsection\n"
>> +
>> +        "    larl    %[pre_target_addr],pre_odd_ex_target\n"
>> +        "    ex    0,1(%[pre_target_addr])\n"
>> +        : [pre_target_addr] "=&a" (pre_target_addr),
>> +          [to] "+d" (to)
>> +        : [from] "d" (from)
>> +    );
>> +
>> +    assert((pre_target_addr + 1) & 1);
>> +    report(to != from, "did not perform ex with odd target");
>> +    return 0;
>> +}
> 
> Can this be triggered with KVM, or is this just a test for TCG?

With "triggered" I mean: Can this cause an interception in KVM?

  Thomas

Re: [kvm-unit-tests PATCH v3 3/3] s390x/spec_ex: Add test of EXECUTE with odd target address

[Claudio Imbrenda]

On Fri, 17 Mar 2023 15:11:35 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 17/03/2023 15.09, Thomas Huth wrote:
> > On 15/03/2023 16.54, Nina Schoetterl-Glausch wrote:  
> >> The EXECUTE instruction executes the instruction at the given target
> >> address. This address must be halfword aligned, otherwise a
> >> specification exception occurs.
> >> Add a test for this.
> >>
> >> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> >> ---
> >>   s390x/spec_ex.c | 25 +++++++++++++++++++++++++
> >>   1 file changed, 25 insertions(+)
> >>
> >> diff --git a/s390x/spec_ex.c b/s390x/spec_ex.c
> >> index 83b8c58e..5fa05dba 100644
> >> --- a/s390x/spec_ex.c
> >> +++ b/s390x/spec_ex.c
> >> @@ -177,6 +177,30 @@ static int short_psw_bit_12_is_0(void)
> >>       return 0;
> >>   }
> >> +static int odd_ex_target(void)
> >> +{
> >> +    uint64_t pre_target_addr;
> >> +    int to = 0, from = 0x0dd;
> >> +
> >> +    asm volatile ( ".pushsection .text.ex_odd\n"
> >> +        "    .balign    2\n"
> >> +        "pre_odd_ex_target:\n"
> >> +        "    . = . + 1\n"
> >> +        "    lr    %[to],%[from]\n"
> >> +        "    .popsection\n"
> >> +
> >> +        "    larl    %[pre_target_addr],pre_odd_ex_target\n"
> >> +        "    ex    0,1(%[pre_target_addr])\n"
> >> +        : [pre_target_addr] "=&a" (pre_target_addr),
> >> +          [to] "+d" (to)
> >> +        : [from] "d" (from)
> >> +    );
> >> +
> >> +    assert((pre_target_addr + 1) & 1);
> >> +    report(to != from, "did not perform ex with odd target");
> >> +    return 0;
> >> +}  
> > 
> > Can this be triggered with KVM, or is this just a test for TCG?  
> 
> With "triggered" I mean: Can this cause an interception in KVM?

AFAIK no, but KVM and TCG are not the only things we might want to test.

we are aware of the TCG tests, and we would like to also keep the KVM
unit tests.

> 
>   Thomas
> 

Re: [kvm-unit-tests PATCH v3 3/3] s390x/spec_ex: Add test of EXECUTE with odd target address

[Thomas Huth]

On 17/03/2023 16.36, Claudio Imbrenda wrote:
> On Fri, 17 Mar 2023 15:11:35 +0100
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 17/03/2023 15.09, Thomas Huth wrote:
>>> On 15/03/2023 16.54, Nina Schoetterl-Glausch wrote:
>>>> The EXECUTE instruction executes the instruction at the given target
>>>> address. This address must be halfword aligned, otherwise a
>>>> specification exception occurs.
>>>> Add a test for this.
>>>>
>>>> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
>>>> ---
>>>>    s390x/spec_ex.c | 25 +++++++++++++++++++++++++
>>>>    1 file changed, 25 insertions(+)
>>>>
>>>> diff --git a/s390x/spec_ex.c b/s390x/spec_ex.c
>>>> index 83b8c58e..5fa05dba 100644
>>>> --- a/s390x/spec_ex.c
>>>> +++ b/s390x/spec_ex.c
>>>> @@ -177,6 +177,30 @@ static int short_psw_bit_12_is_0(void)
>>>>        return 0;
>>>>    }
>>>> +static int odd_ex_target(void)
>>>> +{
>>>> +    uint64_t pre_target_addr;
>>>> +    int to = 0, from = 0x0dd;
>>>> +
>>>> +    asm volatile ( ".pushsection .text.ex_odd\n"
>>>> +        "    .balign    2\n"
>>>> +        "pre_odd_ex_target:\n"
>>>> +        "    . = . + 1\n"
>>>> +        "    lr    %[to],%[from]\n"
>>>> +        "    .popsection\n"
>>>> +
>>>> +        "    larl    %[pre_target_addr],pre_odd_ex_target\n"
>>>> +        "    ex    0,1(%[pre_target_addr])\n"
>>>> +        : [pre_target_addr] "=&a" (pre_target_addr),
>>>> +          [to] "+d" (to)
>>>> +        : [from] "d" (from)
>>>> +    );
>>>> +
>>>> +    assert((pre_target_addr + 1) & 1);
>>>> +    report(to != from, "did not perform ex with odd target");
>>>> +    return 0;
>>>> +}
>>>
>>> Can this be triggered with KVM, or is this just a test for TCG?
>>
>> With "triggered" I mean: Can this cause an interception in KVM?
> 
> AFAIK no, but KVM and TCG are not the only things we might want to test.

Ok, fair, KVM unit tests are not for KVM only anymore since quite a while, 
so if this is helpful elsewhere, I'm fine with this.

Acked-by: Thomas Huth <thuth@redhat.com>

Re: [kvm-unit-tests PATCH v3 3/3] s390x/spec_ex: Add test of EXECUTE with odd target address

[Janosch Frank]

On 3/17/23 17:37, Thomas Huth wrote:
> On 17/03/2023 16.36, Claudio Imbrenda wrote:
>> On Fri, 17 Mar 2023 15:11:35 +0100
>> Thomas Huth <thuth@redhat.com> wrote:
>>
>>> On 17/03/2023 15.09, Thomas Huth wrote:
>>>> On 15/03/2023 16.54, Nina Schoetterl-Glausch wrote:
>>>>> The EXECUTE instruction executes the instruction at the given target
>>>>> address. This address must be halfword aligned, otherwise a
>>>>> specification exception occurs.
>>>>> Add a test for this.
>>>>>
>>>>> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
>>>>> ---
>>>>>     s390x/spec_ex.c | 25 +++++++++++++++++++++++++
>>>>>     1 file changed, 25 insertions(+)
>>>>>
>>>>> diff --git a/s390x/spec_ex.c b/s390x/spec_ex.c
>>>>> index 83b8c58e..5fa05dba 100644
>>>>> --- a/s390x/spec_ex.c
>>>>> +++ b/s390x/spec_ex.c
>>>>> @@ -177,6 +177,30 @@ static int short_psw_bit_12_is_0(void)
>>>>>         return 0;
>>>>>     }
>>>>> +static int odd_ex_target(void)
>>>>> +{
>>>>> +    uint64_t pre_target_addr;
>>>>> +    int to = 0, from = 0x0dd;
>>>>> +
>>>>> +    asm volatile ( ".pushsection .text.ex_odd\n"
>>>>> +        "    .balign    2\n"
>>>>> +        "pre_odd_ex_target:\n"
>>>>> +        "    . = . + 1\n"
>>>>> +        "    lr    %[to],%[from]\n"
>>>>> +        "    .popsection\n"
>>>>> +
>>>>> +        "    larl    %[pre_target_addr],pre_odd_ex_target\n"
>>>>> +        "    ex    0,1(%[pre_target_addr])\n"
>>>>> +        : [pre_target_addr] "=&a" (pre_target_addr),
>>>>> +          [to] "+d" (to)
>>>>> +        : [from] "d" (from)
>>>>> +    );
>>>>> +
>>>>> +    assert((pre_target_addr + 1) & 1);
>>>>> +    report(to != from, "did not perform ex with odd target");
>>>>> +    return 0;
>>>>> +}
>>>>
>>>> Can this be triggered with KVM, or is this just a test for TCG?
>>>
>>> With "triggered" I mean: Can this cause an interception in KVM?
>>
>> AFAIK no, but KVM and TCG are not the only things we might want to test.
> 
> Ok, fair, KVM unit tests are not for KVM only anymore since quite a while,
> so if this is helpful elsewhere, I'm fine with this.
> 
> Acked-by: Thomas Huth <thuth@redhat.com>

Yes, we might be due for a rename on s390x. On multiple occasions I had 
to tell people that even if the name starts with KVM we're fully capable 
of testing LPAR and zVM :-)

I also want this in for completeness, the line count is certainly of no 
concern here.