[kvm-unit-tests PATCH v3 0/1] s390x: verify EQBS/SQBS is unavailable

[kvm-unit-tests PATCH v3 0/1] s390x: verify EQBS/SQBS is unavailable

[Nico Boehr]

v2->v3:
---
- Claudio pointed out the clobber of r1 is unneeded, so remove it.

QEMU doesn't provide EQBS/SQBS instructions, so we should check they
result in an exception.

Nico Boehr (1):
  s390x: verify EQBS/SQBS is unavailable

 s390x/intercept.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

-- 
2.36.1

[kvm-unit-tests PATCH v3 1/1] s390x: verify EQBS/SQBS is unavailable

[Nico Boehr]

QEMU doesn't provide EQBS/SQBS instructions, so we should check they
result in an exception.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
---
 s390x/intercept.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/s390x/intercept.c b/s390x/intercept.c
index 9e826b6c79ad..48eb2d22a2cc 100644
--- a/s390x/intercept.c
+++ b/s390x/intercept.c
@@ -197,6 +197,34 @@ static void test_diag318(void)
 
 }
 
+static void test_qbs(void)
+{
+	report_prefix_push("qbs");
+	if (!host_is_qemu()) {
+		report_skip("QEMU-only test");
+		report_prefix_pop();
+		return;
+	}
+
+	report_prefix_push("sqbs");
+	expect_pgm_int();
+	asm volatile(
+		"	.insn   rsy,0xeb000000008a,0,0,0(0)\n"
+		: : : "memory", "cc");
+	check_pgm_int_code(PGM_INT_CODE_OPERATION);
+	report_prefix_pop();
+
+	report_prefix_push("eqbs");
+	expect_pgm_int();
+	asm volatile(
+		"	.insn   rrf,0xb99c0000,0,0,0,0\n"
+		: : : "memory", "cc");
+	check_pgm_int_code(PGM_INT_CODE_OPERATION);
+	report_prefix_pop();
+
+	report_prefix_pop();
+}
+
 struct {
 	const char *name;
 	void (*func)(void);
@@ -208,6 +236,7 @@ struct {
 	{ "stidp", test_stidp, false },
 	{ "testblock", test_testblock, false },
 	{ "diag318", test_diag318, false },
+	{ "qbs", test_qbs, false },
 	{ NULL, NULL, false }
 };
 
-- 
2.36.1

Re: [kvm-unit-tests PATCH v3 1/1] s390x: verify EQBS/SQBS is unavailable

[Claudio Imbrenda]

On Wed,  3 Aug 2022 15:58:51 +0200
Nico Boehr <nrb@linux.ibm.com> wrote:

> QEMU doesn't provide EQBS/SQBS instructions, so we should check they
> result in an exception.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> ---
>  s390x/intercept.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/s390x/intercept.c b/s390x/intercept.c
> index 9e826b6c79ad..48eb2d22a2cc 100644
> --- a/s390x/intercept.c
> +++ b/s390x/intercept.c
> @@ -197,6 +197,34 @@ static void test_diag318(void)
>  
>  }
>  
> +static void test_qbs(void)
> +{
> +	report_prefix_push("qbs");
> +	if (!host_is_qemu()) {
> +		report_skip("QEMU-only test");
> +		report_prefix_pop();
> +		return;
> +	}
> +
> +	report_prefix_push("sqbs");
> +	expect_pgm_int();
> +	asm volatile(
> +		"	.insn   rsy,0xeb000000008a,0,0,0(0)\n"
> +		: : : "memory", "cc");
> +	check_pgm_int_code(PGM_INT_CODE_OPERATION);
> +	report_prefix_pop();
> +
> +	report_prefix_push("eqbs");
> +	expect_pgm_int();
> +	asm volatile(
> +		"	.insn   rrf,0xb99c0000,0,0,0,0\n"
> +		: : : "memory", "cc");
> +	check_pgm_int_code(PGM_INT_CODE_OPERATION);
> +	report_prefix_pop();
> +
> +	report_prefix_pop();
> +}
> +
>  struct {
>  	const char *name;
>  	void (*func)(void);
> @@ -208,6 +236,7 @@ struct {
>  	{ "stidp", test_stidp, false },
>  	{ "testblock", test_testblock, false },
>  	{ "diag318", test_diag318, false },
> +	{ "qbs", test_qbs, false },
>  	{ NULL, NULL, false }
>  };
>  

Re: [kvm-unit-tests PATCH v3 1/1] s390x: verify EQBS/SQBS is unavailable

[Thomas Huth]

On 03/08/2022 15.58, Nico Boehr wrote:
> QEMU doesn't provide EQBS/SQBS instructions, so we should check they
> result in an exception.

I somewhat fail to see the exact purpose of this patch... QEMU still doesn't 
emulate a lot of other instructions, too, so why are we checking now these 
QBS instructions? Why not all the others? Why do we need a test to verify 
that there is an exception in this case - was there a bug somewhere that 
didn't cause an exception in certain circumstances?

> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
>   s390x/intercept.c | 29 +++++++++++++++++++++++++++++
>   1 file changed, 29 insertions(+)
> 
> diff --git a/s390x/intercept.c b/s390x/intercept.c
> index 9e826b6c79ad..48eb2d22a2cc 100644
> --- a/s390x/intercept.c
> +++ b/s390x/intercept.c
> @@ -197,6 +197,34 @@ static void test_diag318(void)
>   
>   }
>   
> +static void test_qbs(void)
> +{
> +	report_prefix_push("qbs");

You should definitely add a comment here, explaining why this is only a test 
for QEMU and saying that this could be removed as soon as QEMU implements 
these instructions later - otherwise this would be very confusing to the 
readers later (if they forget or cannot check the commit message).

> +	if (!host_is_qemu()) {
> +		report_skip("QEMU-only test");
> +		report_prefix_pop();
> +		return;
> +	}
> +
> +	report_prefix_push("sqbs");
> +	expect_pgm_int();
> +	asm volatile(
> +		"	.insn   rsy,0xeb000000008a,0,0,0(0)\n"
> +		: : : "memory", "cc");
> +	check_pgm_int_code(PGM_INT_CODE_OPERATION);
> +	report_prefix_pop();
> +
> +	report_prefix_push("eqbs");
> +	expect_pgm_int();
> +	asm volatile(
> +		"	.insn   rrf,0xb99c0000,0,0,0,0\n"
> +		: : : "memory", "cc");
> +	check_pgm_int_code(PGM_INT_CODE_OPERATION);
> +	report_prefix_pop();
> +
> +	report_prefix_pop();
> +}

  Thomas

Re: [kvm-unit-tests PATCH v3 1/1] s390x: verify EQBS/SQBS is unavailable

[Nico Boehr]

Quoting Thomas Huth (2022-08-04 00:17:38)
> On 03/08/2022 15.58, Nico Boehr wrote:
> > QEMU doesn't provide EQBS/SQBS instructions, so we should check they
> > result in an exception.
> 
> I somewhat fail to see the exact purpose of this patch... QEMU still doesn't 
> emulate a lot of other instructions, too, so why are we checking now these 
> QBS instructions? 

I agree with you, it certainly doesn't make sense to test all kinds of random instructions that aren't implemented in QEMU.

But, for the QBS instructions, there is a special case in handle_b9 and handle_eb in QEMU. I would argue since there is code for it, there can be (or even should be) tests for it.

But if you guys say it is not worth having this test, this is fine for me as well.

[...]
> > diff --git a/s390x/intercept.c b/s390x/intercept.c
> > index 9e826b6c79ad..48eb2d22a2cc 100644
> > --- a/s390x/intercept.c
> > +++ b/s390x/intercept.c
> > @@ -197,6 +197,34 @@ static void test_diag318(void)
> >   
> >   }
> >   
> > +static void test_qbs(void)
> > +{
> > +     report_prefix_push("qbs");
> 
> You should definitely add a comment here, explaining why this is only a test 
> for QEMU and saying that this could be removed as soon as QEMU implements 
> these instructions later - otherwise this would be very confusing to the 
> readers later (if they forget or cannot check the commit message).

OK, I can add this once we have an answer to your first question.

Re: [kvm-unit-tests PATCH v3 1/1] s390x: verify EQBS/SQBS is unavailable

[Janosch Frank]

On 8/4/22 00:17, Thomas Huth wrote:
> On 03/08/2022 15.58, Nico Boehr wrote:
>> QEMU doesn't provide EQBS/SQBS instructions, so we should check they
>> result in an exception.
> 
> I somewhat fail to see the exact purpose of this patch... QEMU still doesn't
> emulate a lot of other instructions, too, so why are we checking now these
> QBS instructions? Why not all the others? Why do we need a test to verify
> that there is an exception in this case - was there a bug somewhere that
> didn't cause an exception in certain circumstances?

Looking at the patch that introduced the QEMU handlers (1eecf41b) I 
wonder why those two cases were added. From my point of view it makes 
sense to remove the special handling for those two instructions.

@Christian: Any idea why this was added? Can we remove it?

The only reason I can think of to test this is the existence of EC* bits 
that control the behavior for those instructions. So if we set those 
without having QEMU handling code then we're in trouble.

But then I'd also expect that we need to set a stfle bit to indicate the 
availability and this test doesn't check for that and would indicate a 
false-positive.

Re: [kvm-unit-tests PATCH v3 1/1] s390x: verify EQBS/SQBS is unavailable

[Thomas Huth]

On 24/08/2022 09.40, Janosch Frank wrote:
> On 8/4/22 00:17, Thomas Huth wrote:
>> On 03/08/2022 15.58, Nico Boehr wrote:
>>> QEMU doesn't provide EQBS/SQBS instructions, so we should check they
>>> result in an exception.
>>
>> I somewhat fail to see the exact purpose of this patch... QEMU still doesn't
>> emulate a lot of other instructions, too, so why are we checking now these
>> QBS instructions? Why not all the others? Why do we need a test to verify
>> that there is an exception in this case - was there a bug somewhere that
>> didn't cause an exception in certain circumstances?
> 
> Looking at the patch that introduced the QEMU handlers (1eecf41b) I wonder 
> why those two cases were added. From my point of view it makes sense to 
> remove the special handling for those two instructions.

Ah, there are handlers for these instructions in QEMU - that's what I was 
missing. But I agree with you, these do not look very useful and should just 
be dropped on the QEMU side.

  Thomas

Re: [kvm-unit-tests PATCH v3 1/1] s390x: verify EQBS/SQBS is unavailable

[Christian Borntraeger]

Am 24.08.22 um 09:40 schrieb Janosch Frank:
> On 8/4/22 00:17, Thomas Huth wrote:
>> On 03/08/2022 15.58, Nico Boehr wrote:
>>> QEMU doesn't provide EQBS/SQBS instructions, so we should check they
>>> result in an exception.
>>
>> I somewhat fail to see the exact purpose of this patch... QEMU still doesn't
>> emulate a lot of other instructions, too, so why are we checking now these
>> QBS instructions? Why not all the others? Why do we need a test to verify
>> that there is an exception in this case - was there a bug somewhere that
>> didn't cause an exception in certain circumstances?
> 
> Looking at the patch that introduced the QEMU handlers (1eecf41b) I wonder why those two cases were added. From my point of view it makes sense to remove the special handling for those two instructions.
> 
> @Christian: Any idea why this was added? Can we remove it?

No idea why it was added (in an always fail way). Yes we could remove it until we need it.