[kvm-unit-tests,v4,2/4] s390x: stsi: Define vm_is_kvm to be used in different tests

Commit Message

Pierre Morel Feb. 8, 2022, 1:27 p.m. UTC

We need in several tests to check if the VM we are running in
is KVM.
Let's add the test.

To check the VM type we use the STSI 3.2.2 instruction, let's
define it's response structure in a central header.

Signed-off-by: Pierre Morel <[e-mail]>
---
 lib/s390x/stsi.h | 32 +++++++++++++++++++++++++++
 lib/s390x/vm.c   | 56 ++++++++++++++++++++++++++++++++++++++++++++++--
 lib/s390x/vm.h   |  3 +++
 s390x/stsi.c     | 23 ++------------------
 4 files changed, 91 insertions(+), 23 deletions(-)
 create mode 100644 lib/s390x/stsi.h

Comments

Janosch Frank Feb. 8, 2022, 3:31 p.m. UTC | #1

On 2/8/22 14:27, Pierre Morel wrote:
> We need in several tests to check if the VM we are running in
> is KVM.
> Let's add the test.

Several tests are in need of a way to check on which hypervisor and 
virtualization level they are running on to be able to fence certain 
tests. This patch adds functions that return true if a vm is running 
under KVM, LPAR or generally as a level 2 guest.

> 
> To check the VM type we use the STSI 3.2.2 instruction, let's

To check if we're running under KVM we...

> define it's response structure in a central header.

Since we already have a stsi test that defines the 3.2.2 structure let's 
move the struct from the test into a new library header.

> 
> Signed-off-by: Pierre Morel <[e-mail]>
> ---
>   lib/s390x/stsi.h | 32 +++++++++++++++++++++++++++
>   lib/s390x/vm.c   | 56 ++++++++++++++++++++++++++++++++++++++++++++++--
>   lib/s390x/vm.h   |  3 +++
>   s390x/stsi.c     | 23 ++------------------
>   4 files changed, 91 insertions(+), 23 deletions(-)
>   create mode 100644 lib/s390x/stsi.h
> 
> diff --git a/lib/s390x/stsi.h b/lib/s390x/stsi.h
> new file mode 100644
> index 00000000..9b40664f
> --- /dev/null
> +++ b/lib/s390x/stsi.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Structures used to Store System Information
> + *
> + * Copyright IBM Corp. 2022
> + */
> +
> +#ifndef _S390X_STSI_H_
> +#define _S390X_STSI_H_
> +
> +struct sysinfo_3_2_2 {

Any particular reason why you renamed this?

> +	uint8_t reserved[31];
> +	uint8_t count;
> +	struct {
> +		uint8_t reserved2[4];
> +		uint16_t total_cpus;
> +		uint16_t conf_cpus;
> +		uint16_t standby_cpus;
> +		uint16_t reserved_cpus;
> +		uint8_t name[8];
> +		uint32_t caf;
> +		uint8_t cpi[16];
> +		uint8_t reserved5[3];
> +		uint8_t ext_name_encoding;
> +		uint32_t reserved3;
> +		uint8_t uuid[16];
> +	} vm[8];
> +	uint8_t reserved4[1504];
> +	uint8_t ext_names[8][256];
> +};
> +
> +#endif  /* _S390X_STSI_H_ */
> diff --git a/lib/s390x/vm.c b/lib/s390x/vm.c
> index a5b92863..38886b76 100644
> --- a/lib/s390x/vm.c
> +++ b/lib/s390x/vm.c
> @@ -12,6 +12,7 @@
>   #include <alloc_page.h>
>   #include <asm/arch_def.h>
>   #include "vm.h"
> +#include "stsi.h"
>   
>   /**
>    * Detect whether we are running with TCG (instead of KVM)
> @@ -26,9 +27,13 @@ bool vm_is_tcg(void)
>   	if (initialized)
>   		return is_tcg;
>   
> -	buf = alloc_page();
> -	if (!buf)
> +	if (!vm_is_vm()) {
> +		initialized = true;
>   		return false;
> +	}
> +
> +	buf = alloc_page();
> +	assert(buf);
>   
>   	if (stsi(buf, 1, 1, 1))
>   		goto out;
> @@ -43,3 +48,50 @@ out:
>   	free_page(buf);
>   	return is_tcg;
>   }
> +
> +/**
> + * Detect whether we are running with KVM
> + */
> +
> +bool vm_is_kvm(void)
> +{
> +	/* EBCDIC for "KVM/" */
> +	const uint8_t kvm_ebcdic[] = { 0xd2, 0xe5, 0xd4, 0x61 };
> +	static bool initialized;
> +	static bool is_kvm;
> +	struct sysinfo_3_2_2 *stsi_322;
> +
> +	if (initialized)
> +		return is_kvm;
> +
> +	if (!vm_is_vm() || vm_is_tcg()) {
> +		initialized = true;
> +		return is_kvm;
> +	}
> +
> +	stsi_322 = alloc_page();
> +	assert(stsi_322);
> +
> +	if (stsi(stsi_322, 3, 2, 2))
> +		goto out;
> +
> +	/*
> +	 * If the manufacturer string is "KVM/" in EBCDIC, then we
> +	 * are on KVM.
> +	 */
> +	is_kvm = !memcmp(&stsi_322->vm[0].cpi, kvm_ebcdic, sizeof(kvm_ebcdic));
> +	initialized = true;
> +out:
> +	free_page(stsi_322);
> +	return is_kvm;
> +}
> +
> +bool vm_is_lpar(void)
> +{
> +	return stsi_get_fc() == 2;
> +}
> +
> +bool vm_is_vm(void)
> +{
> +	return stsi_get_fc() == 3;

This would be true when running under z/VM, no?

I.e. what you're testing here is that we're a level 2 guest and hence 
the naming could be improved.

Also: what if we're under VSIE where we would be > 3?

> +}
> diff --git a/lib/s390x/vm.h b/lib/s390x/vm.h
> index 7abba0cc..3aaf76af 100644
> --- a/lib/s390x/vm.h
> +++ b/lib/s390x/vm.h
> @@ -9,5 +9,8 @@
>   #define _S390X_VM_H_
>   
>   bool vm_is_tcg(void);
> +bool vm_is_kvm(void);
> +bool vm_is_vm(void);
> +bool vm_is_lpar(void);
>   
>   #endif  /* _S390X_VM_H_ */
> diff --git a/s390x/stsi.c b/s390x/stsi.c
> index 391f8849..1ed045e2 100644
> --- a/s390x/stsi.c
> +++ b/s390x/stsi.c
> @@ -13,27 +13,8 @@
>   #include <asm/asm-offsets.h>
>   #include <asm/interrupt.h>
>   #include <smp.h>
> +#include "stsi.h"

#include <stsi.h>

We're not in the lib.

>   
> -struct stsi_322 {
> -	uint8_t reserved[31];
> -	uint8_t count;
> -	struct {
> -		uint8_t reserved2[4];
> -		uint16_t total_cpus;
> -		uint16_t conf_cpus;
> -		uint16_t standby_cpus;
> -		uint16_t reserved_cpus;
> -		uint8_t name[8];
> -		uint32_t caf;
> -		uint8_t cpi[16];
> -		uint8_t reserved5[3];
> -		uint8_t ext_name_encoding;
> -		uint32_t reserved3;
> -		uint8_t uuid[16];
> -	} vm[8];
> -	uint8_t reserved4[1504];
> -	uint8_t ext_names[8][256];
> -};
>   static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2)));
>   
>   static void test_specs(void)
> @@ -91,7 +72,7 @@ static void test_3_2_2(void)
>   	/* EBCDIC for "KVM/" */
>   	const uint8_t cpi_kvm[] = { 0xd2, 0xe5, 0xd4, 0x61 };
>   	const char vm_name_ext[] = "kvm-unit-test";
> -	struct stsi_322 *data = (void *)pagebuf;
> +	struct sysinfo_3_2_2 *data = (void *)pagebuf;
>   
>   	report_prefix_push("3.2.2");
>

Nico Boehr Feb. 8, 2022, 3:35 p.m. UTC | #2

On Tue, 2022-02-08 at 14:27 +0100, Pierre Morel wrote:
> We need in several tests to check if the VM we are running in
> is KVM.
> Let's add the test.
> 
> To check the VM type we use the STSI 3.2.2 instruction, let's
> define it's response structure in a central header.
> 
> Signed-off-by: Pierre Morel <[e-mail]>

> ---
>  lib/s390x/stsi.h | 32 +++++++++++++++++++++++++++
>  lib/s390x/vm.c   | 56
> ++++++++++++++++++++++++++++++++++++++++++++++--
>  lib/s390x/vm.h   |  3 +++
>  s390x/stsi.c     | 23 ++------------------
>  4 files changed, 91 insertions(+), 23 deletions(-)
>  create mode 100644 lib/s390x/stsi.h
> 
> diff --git a/lib/s390x/stsi.h b/lib/s390x/stsi.h
> new file mode 100644
> index 00000000..9b40664f
> --- /dev/null
> +++ b/lib/s390x/stsi.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */

This was taken from stsi.c which is GPL 2 only, so this probably should
be as well.

> diff --git a/lib/s390x/vm.c b/lib/s390x/vm.c
> index a5b92863..38886b76 100644
> --- a/lib/s390x/vm.c
> +++ b/lib/s390x/vm.c
> @@ -12,6 +12,7 @@
>  #include <alloc_page.h>
>  #include <asm/arch_def.h>
>  #include "vm.h"
> +#include "stsi.h"
>  
>  /**
>   * Detect whether we are running with TCG (instead of KVM)
> @@ -26,9 +27,13 @@ bool vm_is_tcg(void)
>         if (initialized)
>                 return is_tcg;
>  
> -       buf = alloc_page();
> -       if (!buf)
> +       if (!vm_is_vm()) {
> +               initialized = true;
>                 return false;
> diff --git a/lib/s390x/vm.c b/lib/s390x/vm.c
> index a5b92863..38886b76 100644
> --- a/lib/s390x/vm.c
> +++ b/lib/s390x/vm.c
> @@ -12,6 +12,7 @@
>  #include <alloc_page.h>
>  #include <asm/arch_def.h>
>  #include "vm.h"
> +#include "stsi.h"
>  
>  /**
>   * Detect whether we are running with TCG (instead of KVM)
> @@ -26,9 +27,13 @@ bool vm_is_tcg(void)
>         if (initialized)
>                 return is_tcg;
>  
> -       buf = alloc_page();
> -       if (!buf)
> +       if (!vm_is_vm()) {
> +               initialized = true;
>                 return false;

I would personally prefer return is_tcg here to make it obvious we're
relying on the previous initalization to false for subsequent calls.

> +       }
> +
> +       buf = alloc_page();
> +       assert(buf);
>  
>         if (stsi(buf, 1, 1, 1))
>                 goto out;
> @@ -43,3 +48,50 @@ out:
>         free_page(buf);
>         return is_tcg;
>  }
> +
> +/**
> + * Detect whether we are running with KVM
> + */
> +

No newline here.

> +bool vm_is_kvm(void)
> +{
> +       /* EBCDIC for "KVM/" */
> +       const uint8_t kvm_ebcdic[] = { 0xd2, 0xe5, 0xd4, 0x61 };
> +       static bool initialized;
> +       static bool is_kvm;

Might make sense to initizalize these to false to make it consistent
with vm_is_tcg().

Nico Boehr Feb. 8, 2022, 3:43 p.m. UTC | #3

On Tue, 2022-02-08 at 16:31 +0100, Janosch Frank wrote:
> > diff --git a/lib/s390x/stsi.h b/lib/s390x/stsi.h
> > new file mode 100644
> > index 00000000..9b40664f
> > --- /dev/null
> > +++ b/lib/s390x/stsi.h
> > @@ -0,0 +1,32 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Structures used to Store System Information
> > + *
> > + * Copyright IBM Corp. 2022
> > + */
> > +
> > +#ifndef _S390X_STSI_H_
> > +#define _S390X_STSI_H_
> > +
> > +struct sysinfo_3_2_2 {
> 
> Any particular reason why you renamed this?

Stumbled across this as well. I think this makes it consistent with
Linux' arch/s390/include/asm/sysinfo.h.

The PoP, on the other hand, calls it SYSIB, so this at least resolves
the inconsistency between kvm-unit-tests and Linux.

Pierre Morel Feb. 14, 2022, 7:55 a.m. UTC | #4

On 2/8/22 16:35, Nico Boehr wrote:
> On Tue, 2022-02-08 at 14:27 +0100, Pierre Morel wrote:
>> We need in several tests to check if the VM we are running in
>> is KVM.
>> Let's add the test.
>>
>> To check the VM type we use the STSI 3.2.2 instruction, let's
>> define it's response structure in a central header.
>>
>> Signed-off-by: Pierre Morel <[e-mail]>
> 
>> ---
>>   lib/s390x/stsi.h | 32 +++++++++++++++++++++++++++
>>   lib/s390x/vm.c   | 56
>> ++++++++++++++++++++++++++++++++++++++++++++++--
>>   lib/s390x/vm.h   |  3 +++
>>   s390x/stsi.c     | 23 ++------------------
>>   4 files changed, 91 insertions(+), 23 deletions(-)
>>   create mode 100644 lib/s390x/stsi.h
>>
>> diff --git a/lib/s390x/stsi.h b/lib/s390x/stsi.h
>> new file mode 100644
>> index 00000000..9b40664f
>> --- /dev/null
>> +++ b/lib/s390x/stsi.h
>> @@ -0,0 +1,32 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> 
> This was taken from stsi.c which is GPL 2 only, so this probably should
> be as well.
> 
>> diff --git a/lib/s390x/vm.c b/lib/s390x/vm.c
>> index a5b92863..38886b76 100644
>> --- a/lib/s390x/vm.c
>> +++ b/lib/s390x/vm.c
>> @@ -12,6 +12,7 @@
>>   #include <alloc_page.h>
>>   #include <asm/arch_def.h>
>>   #include "vm.h"
>> +#include "stsi.h"
>>   
>>   /**
>>    * Detect whether we are running with TCG (instead of KVM)
>> @@ -26,9 +27,13 @@ bool vm_is_tcg(void)
>>          if (initialized)
>>                  return is_tcg;
>>   
>> -       buf = alloc_page();
>> -       if (!buf)
>> +       if (!vm_is_vm()) {
>> +               initialized = true;
>>                  return false;
>> diff --git a/lib/s390x/vm.c b/lib/s390x/vm.c
>> index a5b92863..38886b76 100644
>> --- a/lib/s390x/vm.c
>> +++ b/lib/s390x/vm.c
>> @@ -12,6 +12,7 @@
>>   #include <alloc_page.h>
>>   #include <asm/arch_def.h>
>>   #include "vm.h"
>> +#include "stsi.h"
>>   
>>   /**
>>    * Detect whether we are running with TCG (instead of KVM)
>> @@ -26,9 +27,13 @@ bool vm_is_tcg(void)
>>          if (initialized)
>>                  return is_tcg;
>>   
>> -       buf = alloc_page();
>> -       if (!buf)
>> +       if (!vm_is_vm()) {
>> +               initialized = true;
>>                  return false;
> 
> I would personally prefer return is_tcg here to make it obvious we're
> relying on the previous initalization to false for subsequent calls.

OK

> 
>> +       }
>> +
>> +       buf = alloc_page();
>> +       assert(buf);
>>   
>>          if (stsi(buf, 1, 1, 1))
>>                  goto out;
>> @@ -43,3 +48,50 @@ out:
>>          free_page(buf);
>>          return is_tcg;
>>   }
>> +
>> +/**
>> + * Detect whether we are running with KVM
>> + */
>> +
> 
> No newline here.

ok

> 
>> +bool vm_is_kvm(void)
>> +{
>> +       /* EBCDIC for "KVM/" */
>> +       const uint8_t kvm_ebcdic[] = { 0xd2, 0xe5, 0xd4, 0x61 };
>> +       static bool initialized;
>> +       static bool is_kvm;
> 
> Might make sense to initizalize these to false to make it consistent
> with vm_is_tcg().
> 

OK

Pierre Morel Feb. 14, 2022, 8:01 a.m. UTC | #5

On 2/8/22 16:43, Nico Boehr wrote:
> On Tue, 2022-02-08 at 16:31 +0100, Janosch Frank wrote:
>>> diff --git a/lib/s390x/stsi.h b/lib/s390x/stsi.h
>>> new file mode 100644
>>> index 00000000..9b40664f
>>> --- /dev/null
>>> +++ b/lib/s390x/stsi.h
>>> @@ -0,0 +1,32 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>> +/*
>>> + * Structures used to Store System Information
>>> + *
>>> + * Copyright IBM Corp. 2022
>>> + */
>>> +
>>> +#ifndef _S390X_STSI_H_
>>> +#define _S390X_STSI_H_
>>> +
>>> +struct sysinfo_3_2_2 {
>>
>> Any particular reason why you renamed this?
> 
> Stumbled across this as well. I think this makes it consistent with
> Linux' arch/s390/include/asm/sysinfo.h.
> 
> The PoP, on the other hand, calls it SYSIB, so this at least resolves
> the inconsistency between kvm-unit-tests and Linux.
> 

In fact I do not know why I renamed it probably because I coded the next 
patches first and used sysinfo in it.
QEMU calls these structures SysIB so for me we do as you want.

Pierre Morel Feb. 14, 2022, 9:18 a.m. UTC | #6

On 2/8/22 16:31, Janosch Frank wrote:
> On 2/8/22 14:27, Pierre Morel wrote:
>> We need in several tests to check if the VM we are running in
>> is KVM.
>> Let's add the test.
> 
> Several tests are in need of a way to check on which hypervisor and 
> virtualization level they are running on to be able to fence certain 
> tests. This patch adds functions that return true if a vm is running 
> under KVM, LPAR or generally as a level 2 guest.
> 
>>
>> To check the VM type we use the STSI 3.2.2 instruction, let's
> 
> To check if we're running under KVM we...

OK

> 
>> define it's response structure in a central header.
> 
> Since we already have a stsi test that defines the 3.2.2 structure let's 
> move the struct from the test into a new library header.

OK

> 
>>
>> Signed-off-by: Pierre Morel <[e-mail]>
>> ---
>>   lib/s390x/stsi.h | 32 +++++++++++++++++++++++++++
>>   lib/s390x/vm.c   | 56 ++++++++++++++++++++++++++++++++++++++++++++++--
>>   lib/s390x/vm.h   |  3 +++
>>   s390x/stsi.c     | 23 ++------------------
>>   4 files changed, 91 insertions(+), 23 deletions(-)
>>   create mode 100644 lib/s390x/stsi.h
>>
>> diff --git a/lib/s390x/stsi.h b/lib/s390x/stsi.h
>> new file mode 100644
>> index 00000000..9b40664f
>> --- /dev/null
>> +++ b/lib/s390x/stsi.h
>> @@ -0,0 +1,32 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Structures used to Store System Information
>> + *
>> + * Copyright IBM Corp. 2022
>> + */
>> +
>> +#ifndef _S390X_STSI_H_
>> +#define _S390X_STSI_H_
>> +
>> +struct sysinfo_3_2_2 {
> 
> Any particular reason why you renamed this?

In addition to what I answered to Nico:
I found stsi_3_2_2 better to describe a function calling STSI(3,2,2) 
than the result of the STSI(3,2,2) instruction, the SYStem Information 
Block.


> 
>> +    uint8_t reserved[31];
>> +    uint8_t count;
>> +    struct {
>> +        uint8_t reserved2[4];
>> +        uint16_t total_cpus;
>> +        uint16_t conf_cpus;
>> +        uint16_t standby_cpus;
>> +        uint16_t reserved_cpus;
>> +        uint8_t name[8];
>> +        uint32_t caf;
>> +        uint8_t cpi[16];
>> +        uint8_t reserved5[3];
>> +        uint8_t ext_name_encoding;
>> +        uint32_t reserved3;
>> +        uint8_t uuid[16];
>> +    } vm[8];
>> +    uint8_t reserved4[1504];
>> +    uint8_t ext_names[8][256];
>> +};
>> +
>> +#endif  /* _S390X_STSI_H_ */
>> diff --git a/lib/s390x/vm.c b/lib/s390x/vm.c
>> index a5b92863..38886b76 100644
>> --- a/lib/s390x/vm.c
>> +++ b/lib/s390x/vm.c
>> @@ -12,6 +12,7 @@
>>   #include <alloc_page.h>
>>   #include <asm/arch_def.h>
>>   #include "vm.h"
>> +#include "stsi.h"
>>   /**
>>    * Detect whether we are running with TCG (instead of KVM)
>> @@ -26,9 +27,13 @@ bool vm_is_tcg(void)
>>       if (initialized)
>>           return is_tcg;
>> -    buf = alloc_page();
>> -    if (!buf)
>> +    if (!vm_is_vm()) {
>> +        initialized = true;
>>           return false;
>> +    }
>> +
>> +    buf = alloc_page();
>> +    assert(buf);
>>       if (stsi(buf, 1, 1, 1))
>>           goto out;
>> @@ -43,3 +48,50 @@ out:
>>       free_page(buf);
>>       return is_tcg;
>>   }
>> +
>> +/**
>> + * Detect whether we are running with KVM
>> + */
>> +
>> +bool vm_is_kvm(void)
>> +{
>> +    /* EBCDIC for "KVM/" */
>> +    const uint8_t kvm_ebcdic[] = { 0xd2, 0xe5, 0xd4, 0x61 };
>> +    static bool initialized;
>> +    static bool is_kvm;
>> +    struct sysinfo_3_2_2 *stsi_322;
>> +
>> +    if (initialized)
>> +        return is_kvm;
>> +
>> +    if (!vm_is_vm() || vm_is_tcg()) {
>> +        initialized = true;
>> +        return is_kvm;
>> +    }
>> +
>> +    stsi_322 = alloc_page();
>> +    assert(stsi_322);
>> +
>> +    if (stsi(stsi_322, 3, 2, 2))
>> +        goto out;
>> +
>> +    /*
>> +     * If the manufacturer string is "KVM/" in EBCDIC, then we
>> +     * are on KVM.
>> +     */
>> +    is_kvm = !memcmp(&stsi_322->vm[0].cpi, kvm_ebcdic, 
>> sizeof(kvm_ebcdic));
>> +    initialized = true;
>> +out:
>> +    free_page(stsi_322);
>> +    return is_kvm;
>> +}
>> +
>> +bool vm_is_lpar(void)
>> +{
>> +    return stsi_get_fc() == 2;
>> +}
>> +
>> +bool vm_is_vm(void)
>> +{
>> +    return stsi_get_fc() == 3;
> 
> This would be true when running under z/VM, no?

yes

> 
> I.e. what you're testing here is that we're a level 2 guest and hence 
> the naming could be improved.

STSI(0,x,x) used by stsi_get_fc() returns the current configuration 
level number which can be one of: basic machine (1), LPAR(2) or VM(3)

by the way stsi_get_fc() should probably better be named 
stsi_get_cfglevel() or something like that.

> 
> Also: what if we're under VSIE where we would be > 3?

No other configuration level is defined in the architecture than (1,2,3)

> 
>> +}
>> diff --git a/lib/s390x/vm.h b/lib/s390x/vm.h
>> index 7abba0cc..3aaf76af 100644
>> --- a/lib/s390x/vm.h
>> +++ b/lib/s390x/vm.h
>> @@ -9,5 +9,8 @@
>>   #define _S390X_VM_H_
>>   bool vm_is_tcg(void);
>> +bool vm_is_kvm(void);
>> +bool vm_is_vm(void);
>> +bool vm_is_lpar(void);
>>   #endif  /* _S390X_VM_H_ */
>> diff --git a/s390x/stsi.c b/s390x/stsi.c
>> index 391f8849..1ed045e2 100644
>> --- a/s390x/stsi.c
>> +++ b/s390x/stsi.c
>> @@ -13,27 +13,8 @@
>>   #include <asm/asm-offsets.h>
>>   #include <asm/interrupt.h>
>>   #include <smp.h>
>> +#include "stsi.h"
> 
> #include <stsi.h>
> 
> We're not in the lib.

OK

> 
>> -struct stsi_322 {
>> -    uint8_t reserved[31];
>> -    uint8_t count;
>> -    struct {
>> -        uint8_t reserved2[4];
>> -        uint16_t total_cpus;
>> -        uint16_t conf_cpus;
>> -        uint16_t standby_cpus;
>> -        uint16_t reserved_cpus;
>> -        uint8_t name[8];
>> -        uint32_t caf;
>> -        uint8_t cpi[16];
>> -        uint8_t reserved5[3];
>> -        uint8_t ext_name_encoding;
>> -        uint32_t reserved3;
>> -        uint8_t uuid[16];
>> -    } vm[8];
>> -    uint8_t reserved4[1504];
>> -    uint8_t ext_names[8][256];
>> -};
>>   static uint8_t pagebuf[PAGE_SIZE * 2] 
>> __attribute__((aligned(PAGE_SIZE * 2)));
>>   static void test_specs(void)
>> @@ -91,7 +72,7 @@ static void test_3_2_2(void)
>>       /* EBCDIC for "KVM/" */
>>       const uint8_t cpi_kvm[] = { 0xd2, 0xe5, 0xd4, 0x61 };
>>       const char vm_name_ext[] = "kvm-unit-test";
>> -    struct stsi_322 *data = (void *)pagebuf;
>> +    struct sysinfo_3_2_2 *data = (void *)pagebuf;
>>       report_prefix_push("3.2.2");
> 

Thanks for the review,
Pierre

Claudio Imbrenda Feb. 15, 2022, 11:18 a.m. UTC | #7

On Tue,  8 Feb 2022 14:27:07 +0100
Pierre Morel <[e-mail]> wrote:

> We need in several tests to check if the VM we are running in
> is KVM.
> Let's add the test.
> 
> To check the VM type we use the STSI 3.2.2 instruction, let's
> define it's response structure in a central header.
> 
> Signed-off-by: Pierre Morel <[e-mail]>
> ---
>  lib/s390x/stsi.h | 32 +++++++++++++++++++++++++++
>  lib/s390x/vm.c   | 56 ++++++++++++++++++++++++++++++++++++++++++++++--
>  lib/s390x/vm.h   |  3 +++
>  s390x/stsi.c     | 23 ++------------------
>  4 files changed, 91 insertions(+), 23 deletions(-)
>  create mode 100644 lib/s390x/stsi.h
> 
> diff --git a/lib/s390x/stsi.h b/lib/s390x/stsi.h
> new file mode 100644
> index 00000000..9b40664f
> --- /dev/null
> +++ b/lib/s390x/stsi.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Structures used to Store System Information
> + *
> + * Copyright IBM Corp. 2022
> + */
> +
> +#ifndef _S390X_STSI_H_
> +#define _S390X_STSI_H_
> +
> +struct sysinfo_3_2_2 {
> +	uint8_t reserved[31];
> +	uint8_t count;
> +	struct {
> +		uint8_t reserved2[4];
> +		uint16_t total_cpus;
> +		uint16_t conf_cpus;
> +		uint16_t standby_cpus;
> +		uint16_t reserved_cpus;
> +		uint8_t name[8];
> +		uint32_t caf;
> +		uint8_t cpi[16];
> +		uint8_t reserved5[3];
> +		uint8_t ext_name_encoding;
> +		uint32_t reserved3;
> +		uint8_t uuid[16];
> +	} vm[8];
> +	uint8_t reserved4[1504];
> +	uint8_t ext_names[8][256];
> +};
> +
> +#endif  /* _S390X_STSI_H_ */
> diff --git a/lib/s390x/vm.c b/lib/s390x/vm.c
> index a5b92863..38886b76 100644
> --- a/lib/s390x/vm.c
> +++ b/lib/s390x/vm.c
> @@ -12,6 +12,7 @@
>  #include <alloc_page.h>
>  #include <asm/arch_def.h>
>  #include "vm.h"
> +#include "stsi.h"
>  
>  /**
>   * Detect whether we are running with TCG (instead of KVM)
> @@ -26,9 +27,13 @@ bool vm_is_tcg(void)
>  	if (initialized)
>  		return is_tcg;
>  
> -	buf = alloc_page();
> -	if (!buf)
> +	if (!vm_is_vm()) {
> +		initialized = true;
>  		return false;
> +	}
> +
> +	buf = alloc_page();
> +	assert(buf);
>  
>  	if (stsi(buf, 1, 1, 1))
>  		goto out;
> @@ -43,3 +48,50 @@ out:
>  	free_page(buf);
>  	return is_tcg;
>  }
> +
> +/**
> + * Detect whether we are running with KVM
> + */
> +
> +bool vm_is_kvm(void)

I think this is too messy

I think a cleaner approach would be to have one "detect_environment"
function to call stsi and find out which environment we are running on.

then the various vm_is_* would just be something like

bool vm_is_kvm(void)
{
	return detect_environment() == VM_IS_KVM;
}

obviously the detect_environment function would call stsi only once and
then cache the result.

bonus, we could make that function public too, so e.g. a testcase could
do a switch, instead of having to do a series of nested ifs

> +{
> +	/* EBCDIC for "KVM/" */
> +	const uint8_t kvm_ebcdic[] = { 0xd2, 0xe5, 0xd4, 0x61 };
> +	static bool initialized;
> +	static bool is_kvm;
> +	struct sysinfo_3_2_2 *stsi_322;
> +
> +	if (initialized)
> +		return is_kvm;
> +
> +	if (!vm_is_vm() || vm_is_tcg()) {
> +		initialized = true;
> +		return is_kvm;
> +	}
> +
> +	stsi_322 = alloc_page();
> +	assert(stsi_322);
> +
> +	if (stsi(stsi_322, 3, 2, 2))
> +		goto out;
> +
> +	/*
> +	 * If the manufacturer string is "KVM/" in EBCDIC, then we
> +	 * are on KVM.
> +	 */
> +	is_kvm = !memcmp(&stsi_322->vm[0].cpi, kvm_ebcdic, sizeof(kvm_ebcdic));
> +	initialized = true;
> +out:
> +	free_page(stsi_322);
> +	return is_kvm;
> +}
> +
> +bool vm_is_lpar(void)
> +{
> +	return stsi_get_fc() == 2;
> +}
> +
> +bool vm_is_vm(void)

is it enough to call it vm? maybe zvm? I don't have a strong opinion,
though

> +{
> +	return stsi_get_fc() == 3;
> +}
> diff --git a/lib/s390x/vm.h b/lib/s390x/vm.h
> index 7abba0cc..3aaf76af 100644
> --- a/lib/s390x/vm.h
> +++ b/lib/s390x/vm.h
> @@ -9,5 +9,8 @@
>  #define _S390X_VM_H_
>  
>  bool vm_is_tcg(void);
> +bool vm_is_kvm(void);
> +bool vm_is_vm(void);
> +bool vm_is_lpar(void);
>  
>  #endif  /* _S390X_VM_H_ */
> diff --git a/s390x/stsi.c b/s390x/stsi.c
> index 391f8849..1ed045e2 100644
> --- a/s390x/stsi.c
> +++ b/s390x/stsi.c
> @@ -13,27 +13,8 @@
>  #include <asm/asm-offsets.h>
>  #include <asm/interrupt.h>
>  #include <smp.h>
> +#include "stsi.h"
>  
> -struct stsi_322 {
> -	uint8_t reserved[31];
> -	uint8_t count;
> -	struct {
> -		uint8_t reserved2[4];
> -		uint16_t total_cpus;
> -		uint16_t conf_cpus;
> -		uint16_t standby_cpus;
> -		uint16_t reserved_cpus;
> -		uint8_t name[8];
> -		uint32_t caf;
> -		uint8_t cpi[16];
> -		uint8_t reserved5[3];
> -		uint8_t ext_name_encoding;
> -		uint32_t reserved3;
> -		uint8_t uuid[16];
> -	} vm[8];
> -	uint8_t reserved4[1504];
> -	uint8_t ext_names[8][256];
> -};
>  static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2)));
>  
>  static void test_specs(void)
> @@ -91,7 +72,7 @@ static void test_3_2_2(void)
>  	/* EBCDIC for "KVM/" */
>  	const uint8_t cpi_kvm[] = { 0xd2, 0xe5, 0xd4, 0x61 };
>  	const char vm_name_ext[] = "kvm-unit-test";
> -	struct stsi_322 *data = (void *)pagebuf;
> +	struct sysinfo_3_2_2 *data = (void *)pagebuf;
>  
>  	report_prefix_push("3.2.2");
>

Patch

diff --git a/lib/s390x/stsi.h b/lib/s390x/stsi.h
new file mode 100644
index 00000000..9b40664f
--- /dev/null
+++ b/lib/s390x/stsi.h
@@ -0,0 +1,32 @@ 
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Structures used to Store System Information
+ *
+ * Copyright IBM Corp. 2022
+ */
+
+#ifndef _S390X_STSI_H_
+#define _S390X_STSI_H_
+
+struct sysinfo_3_2_2 {
+	uint8_t reserved[31];
+	uint8_t count;
+	struct {
+		uint8_t reserved2[4];
+		uint16_t total_cpus;
+		uint16_t conf_cpus;
+		uint16_t standby_cpus;
+		uint16_t reserved_cpus;
+		uint8_t name[8];
+		uint32_t caf;
+		uint8_t cpi[16];
+		uint8_t reserved5[3];
+		uint8_t ext_name_encoding;
+		uint32_t reserved3;
+		uint8_t uuid[16];
+	} vm[8];
+	uint8_t reserved4[1504];
+	uint8_t ext_names[8][256];
+};
+
+#endif  /* _S390X_STSI_H_ */
diff --git a/lib/s390x/vm.c b/lib/s390x/vm.c
index a5b92863..38886b76 100644
--- a/lib/s390x/vm.c
+++ b/lib/s390x/vm.c
@@ -12,6 +12,7 @@ 
 #include <alloc_page.h>
 #include <asm/arch_def.h>
 #include "vm.h"
+#include "stsi.h"
 
 /**
  * Detect whether we are running with TCG (instead of KVM)
@@ -26,9 +27,13 @@  bool vm_is_tcg(void)
 	if (initialized)
 		return is_tcg;
 
-	buf = alloc_page();
-	if (!buf)
+	if (!vm_is_vm()) {
+		initialized = true;
 		return false;
+	}
+
+	buf = alloc_page();
+	assert(buf);
 
 	if (stsi(buf, 1, 1, 1))
 		goto out;
@@ -43,3 +48,50 @@  out:
 	free_page(buf);
 	return is_tcg;
 }
+
+/**
+ * Detect whether we are running with KVM
+ */
+
+bool vm_is_kvm(void)
+{
+	/* EBCDIC for "KVM/" */
+	const uint8_t kvm_ebcdic[] = { 0xd2, 0xe5, 0xd4, 0x61 };
+	static bool initialized;
+	static bool is_kvm;
+	struct sysinfo_3_2_2 *stsi_322;
+
+	if (initialized)
+		return is_kvm;
+
+	if (!vm_is_vm() || vm_is_tcg()) {
+		initialized = true;
+		return is_kvm;
+	}
+
+	stsi_322 = alloc_page();
+	assert(stsi_322);
+
+	if (stsi(stsi_322, 3, 2, 2))
+		goto out;
+
+	/*
+	 * If the manufacturer string is "KVM/" in EBCDIC, then we
+	 * are on KVM.
+	 */
+	is_kvm = !memcmp(&stsi_322->vm[0].cpi, kvm_ebcdic, sizeof(kvm_ebcdic));
+	initialized = true;
+out:
+	free_page(stsi_322);
+	return is_kvm;
+}
+
+bool vm_is_lpar(void)
+{
+	return stsi_get_fc() == 2;
+}
+
+bool vm_is_vm(void)
+{
+	return stsi_get_fc() == 3;
+}
diff --git a/lib/s390x/vm.h b/lib/s390x/vm.h
index 7abba0cc..3aaf76af 100644
--- a/lib/s390x/vm.h
+++ b/lib/s390x/vm.h
@@ -9,5 +9,8 @@ 
 #define _S390X_VM_H_
 
 bool vm_is_tcg(void);
+bool vm_is_kvm(void);
+bool vm_is_vm(void);
+bool vm_is_lpar(void);
 
 #endif  /* _S390X_VM_H_ */
diff --git a/s390x/stsi.c b/s390x/stsi.c
index 391f8849..1ed045e2 100644
--- a/s390x/stsi.c
+++ b/s390x/stsi.c
@@ -13,27 +13,8 @@ 
 #include <asm/asm-offsets.h>
 #include <asm/interrupt.h>
 #include <smp.h>
+#include "stsi.h"
 
-struct stsi_322 {
-	uint8_t reserved[31];
-	uint8_t count;
-	struct {
-		uint8_t reserved2[4];
-		uint16_t total_cpus;
-		uint16_t conf_cpus;
-		uint16_t standby_cpus;
-		uint16_t reserved_cpus;
-		uint8_t name[8];
-		uint32_t caf;
-		uint8_t cpi[16];
-		uint8_t reserved5[3];
-		uint8_t ext_name_encoding;
-		uint32_t reserved3;
-		uint8_t uuid[16];
-	} vm[8];
-	uint8_t reserved4[1504];
-	uint8_t ext_names[8][256];
-};
 static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2)));
 
 static void test_specs(void)
@@ -91,7 +72,7 @@  static void test_3_2_2(void)
 	/* EBCDIC for "KVM/" */
 	const uint8_t cpi_kvm[] = { 0xd2, 0xe5, 0xd4, 0x61 };
 	const char vm_name_ext[] = "kvm-unit-test";
-	struct stsi_322 *data = (void *)pagebuf;
+	struct sysinfo_3_2_2 *data = (void *)pagebuf;
 
 	report_prefix_push("3.2.2");