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

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

On 2/16/22 13:26, Pierre Morel wrote:
> 
> 
> 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 ?

I guess it should be stsi_get_current_level() or something like that.
Let me add that to the list of vm.h rework items.

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