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/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.

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