Re: [PATCH v10 2/3] KVM: s390: guest support for topology function
From: Pierre Morel
Date: Tue Jun 28 2022 - 10:08:46 EST
On 6/28/22 14:18, Janis Schoetterl-Glausch wrote:
On 6/28/22 12:58, Pierre Morel wrote:
On 6/28/22 10:59, Janis Schoetterl-Glausch wrote:
On 6/20/22 14:54, 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
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.
Signed-off-by: Pierre Morel <pmorel@xxxxxxxxxxxxx>
---
arch/s390/include/asm/kvm_host.h | 11 ++++++++---
arch/s390/kvm/kvm-s390.c | 27 ++++++++++++++++++++++++++-
arch/s390/kvm/priv.c | 15 +++++++++++----
arch/s390/kvm/vsie.c | 3 +++
4 files changed, 48 insertions(+), 8 deletions(-)
[...]
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 8fcb56141689..95b96019ca8e 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -1691,6 +1691,25 @@ static int kvm_s390_get_cpu_model(struct kvm *kvm, struct kvm_device_attr *attr)
return ret;
}
+/**
+ * kvm_s390_sca_set_mtcr
I wonder if there is a better name, kvm_s390_report_topology_change maybe?
+ * @kvm: guest KVM description
+ *
+ * Is only relevant if the topology facility is present,
+ * the caller should check KVM facility 11
+ *
+ * Updates the Multiprocessor Topology-Change-Report to signal
+ * the guest with a topology change.
+ */
+static void kvm_s390_sca_set_mtcr(struct kvm *kvm)
+{
Do we need a sca_lock read_section here? If we don't why not?
Did not see one up the stack, but I might have overlooked something.
Yes we do.
As I said about your well justified comment in a previous mail, ipte_lock is not the right thing to use here and I will replace with an inter locked update.
Not sure I'm understanding you right, you're saying we need both? i.e.:
struct bsca_block *sca;
read_lock(&vcpu->kvm->arch.sca_lock);
sca = kvm->arch.sca;
atomic_or(SCA_UTILITY_MTCR, &sca->utility);
read_unlock(&vcpu->kvm->arch.sca_lock);
Obviously you would need to change the definition of the utility field and could not use a bit field like Janosch
suggested, unless you want to use a cmpxchg loop.
It's a bit ugly that utility is a two byte value.
Maybe there is a nicer way to set that bit, OR (OI, OIY) seem appropriate, but I don't know if they have a nice
abstraction in Linux or if you'd need inline asm.
I was think to something like this because it is what is used most of
the time when a bit is to be change concurrently with firmware.
+union sca_utility {
+ __u16 val;
+ struct {
+ __u16 mtcr : 1;
+ __u16 reserved : 15;
+ };
+};
+
struct bsca_block {
union ipte_control ipte_control;
__u64 reserved[5];
__u64 mcn;
- __u64 reserved2;
+ union sca_utility utility;
+ __u8 reserved2[6];
struct bsca_entry cpu[KVM_S390_BSCA_CPU_SLOTS];
}
....
static void kvm_s390_sca_set_mtcr(struct kvm *kvm, int val)
{
struct bsca_block *sca = kvm->arch.sca;
union sca_utility new, old;
read_lock(&kvm->arch.sca_lock);
do {
old = READ_ONCE(sca->utility);
new = old;
new.mtcr = val ? 1 : 0;
} while (cmpxchg(&sca->utility.val, old.val, new.val) != old.val);
read_lock(&kvm->arch.sca_lock);
}
+ struct bsca_block *sca = kvm->arch.sca; /* SCA version doesn't matter */
+
+ ipte_lock(kvm);
+ sca->utility |= SCA_UTILITY_MTCR;
+ ipte_unlock(kvm);
+}
+
[...]
--
Pierre Morel
IBM Lab Boeblingen