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

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

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 ?

Do you have an experimental return on this?

> 
>> @@ -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

-- 
Pierre Morel
IBM Lab Boeblingen