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

Pierre Morel
2022-02-14T09:16:54Z

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 <pmorel@linux.ibm.com>
>> ---
>>   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

--
Pierre Morel
IBM Lab Boeblingen