Messages in this thread | | | Date | Fri, 18 Feb 2022 15:28:18 +0100 | From | Janosch Frank <> | Subject | Re: [PATCH v7 1/1] s390x: KVM: guest support for topology function |
| |
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
| |