[kvm-unit-tests PATCH 0/2] s390x: diag10: Fixup

[kvm-unit-tests PATCH 0/2] s390x: diag10: Fixup

[Janosch Frank]

While reviewing Claudio's patch set I found a problem that should've
been caught by the diag10 test but wasn't.

Not only does the test never check the good case but it also doesn't
fence against environments where diag10 is not available. The part
that makes this problematic is the fact that this test only tests priv
and spec PGMs. These PGMs are presented even if no diag10 support is
provided since they are also part of the base diagnose architecture.

The tests currently succeed in TCG emulation and PV, both of which do
not implement this specific diagnose.

Therefore this series fences TCG & PV as well as adding a check if the page has
really been cleared.


Janosch Frank (2):
  s390x: diag10: Fence tcg and pv environments
  s390x: diag10: Check page clear

 s390x/diag10.c      | 26 ++++++++++++++++++++++++++
 s390x/unittests.cfg |  1 +
 2 files changed, 27 insertions(+)

-- 
2.48.1

[kvm-unit-tests PATCH 1/2] s390x: diag10: Fence tcg and pv environments

[Janosch Frank]

Diag10 isn't supported under either of these environments so let's
make sure that the test bails out accordingly.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 s390x/diag10.c      | 15 +++++++++++++++
 s390x/unittests.cfg |  1 +
 2 files changed, 16 insertions(+)

diff --git a/s390x/diag10.c b/s390x/diag10.c
index 579a7a5d..00725f58 100644
--- a/s390x/diag10.c
+++ b/s390x/diag10.c
@@ -9,6 +9,8 @@
  */
 
 #include <libcflat.h>
+#include <uv.h>
+#include <hardware.h>
 #include <asm/asm-offsets.h>
 #include <asm/interrupt.h>
 #include <asm/page.h>
@@ -95,8 +97,21 @@ static void test_priv(void)
 int main(void)
 {
 	report_prefix_push("diag10");
+
+	if (host_is_tcg()) {
+		report_skip("Test unsupported under TCG");
+		goto out;
+	}
+	if (uv_os_is_guest()) {
+		report_skip("Test unsupported under PV");
+		goto out;
+	}
+
 	test_prefix();
 	test_params();
 	test_priv();
+
+out:
+	report_prefix_pop();
 	return report_summary();
 }
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index a9af6680..9c43ab2f 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -51,6 +51,7 @@ extra_params = -device virtio-net-ccw
 
 [diag10]
 file = diag10.elf
+accel = kvm
 
 [diag308]
 file = diag308.elf
-- 
2.48.1

[kvm-unit-tests PATCH 2/2] s390x: diag10: Check page clear

[Janosch Frank]

We should get a new page after we discarded the page.
So let's check for that.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 s390x/diag10.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/s390x/diag10.c b/s390x/diag10.c
index 00725f58..b68481ad 100644
--- a/s390x/diag10.c
+++ b/s390x/diag10.c
@@ -94,6 +94,16 @@ static void test_priv(void)
 	report_prefix_pop();
 }
 
+static void test_content(void)
+{
+	report_prefix_push("content");
+	memset((void *)page0, 0x42, PAGE_SIZE);
+	memset((void *)page1, 0, PAGE_SIZE);
+	diag10(page0, page0);
+	report(!memcmp((void *)page0, (void *)page1, PAGE_SIZE), "Page cleared");
+	report_prefix_pop();
+}
+
 int main(void)
 {
 	report_prefix_push("diag10");
@@ -110,6 +120,7 @@ int main(void)
 	test_prefix();
 	test_params();
 	test_priv();
+	test_content();
 
 out:
 	report_prefix_pop();
-- 
2.48.1

Re: [kvm-unit-tests PATCH 1/2] s390x: diag10: Fence tcg and pv environments

[Claudio Imbrenda]

On Wed, 28 May 2025 09:13:49 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> Diag10 isn't supported under either of these environments so let's
> make sure that the test bails out accordingly.

does KVM always implement diag10?

is there no other way to check whether diag10 is available?

we could, for example, try to run it "correctly" and see whether we get
a Specification exception, and then fence.

> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  s390x/diag10.c      | 15 +++++++++++++++
>  s390x/unittests.cfg |  1 +
>  2 files changed, 16 insertions(+)
> 
> diff --git a/s390x/diag10.c b/s390x/diag10.c
> index 579a7a5d..00725f58 100644
> --- a/s390x/diag10.c
> +++ b/s390x/diag10.c
> @@ -9,6 +9,8 @@
>   */
>  
>  #include <libcflat.h>
> +#include <uv.h>
> +#include <hardware.h>
>  #include <asm/asm-offsets.h>
>  #include <asm/interrupt.h>
>  #include <asm/page.h>
> @@ -95,8 +97,21 @@ static void test_priv(void)
>  int main(void)
>  {
>  	report_prefix_push("diag10");
> +
> +	if (host_is_tcg()) {
> +		report_skip("Test unsupported under TCG");
> +		goto out;
> +	}
> +	if (uv_os_is_guest()) {
> +		report_skip("Test unsupported under PV");
> +		goto out;
> +	}
> +
>  	test_prefix();
>  	test_params();
>  	test_priv();
> +
> +out:
> +	report_prefix_pop();
>  	return report_summary();
>  }
> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> index a9af6680..9c43ab2f 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -51,6 +51,7 @@ extra_params = -device virtio-net-ccw
>  
>  [diag10]
>  file = diag10.elf
> +accel = kvm
>  
>  [diag308]
>  file = diag308.elf

Re: [kvm-unit-tests PATCH 2/2] s390x: diag10: Check page clear

[Claudio Imbrenda]

On Wed, 28 May 2025 09:13:50 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> We should get a new page after we discarded the page.
> So let's check for that.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

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

> ---
>  s390x/diag10.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/s390x/diag10.c b/s390x/diag10.c
> index 00725f58..b68481ad 100644
> --- a/s390x/diag10.c
> +++ b/s390x/diag10.c
> @@ -94,6 +94,16 @@ static void test_priv(void)
>  	report_prefix_pop();
>  }
>  
> +static void test_content(void)
> +{
> +	report_prefix_push("content");
> +	memset((void *)page0, 0x42, PAGE_SIZE);
> +	memset((void *)page1, 0, PAGE_SIZE);
> +	diag10(page0, page0);
> +	report(!memcmp((void *)page0, (void *)page1, PAGE_SIZE), "Page cleared");
> +	report_prefix_pop();
> +}
> +
>  int main(void)
>  {
>  	report_prefix_push("diag10");
> @@ -110,6 +120,7 @@ int main(void)
>  	test_prefix();
>  	test_params();
>  	test_priv();
> +	test_content();
>  
>  out:
>  	report_prefix_pop();

Re: [kvm-unit-tests PATCH 1/2] s390x: diag10: Fence tcg and pv environments

[Thomas Huth]

On 28/05/2025 11.13, Janosch Frank wrote:
> Diag10 isn't supported under either of these environments so let's
> make sure that the test bails out accordingly.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   s390x/diag10.c      | 15 +++++++++++++++
>   s390x/unittests.cfg |  1 +
>   2 files changed, 16 insertions(+)
> 
> diff --git a/s390x/diag10.c b/s390x/diag10.c
> index 579a7a5d..00725f58 100644
> --- a/s390x/diag10.c
> +++ b/s390x/diag10.c
> @@ -9,6 +9,8 @@
>    */
>   
>   #include <libcflat.h>
> +#include <uv.h>
> +#include <hardware.h>
>   #include <asm/asm-offsets.h>
>   #include <asm/interrupt.h>
>   #include <asm/page.h>
> @@ -95,8 +97,21 @@ static void test_priv(void)
>   int main(void)
>   {
>   	report_prefix_push("diag10");
> +
> +	if (host_is_tcg()) {
> +		report_skip("Test unsupported under TCG");
> +		goto out;
> +	}
> +	if (uv_os_is_guest()) {
> +		report_skip("Test unsupported under PV");
> +		goto out;
> +	}
> +
>   	test_prefix();
>   	test_params();
>   	test_priv();

Would it make sense to run test_priv() at least for TCG/PV, too?

  Thomas

Re: [kvm-unit-tests PATCH 1/2] s390x: diag10: Fence tcg and pv environments

[Janosch Frank]

On 5/28/25 11:55 AM, Thomas Huth wrote:
> On 28/05/2025 11.13, Janosch Frank wrote:
>> Diag10 isn't supported under either of these environments so let's
>> make sure that the test bails out accordingly.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>    s390x/diag10.c      | 15 +++++++++++++++
>>    s390x/unittests.cfg |  1 +
>>    2 files changed, 16 insertions(+)
>>
>> diff --git a/s390x/diag10.c b/s390x/diag10.c
>> index 579a7a5d..00725f58 100644
>> --- a/s390x/diag10.c
>> +++ b/s390x/diag10.c
>> @@ -9,6 +9,8 @@
>>     */
>>    
>>    #include <libcflat.h>
>> +#include <uv.h>
>> +#include <hardware.h>
>>    #include <asm/asm-offsets.h>
>>    #include <asm/interrupt.h>
>>    #include <asm/page.h>
>> @@ -95,8 +97,21 @@ static void test_priv(void)
>>    int main(void)
>>    {
>>    	report_prefix_push("diag10");
>> +
>> +	if (host_is_tcg()) {
>> +		report_skip("Test unsupported under TCG");
>> +		goto out;
>> +	}
>> +	if (uv_os_is_guest()) {
>> +		report_skip("Test unsupported under PV");
>> +		goto out;
>> +	}
>> +
>>    	test_prefix();
>>    	test_params();
>>    	test_priv();
> 
> Would it make sense to run test_priv() at least for TCG/PV, too?

Could do but I think such tests should go into a general diag test.

The KVM code tests priv before checking the subcode and going into the 
specific handler. So if the priv test on diag44 succeeds, then it should 
succeed on any other diag even if it's not implemented.

Under PV we'd be testing HW and/or FW instead of QEMU/KVM.

Re: [kvm-unit-tests PATCH 1/2] s390x: diag10: Fence tcg and pv environments

[Janosch Frank]

On 5/28/25 11:41 AM, Claudio Imbrenda wrote:
> On Wed, 28 May 2025 09:13:49 +0000
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> Diag10 isn't supported under either of these environments so let's
>> make sure that the test bails out accordingly.
> 
> does KVM always implement diag10?

It was introduced October of 2011 and there's no way of disabling it 
that I can see.

> 
> is there no other way to check whether diag10 is available?

The z/VM CP programming services doesn't specify a feature bit or any 
other way to check as far as I can see.

> 
> we could, for example, try to run it "correctly" and see whether we get
> a Specification exception, and then fence.

We could move the content test to the top and bail out if there's a 
spec. That would allow us to handle the addition of diag10 support for 
TCG without a test change.

But:
How likely is it that we want to implement diag10 in TCG?
My guess is that adding it to PV is even less likely.
Not running them under PV and TCG gives us back some CI time although 
it's likely minuscule.

Re: [kvm-unit-tests PATCH 1/2] s390x: diag10: Fence tcg and pv environments

[Claudio Imbrenda]

On Wed, 28 May 2025 13:13:45 +0200
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 5/28/25 11:41 AM, Claudio Imbrenda wrote:
> > On Wed, 28 May 2025 09:13:49 +0000
> > Janosch Frank <frankja@linux.ibm.com> wrote:
> >   
> >> Diag10 isn't supported under either of these environments so let's
> >> make sure that the test bails out accordingly.  
> > 
> > does KVM always implement diag10?  
> 
> It was introduced October of 2011 and there's no way of disabling it 
> that I can see.
> 
> > 
> > is there no other way to check whether diag10 is available?  
> 
> The z/VM CP programming services doesn't specify a feature bit or any 
> other way to check as far as I can see.
> 
> > 
> > we could, for example, try to run it "correctly" and see whether we get
> > a Specification exception, and then fence.  
> 
> We could move the content test to the top and bail out if there's a 
> spec. That would allow us to handle the addition of diag10 support for 
> TCG without a test change.
> 
> But:
> How likely is it that we want to implement diag10 in TCG?
> My guess is that adding it to PV is even less likely.
> Not running them under PV and TCG gives us back some CI time although 
> it's likely minuscule.

fair points

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

Re: [kvm-unit-tests PATCH 1/2] s390x: diag10: Fence tcg and pv environments

[Nico Boehr]

On Wed May 28, 2025 at 11:13 AM CEST, Janosch Frank wrote:
> Diag10 isn't supported under either of these environments so let's
> make sure that the test bails out accordingly.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

Reviewed-by: Nico Boehr <nrb@linux.ibm.com>

Re: [kvm-unit-tests PATCH 2/2] s390x: diag10: Check page clear

[Nico Boehr]

On Wed May 28, 2025 at 11:13 AM CEST, Janosch Frank wrote:
> We should get a new page after we discarded the page.
> So let's check for that.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  s390x/diag10.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/s390x/diag10.c b/s390x/diag10.c
> index 00725f58..b68481ad 100644
> --- a/s390x/diag10.c
> +++ b/s390x/diag10.c
> @@ -94,6 +94,16 @@ static void test_priv(void)
>  	report_prefix_pop();
>  }
>  
> +static void test_content(void)
> +{
> +	report_prefix_push("content");
> +	memset((void *)page0, 0x42, PAGE_SIZE);
> +	memset((void *)page1, 0, PAGE_SIZE);
> +	diag10(page0, page0);
> +	report(!memcmp((void *)page0, (void *)page1, PAGE_SIZE), "Page cleared");
> +	report_prefix_pop();
> +}

Would be nice to test that we don't clear too much, but this is clearly better
than what we had before so:

Reviewed-by: Nico Boehr <nrb@linux.ibm.com>