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

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

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?


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

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

>>
> 
> You are right, we should look at the drawer and book id too.
> Something like that I think:
> 
>           if ((topology_physical_package_id(vcpu->cpu) !=
>                topology_physical_package_id(vcpu->arch.prev_cpu)) ||
>               (topology_book_id(vcpu->cpu) !=
>                topology_book_id(vcpu->arch.prev_cpu)) ||
>               (topology_drawer_id(vcpu->cpu) !=
>                topology_drawer_id(vcpu->arch.prev_cpu)))
>                   return true;
> 
> 
> Thanks,
> regards,
> Pierre