Re: [PATCH v7 1/1] s390x: KVM: guest support for topology function

[Pierre Morel <pmorel@linux.ibm.com>]

On 2/18/22 15:28, Janosch Frank wrote:
> On 2/18/22 14:13, Pierre Morel wrote:
>>
>>
>> On 2/17/22 18:17, Nico Boehr wrote:
>>> On Thu, 2022-02-17 at 10:59 +0100, Pierre Morel wrote:
>>> [...]
>>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>>> index 2296b1ff1e02..af7ea8488fa2 100644
>>>> --- a/arch/s390/kvm/kvm-s390.c
>>>> +++ b/arch/s390/kvm/kvm-s390.c
>>> [...]
> 
> Why is there no interface to clear the SCA_UTILITY_MTCR on a subsystem 
> reset?

Right, I had one in my first version based on interception but I forgot 
to implement an equivalent for KVM as I modified the implementation for 
interpretation.
I will add this.

> 
> 
>>>> -void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>>>> +/**
>>>> + * kvm_s390_vcpu_set_mtcr
>>>> + * @vcp: the virtual CPU
>>>> + *
>>>> + * Is only relevant if the topology facility is present.
>>>> + *
>>>> + * Updates the Multiprocessor Topology-Change-Report to signal
>>>> + * the guest with a topology change.
>>>> + */
>>>> +static void kvm_s390_vcpu_set_mtcr(struct kvm_vcpu *vcpu)
>>>>    {
>>>> +       struct esca_block *esca = vcpu->kvm->arch.sca;
>>>
>>> utility is at the same offset for the bsca and the esca, still
>>> wondering whether it is a good idea to assume esca here...
>>
>> We can take bsca to be coherent with the include file where we define
>> ESCA_UTILITY_MTCR inside the bsca.
>> And we can rename the define to SCA_UTILITY_MTCR as it is common for
>> both BSCA and ESCA the (E) is too much.
> 
> Yes and maybe add a comment that it's at the same offset for esca so 
> there won't come up further questions in the future.

OK

> 
>>
>>>
>>> [...]
>>>> diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
>>>> index 098831e815e6..af04ffbfd587 100644
>>>> --- a/arch/s390/kvm/kvm-s390.h
>>>> +++ b/arch/s390/kvm/kvm-s390.h
>>>> @@ -503,4 +503,29 @@ void kvm_s390_vcpu_crypto_reset_all(struct kvm
>>>> *kvm);
>>>>     */
>>>>    extern unsigned int diag9c_forwarding_hz;
>>>> +#define S390_KVM_TOPOLOGY_NEW_CPU -1
>>>> +/**
>>>> + * kvm_s390_topology_changed
>>>> + * @vcpu: the virtual CPU
>>>> + *
>>>> + * If the topology facility is present, checks if the CPU toplogy
>>>> + * viewed by the guest changed due to load balancing or CPU hotplug.
>>>> + */
>>>> +static inline bool kvm_s390_topology_changed(struct kvm_vcpu *vcpu)
>>>> +{
>>>> +       if (!test_kvm_facility(vcpu->kvm, 11))
>>>> +               return false;
>>>> +
>>>> +       /* A new vCPU has been hotplugged */
>>>> +       if (vcpu->arch.prev_cpu == S390_KVM_TOPOLOGY_NEW_CPU)
>>>> +               return true;
>>>> +
>>>> +       /* The real CPU backing up the vCPU moved to another socket
>>>> */
>>>> +       if (topology_physical_package_id(vcpu->cpu) !=
>>>> +           topology_physical_package_id(vcpu->arch.prev_cpu))
>>>> +               return true;
>>>
>>> Why is it OK to look just at the physical package ID here? What if the
>>> vcpu for example moves to a different book, which has a core with the
>>> same physical package ID?
> 
> I'll need to look up stsi 15* output to understand this.
> But the architecture states that any change to the stsi 15 output sets 
> the change bit so I'd guess Nico is correct.
>

Yes, Nico is correct, as I already answered, however it is not any 
change of stsi(15) but a change of stsi(15.1.2) output which sets the 
change bit.
However the socket identifier is relative to the book and not unique for 
the all system.
The solution given by Heiko is, of course, the most elegant so I will 
use it.

Thanks,

regards,
Pierre

> 

-- 
Pierre Morel
IBM Lab Boeblingen