Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <d8033b41-ca12-8d28-3482-c716838f05d7@linux.ibm.com>
Date:   Tue, 5 Jul 2022 10:09:48 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.10.0
Subject: Re: [PATCH v11 3/3] KVM: s390: resetting the Topology-Change-Report
Content-Language: en-US
To:     Pierre Morel <pmorel@linux.ibm.com>, kvm@vger.kernel.org
Cc:     linux-s390@vger.kernel.org, linux-kernel@vger.kernel.org,
        borntraeger@de.ibm.com, frankja@linux.ibm.com, cohuck@redhat.com,
        david@redhat.com, thuth@redhat.com, imbrenda@linux.ibm.com,
        hca@linux.ibm.com, gor@linux.ibm.com, wintera@linux.ibm.com,
        seiden@linux.ibm.com, nrb@linux.ibm.com
References: <20220701162559.158313-1-pmorel@linux.ibm.com>
 <20220701162559.158313-4-pmorel@linux.ibm.com>
 <d90e2aaa-05ad-6f3a-83f8-428677256673@linux.ibm.com>
 <1f3b404f-0bd6-31cb-57de-591d2e03dd76@linux.ibm.com>
From:   Janis Schoetterl-Glausch <scgl@linux.ibm.com>
In-Reply-To: <1f3b404f-0bd6-31cb-57de-591d2e03dd76@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 7/4/22 15:56, Pierre Morel wrote:
> 
> 
> On 7/4/22 11:35, Janis Schoetterl-Glausch wrote:
>> On 7/1/22 18:25, Pierre Morel wrote:
>>> During a subsystem reset the Topology-Change-Report is cleared.
>>>
>>> Let's give userland the possibility to clear the MTCR in the case
>>> of a subsystem reset.
>>>
>>> To migrate the MTCR, we give userland the possibility to
>>> query the MTCR state.
>>>
>>> We indicate KVM support for the CPU topology facility with a new
>>> KVM capability: KVM_CAP_S390_CPU_TOPOLOGY.
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>   Documentation/virt/kvm/api.rst   | 25 +++++++++++++++
>>>   arch/s390/include/uapi/asm/kvm.h | 10 ++++++
>>>   arch/s390/kvm/kvm-s390.c         | 53 ++++++++++++++++++++++++++++++++
>>>   include/uapi/linux/kvm.h         |  1 +
>>>   4 files changed, 89 insertions(+)
>>>
>>> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
>>> index 11e00a46c610..5e086125d8ad 100644
>>> --- a/Documentation/virt/kvm/api.rst
>>> +++ b/Documentation/virt/kvm/api.rst
>>> @@ -7956,6 +7956,31 @@ should adjust CPUID leaf 0xA to reflect that the PMU is disabled.
>>>   When enabled, KVM will exit to userspace with KVM_EXIT_SYSTEM_EVENT of
>>>   type KVM_SYSTEM_EVENT_SUSPEND to process the guest suspend request.
>>>   +8.37 KVM_CAP_S390_CPU_TOPOLOGY
>>> +------------------------------
>>> +
>>> +:Capability: KVM_CAP_S390_CPU_TOPOLOGY
>>> +:Architectures: s390
>>> +:Type: vm
>>> +
>>> +This capability indicates that KVM will provide the S390 CPU Topology
>>> +facility which consist of the interpretation of the PTF instruction for
>>> +the function code 2 along with interception and forwarding of both the
>>> +PTF instruction with function codes 0 or 1 and the STSI(15,1,x)
>>> +instruction to the userland hypervisor.
>> The latter only if the user STSI capability is also enabled.
> 
> Hum, not sure about this.
> we can not set facility 11 and return 3 to STSI(15) for valid selectors.

I think the PoP allows for this:

When the specified function-code, selector-1, and
selector-2 combination is invalid (is other than as
shown in Figure 10-84), or if it is valid but the
requested information is not available because the
specified level does not implement or does not fully
implement the instruction or because a necessary
part of the level is uninstalled or not initialized, and
provided that an exception is not recognized (see
“Special Conditions”), the condition code is set to 3.
When the function code is nonzero, the combination
is valid, the requested information is available, and
there is no exception, the requested information is
stored in a system-information block (SYSIB) at the
second-operand address.

So if user_stsi is off the information is not available because the level does not fully implement the instruction.
But I'm fine with KVM_CAP_S390_CPU_TOPOLOGY implying KVM_CAP_S390_USER_STSI, too.

> 
> I think that it was right before, KVM_CAP_S390_CPU_TOPOLOGY and KVM_CAP_S390_USER_STSI are independent in KVM, userland can turn on one and not the other.
> But KVM proposes both.
> 
> Of course it is stupid to turn on only KVM_CAP_S390_CPU_TOPOLOGY but KVM is not responsible for this userland is.
> 
> Otherwise, we need to check on KVM_CAP_S390_USER_STSI before authorizing  KVM_CAP_S390_CPU_TOPOLOGY and that looks even more complicated for me,
> or we suppress the KVM_CAP_S390_CPU_TOPOLOGY and implement the all stsi(15) in the kernel what I really do not think is good because of the complexity of the userland API

[...]