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: <d0bfa9a5-0ca5-dc2e-061b-e88d713abe2f@linux.ibm.com>
Date:   Mon, 4 Jul 2022 15:51:48 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.0
Subject: Re: [PATCH v11 2/3] KVM: s390: guest support for topology function
Content-Language: en-US
To:     Janosch Frank <frankja@linux.ibm.com>,
        Janis Schoetterl-Glausch <scgl@linux.ibm.com>,
        kvm@vger.kernel.org
Cc:     linux-s390@vger.kernel.org, linux-kernel@vger.kernel.org,
        borntraeger@de.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-3-pmorel@linux.ibm.com>
 <579337ac-d040-197f-3553-7c8ff202623a@linux.ibm.com>
 <038d7c59-0c9a-7667-cf74-83009e186b42@linux.ibm.com>
 <5e7848ff-f2d9-cf2c-ee5e-5c2765cb2d21@linux.ibm.com>
From:   Pierre Morel <pmorel@linux.ibm.com>
In-Reply-To: <5e7848ff-f2d9-cf2c-ee5e-5c2765cb2d21@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 7/4/22 13:17, Janosch Frank wrote:
> On 7/4/22 13:02, Pierre Morel wrote:
>>
>>
>> On 7/4/22 11:08, Janis Schoetterl-Glausch wrote:
>>> On 7/1/22 18:25, Pierre Morel wrote:
>>>> We report a topology change to the guest for any CPU hotplug.
>>>>
>>>> The reporting to the guest is done using the Multiprocessor
>>>> Topology-Change-Report (MTCR) bit of the utility entry in the guest's
>>>> SCA which will be cleared during the interpretation of PTF.
>>>>
>>>> On every vCPU creation we set the MCTR bit to let the guest know the
>>>> next time he uses the PTF with command 2 instruction that the> 
>>>> topology changed and that he should use the STSI(15.1.x) instruction
>>> s/he/it (twice)
>>>> to get the topology details.
>>>>
>>>> STSI(15.1.x) gives information on the CPU configuration topology.
>>>> Let's accept the interception of STSI with the function code 15 and
>>>> let the userland part of the hypervisor handle it when userland
>>>> support the CPU Topology facility.And the user STSI capability.
>>> Also: supportS.
>>>>
>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>> Reviewed-by: Nico Boehr <nrb@linux.ibm.com>
>>>> ---
>>>>    arch/s390/include/asm/kvm_host.h | 18 +++++++++++++---
>>>>    arch/s390/kvm/kvm-s390.c         | 36 
>>>> ++++++++++++++++++++++++++++++++
>>>>    arch/s390/kvm/priv.c             | 16 ++++++++++----
>>>>    arch/s390/kvm/vsie.c             |  8 +++++++
>>>>    4 files changed, 71 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/arch/s390/include/asm/kvm_host.h 
>>>> b/arch/s390/include/asm/kvm_host.h
>>>> index 766028d54a3e..ae6bd3d607de 100644
>>>> --- a/arch/s390/include/asm/kvm_host.h
>>>> +++ b/arch/s390/include/asm/kvm_host.h
>>>> @@ -93,19 +93,30 @@ union ipte_control {
>>>>        };
>>>>    };
>>> [...]
>>>
>>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>>> index 8fcb56141689..ee59b03f2e45 100644
>>>> --- a/arch/s390/kvm/kvm-s390.c
>>>> +++ b/arch/s390/kvm/kvm-s390.c
>>>> @@ -1691,6 +1691,31 @@ static int kvm_s390_get_cpu_model(struct kvm 
>>>> *kvm, struct kvm_device_attr *attr)
>>>>        return ret;
>>>>    }
>>>> +/**
>>>> + * kvm_s390_update_topology_change_report - update CPU topology 
>>>> change report
>>>> + * @kvm: guest KVM description
>>>> + * @val: set or clear the MTCR bit
>>>> + *
>>>> + * Updates the Multiprocessor Topology-Change-Report bit to signal
>>>> + * the guest with a topology change.
>>>> + * This is only relevant if the topology facility is present.
>>>> + *
>>>> + * The SCA version, bsca or esca, doesn't matter as offset is the 
>>>> same.
>>>> + */
>>>> +static void kvm_s390_update_topology_change_report(struct kvm *kvm, 
>>>> bool val)
>>>> +{
>>>> +    struct bsca_block *sca = kvm->arch.sca;
>>>> +    union sca_utility new, old;
>>>> +
>>>> +    read_lock(&kvm->arch.sca_lock);
>>>
>>> You forgot to put the assignment of sca under the lock.
>>
>> Should I really?
>> What we want to protect here is the content of the sca.
>> The sca itself does not change during the life of the KVM AFAIK.
> 
> The SCA origin as well as the SCA contents can change within the 
> lifetime of a KVM VM.
> 
> When we switch from bsca to esca we'll use new pages.
> When we add/remove cpus we'll update the MCN and the CPU entry.
> 
> 

Oh! then Yes, right.
thanks


-- 
Pierre Morel
IBM Lab Boeblingen