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

[Janosch Frank <frankja@linux.ibm.com>]

On 1/17/22 15:57, Pierre Morel wrote:
> 
> 
> On 1/11/22 13:27, Janosch Frank wrote:
>> On 1/10/22 14:37, 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 <pmorel@linux.ibm.com>
>>> ---
>>>    lib/s390x/stsi.h | 32 ++++++++++++++++++++++++++++++++
>>>    lib/s390x/vm.c   | 39 +++++++++++++++++++++++++++++++++++++++
>>>    lib/s390x/vm.h   |  1 +
>>>    s390x/stsi.c     | 23 ++---------------------
>>>    4 files changed, 74 insertions(+), 21 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..02cc94a6
>>> --- /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 (c) 2021 IBM Inc
>>> + */
>>> +
>>> +#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..3e11401e 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)
>>
>> We could add a fc < 3 check to the vm_is_tcg() function and add a
> 
> OK
> 
>> vm_is_lpar() which does a simple fc ==1 check.
> 
> hum, the doc says 1 is basic, 2 is lpar, 3 is vm, shouldn't we
> do a check on fc == 2 or have a vm_is_vm checking fc < 3 ?
> 

Right
I'll do some tests on the lpar stsi output and have a look what we get back.

> Do you have an experimental return on this?

ENOPARSE

> 
>>
>>> @@ -43,3 +44,41 @@ 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 (stsi_get_fc() < 3) {
>>> +        initialized = true;
>>> +        return is_kvm;
>>> +    }
>>> +
>>> +    stsi_322 = alloc_page();
>>> +    if (!stsi_322)
>>> +        return false;
>>> +
>>> +    if (stsi(stsi_322, 3, 2, 2))
>>> +        goto out;
>>> +
>>> +    /*
>>> +     * If the manufacturer string is "KVM/" in EBCDIC, then we
>>> +     * are on KVM (otherwise the string is "IBM" in EBCDIC)
>>> +     */
>>> +    is_kvm = !memcmp(&stsi_322->vm[0].cpi, kvm_ebcdic,
>>> sizeof(kvm_ebcdic));
>>
>> So I had a look at this before Christmas and I think it's wrong.
>>
>> QEMU will still set the cpi to KVM/LINUX if we are under tcg.
>> So we need to do add a !tcg check here and fix this comment.
>>
>> I.e. we always have the KVM/LINUX cpi but if we're under TCG the
>> manufacturer in fc == 1 is QEMU. I'm not sure if this is intentional and
>> if we want to fix this at some point or not.
> 
> indeed I did not check this!!
> 
>>
>>> +    initialized = true;
>>> +out:
>>> +    free_page(stsi_322);
>>> +    return is_kvm;
>>> +}
> 
> ...snip...
> 
> Thanks for the review, I make the changes.
> Pierre
>