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: <85d3e0ca-186d-197b-308c-d7629488bb8a@linux.ibm.com>
Date:   Wed, 13 Jul 2022 10:42:54 +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 v12 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: <20220711084148.25017-1-pmorel@linux.ibm.com>
 <20220711084148.25017-3-pmorel@linux.ibm.com>
 <92c6d13c-4494-de56-83f4-9d7384444008@linux.ibm.com>
 <1884bc26-b91b-83a7-7f8b-96b6090a0bac@linux.ibm.com>
 <6124248a-24be-b43a-f827-b6bebf9e7f3d@linux.ibm.com>
 <5c3d9637-7739-1323-8630-433ff8cb4dc4@linux.ibm.com>
 <899e5148-8e65-8260-6f3c-546b4f5a650f@linux.ibm.com>
From:   Pierre Morel <pmorel@linux.ibm.com>
In-Reply-To: <899e5148-8e65-8260-6f3c-546b4f5a650f@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 7/13/22 10:34, Janosch Frank wrote:
> [...]
>>>>>>     +/**
>>>>>> + * 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)
>>>>>> +{
>>>>>> +    union sca_utility new, old;
>>>>>> +    struct bsca_block *sca;
>>>>>> +
>>>>>> +    read_lock(&kvm->arch.sca_lock);
>>>>>> +    do {
>>>>>> +        sca = kvm->arch.sca;
>>>>>
>>>>> I find this assignment being in the loop unintuitive, but it should 
>>>>> not make a difference.
>>>>
>>>> The price would be an ugly cast.
>>>
>>> I don't get what you mean. Nothing about the types changes if you 
>>> move it before the loop.
>>
>> Yes right, did wrong understand.
>> It is better before.
> With the assignment moved one line up:
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

Thanks

> 
>>
>>>>
>>>>
>>>>>
>>>>>> +        old = READ_ONCE(sca->utility);
>>>>>> +        new = old;
>>>>>> +        new.mtcr = val;
>>>>>> +    } while (cmpxchg(&sca->utility.val, old.val, new.val) != 
>>>>>> old.val);
>>>>>> +    read_unlock(&kvm->arch.sca_lock);
>>>>>> +}
>>>>>> +
>>>>> [...]
>>>>>
>>>>
>>>>
>>>
>>
> 

-- 
Pierre Morel
IBM Lab Boeblingen