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

[Pierre Morel <pmorel@linux.ibm.com>]

On 2/16/22 09:13, Janosch Frank wrote:
> On 2/15/22 18:30, Pierre Morel wrote:
>>
>>
>> On 2/15/22 16:21, Claudio Imbrenda wrote:
>>> On Tue, 15 Feb 2022 16:08:16 +0100
>>> Janosch Frank <frankja@linux.ibm.com> wrote:
>>>
>>>> On 2/15/22 13:06, Claudio Imbrenda wrote:
>>>>> On Tue, 15 Feb 2022 11:46:32 +0100
>>>>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>>>>> 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 if we're running under KVM we use the STSI 3.2.2
>>>>>> instruction, let's define it's response structure in a central
>>>>>> header.
>>>>>
>>>>> sorry, I had replied to the old series, let me reply here too
>>>>>
>>>>>
>>>>> I think it would look cleaner if there was only one
>>>>> "detect_environment" function, that would call stsi once and detect 
>>>>> the
>>>>> environment, then the various vm_is_* would become something like
>>>>>
>>>>> bool vm_is_*(void)
>>>>> {
>>>>>     return detect_environment() == VM_IS_*;
>>>>> }
>>>>>
>>>>> of course detect_environment would also cache the result with static
>>>>> variables.
>>>>>
>>>>> bonus, we could make that function public, so a testcase could just
>>>>> switch over the type of hypervisor it's being run on, instead of 
>>>>> having
>>>>> to use a series of ifs.
>>>>>
>>>>> and then maybe the various vm_is_* could become static inlines to 
>>>>> be put
>>>>> in the header.
>>>>>
>>>>> please note that "detect_environment" is just the first thing that 
>>>>> came
>>>>> to my mind, I have no preference regarding the name.
>>>>
>>>> I'd like to keep this patch as simple as possible because there are
>>>> multiple patch sets which are gated by it.
>>>>
>>>> The vm.h code and the skey.c z/VM 6 check is a thorn in my side anyway
>>>> and I'd rather have it fixed properly which will likely result in a lot
>>>> of opinions being voiced.
>>>>
>>>> So I'd propose to rename vm_is_vm() to vm_is_guest2() and pick this 
>>>> patch.
>>>
>>> ok for me
>>>
>>> I'll rename the function and queue the patch
>>>
>>
>> Not OK for me, in the POP PTF do not do any difference between guest 2
>> and guest 3.
> 
> If we're running with HW virtualization then every guest >= 2 is a guest 
> 2 at the end. And most of the time we don't want to know the HW level 
> anyway, we want to know who our hypervisor is and vm_is_vm() doesn't 
> tell you one bit about that.

It tells us that we are running under a VM, the POP defines 1 as the 
basic machine, 2 as the LPAR and 3 as the Virtual Machine

I find this definition clear, much more clear than guest 2 that is why I 
used it.

> 
> At this point I would be happier if we remove the function and use 
> stsi_get_fc() == 3 directly. There's no arguing about what we're 
> checking when using that.

Speaking of function name, I do not understand the name of this function 
stsi_get_fc() : returning the function code ?

> 
> We're currently arguing about a function that's only used in this patch, 
> no?

It is absolutely unimportant for me, if you prefer this we do this.
I send the changes.

> 
>>
>> Or do you want to implement a VSIE specificity ?
>>
>>
>>>>
>>>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>>>> ---
>>>>>>     lib/s390x/stsi.h | 32 ++++++++++++++++++++++++++++
>>>>>>     lib/s390x/vm.c   | 55 
>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++--
>>>>>>     lib/s390x/vm.h   |  3 +++
>>>>>>     s390x/stsi.c     | 23 ++------------------
>>>>>>     4 files changed, 90 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..bebc492d
>>>>>> --- /dev/null
>>>>>> +++ b/lib/s390x/stsi.h
>>>>>> @@ -0,0 +1,32 @@
>>>>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>>>>> +/*
>>>>>> + * 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..91acd05b 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;
>>>>>> +    if (!vm_is_vm()) {
>>>>>> +        initialized = true;
>>>>>> +        return is_tcg;
>>>>>> +    }
>>>>>> +
>>>>>>         buf = alloc_page();
>>>>>> -    if (!buf)
>>>>>> -        return false;
>>>>>> +    assert(buf);
>>>>>>         if (stsi(buf, 1, 1, 1))
>>>>>>             goto out;
>>>>>> @@ -43,3 +48,49 @@ 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..dccc53e7 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");
>>>>
>>>
>>
> 

-- 
Pierre Morel
IBM Lab Boeblingen