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 <[email protected]>
---
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/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 766028d54a3e..bb54196d4ed6 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -97,15 +97,19 @@ struct bsca_block {
union ipte_control ipte_control;
__u64 reserved[5];
__u64 mcn;
- __u64 reserved2;
+#define SCA_UTILITY_MTCR 0x8000
+ __u16 utility;
+ __u8 reserved2[6];
struct bsca_entry cpu[KVM_S390_BSCA_CPU_SLOTS];
};
struct esca_block {
union ipte_control ipte_control;
- __u64 reserved1[7];
+ __u64 reserved1[6];
+ __u16 utility;
+ __u8 reserved2[6];
__u64 mcn[4];
- __u64 reserved2[20];
+ __u64 reserved3[20];
struct esca_entry cpu[KVM_S390_ESCA_CPU_SLOTS];
};
@@ -249,6 +253,7 @@ struct kvm_s390_sie_block {
#define ECB_SPECI 0x08
#define ECB_SRSI 0x04
#define ECB_HOSTPROTINT 0x02
+#define ECB_PTF 0x01
__u8 ecb; /* 0x0061 */
#define ECB2_CMMA 0x80
#define ECB2_IEP 0x20
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
+ * @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)
+{
+ struct bsca_block *sca = kvm->arch.sca; /* SCA version doesn't matter */
+
+ ipte_lock(kvm);
+ sca->utility |= SCA_UTILITY_MTCR;
+ ipte_unlock(kvm);
+}
+
static int kvm_s390_vm_set_attr(struct kvm *kvm, struct kvm_device_attr *attr)
{
int ret;
@@ -3143,7 +3162,6 @@ __u64 kvm_s390_get_cpu_timer(struct kvm_vcpu *vcpu)
void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
{
-
gmap_enable(vcpu->arch.enabled_gmap);
kvm_s390_set_cpuflags(vcpu, CPUSTAT_RUNNING);
if (vcpu->arch.cputm_enabled && !is_vcpu_idle(vcpu))
@@ -3272,6 +3290,11 @@ static int kvm_s390_vcpu_setup(struct kvm_vcpu *vcpu)
vcpu->arch.sie_block->ecb |= ECB_HOSTPROTINT;
if (test_kvm_facility(vcpu->kvm, 9))
vcpu->arch.sie_block->ecb |= ECB_SRSI;
+
+ /* PTF needs guest facilities to enable interpretation */
+ if (test_kvm_facility(vcpu->kvm, 11))
+ vcpu->arch.sie_block->ecb |= ECB_PTF;
+
if (test_kvm_facility(vcpu->kvm, 73))
vcpu->arch.sie_block->ecb |= ECB_TE;
if (!kvm_is_ucontrol(vcpu->kvm))
@@ -3403,6 +3426,8 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
rc = kvm_s390_vcpu_setup(vcpu);
if (rc)
goto out_ucontrol_uninit;
+
+ kvm_s390_sca_set_mtcr(vcpu->kvm);
return 0;
out_ucontrol_uninit:
diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
index 12c464c7cddf..77a692238585 100644
--- a/arch/s390/kvm/priv.c
+++ b/arch/s390/kvm/priv.c
@@ -873,10 +873,13 @@ static int handle_stsi(struct kvm_vcpu *vcpu)
if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE)
return kvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP);
- if (fc > 3) {
- kvm_s390_set_psw_cc(vcpu, 3);
- return 0;
- }
+ /* Bailout forbidden function codes */
+ if (fc > 3 && fc != 15)
+ goto out_no_data;
+
+ /* fc 15 is provided with PTF/CPU topology support */
+ if (fc == 15 && !test_kvm_facility(vcpu->kvm, 11))
+ goto out_no_data;
if (vcpu->run->s.regs.gprs[0] & 0x0fffff00
|| vcpu->run->s.regs.gprs[1] & 0xffff0000)
@@ -910,6 +913,10 @@ static int handle_stsi(struct kvm_vcpu *vcpu)
goto out_no_data;
handle_stsi_3_2_2(vcpu, (void *) mem);
break;
+ case 15:
+ trace_kvm_s390_handle_stsi(vcpu, fc, sel1, sel2, operand2);
+ insert_stsi_usr_data(vcpu, operand2, ar, fc, sel1, sel2);
+ return -EREMOTE;
}
if (kvm_s390_pv_cpu_is_protected(vcpu)) {
memcpy((void *)sida_origin(vcpu->arch.sie_block), (void *)mem,
diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
index dada78b92691..4f4fee697550 100644
--- a/arch/s390/kvm/vsie.c
+++ b/arch/s390/kvm/vsie.c
@@ -503,6 +503,9 @@ static int shadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
/* Host-protection-interruption introduced with ESOP */
if (test_kvm_cpu_feat(vcpu->kvm, KVM_S390_VM_CPU_FEAT_ESOP))
scb_s->ecb |= scb_o->ecb & ECB_HOSTPROTINT;
+ /* CPU Topology */
+ if (test_kvm_facility(vcpu->kvm, 11))
+ scb_s->ecb |= scb_o->ecb & ECB_PTF;
/* transactional execution */
if (test_kvm_facility(vcpu->kvm, 73) && wants_tx) {
/* remap the prefix is tx is toggled on */
--
2.31.1
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 <[email protected]>
> ---
> 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/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index 766028d54a3e..bb54196d4ed6 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -97,15 +97,19 @@ struct bsca_block {
> union ipte_control ipte_control;
> __u64 reserved[5];
> __u64 mcn;
> - __u64 reserved2;
> +#define SCA_UTILITY_MTCR 0x8000
I'm not too happy having this in the bsca but not in the esca. I'd
suggest putting it outside the structs or to go with my next suggestion:
Just make it a bit field struct and make that a member in bsca/esca.
No messing about with ANDing, ORing etc.
It's unfortunate that we only use one bit in that field but I'd still
find it easier to read.
> + __u16 utility;
> + __u8 reserved2[6];
> struct bsca_entry cpu[KVM_S390_BSCA_CPU_SLOTS];
> };
>
> struct esca_block {
> union ipte_control ipte_control;
> - __u64 reserved1[7];
> + __u64 reserved1[6];
> + __u16 utility;
> + __u8 reserved2[6];
> __u64 mcn[4];
> - __u64 reserved2[20];
> + __u64 reserved3[20];
> struct esca_entry cpu[KVM_S390_ESCA_CPU_SLOTS];
> };
>
> @@ -249,6 +253,7 @@ struct kvm_s390_sie_block {
> #define ECB_SPECI 0x08
> #define ECB_SRSI 0x04
> #define ECB_HOSTPROTINT 0x02
> +#define ECB_PTF 0x01
> __u8 ecb; /* 0x0061 */
> #define ECB2_CMMA 0x80
> #define ECB2_IEP 0x20
> 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
> + * @kvm: guest KVM description
> + *
> + * Is only relevant if the topology facility is present,
> + * the caller should check KVM facility 11
I'm not sure that this statement make sense since you set the mctr in
kvm_s390_vcpu_setup() unconditionally and don't check stfle 11.
I think we can remove the second line from this.
> + *
> + * Updates the Multiprocessor Topology-Change-Report to signal
> + * the guest with a topology change.
Please swap those two comments
> + */
> +static void kvm_s390_sca_set_mtcr(struct kvm *kvm)
> +{
> + struct bsca_block *sca = kvm->arch.sca; /* SCA version doesn't matter */
Please put the comment above the statement and maybe extend it a bit:
SCA version doesn't matter, the utility field always has the same offset.
> +
> + ipte_lock(kvm);
> + sca->utility |= SCA_UTILITY_MTCR;
> + ipte_unlock(kvm);
> +}
> +
> static int kvm_s390_vm_set_attr(struct kvm *kvm, struct kvm_device_attr *attr)
> {
> int ret;
> @@ -3143,7 +3162,6 @@ __u64 kvm_s390_get_cpu_timer(struct kvm_vcpu *vcpu)
>
> void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> {
> -
Please remove that change
> gmap_enable(vcpu->arch.enabled_gmap);
> kvm_s390_set_cpuflags(vcpu, CPUSTAT_RUNNING);
> if (vcpu->arch.cputm_enabled && !is_vcpu_idle(vcpu))
> @@ -3272,6 +3290,11 @@ static int kvm_s390_vcpu_setup(struct kvm_vcpu *vcpu)
> vcpu->arch.sie_block->ecb |= ECB_HOSTPROTINT;
> if (test_kvm_facility(vcpu->kvm, 9))
> vcpu->arch.sie_block->ecb |= ECB_SRSI;
> +
> + /* PTF needs guest facilities to enable interpretation */
> + if (test_kvm_facility(vcpu->kvm, 11))
> + vcpu->arch.sie_block->ecb |= ECB_PTF;
> +
> if (test_kvm_facility(vcpu->kvm, 73))
> vcpu->arch.sie_block->ecb |= ECB_TE;
> if (!kvm_is_ucontrol(vcpu->kvm))
> @@ -3403,6 +3426,8 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
> rc = kvm_s390_vcpu_setup(vcpu);
> if (rc)
> goto out_ucontrol_uninit;
> +
> + kvm_s390_sca_set_mtcr(vcpu->kvm);
> return 0;
>
> out_ucontrol_uninit:
> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
> index 12c464c7cddf..77a692238585 100644
> --- a/arch/s390/kvm/priv.c
> +++ b/arch/s390/kvm/priv.c
> @@ -873,10 +873,13 @@ static int handle_stsi(struct kvm_vcpu *vcpu)
> if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE)
> return kvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP);
>
> - if (fc > 3) {
> - kvm_s390_set_psw_cc(vcpu, 3);
> - return 0;
> - }
> + /* Bailout forbidden function codes */
> + if (fc > 3 && fc != 15)
> + goto out_no_data;
> +
> + /* fc 15 is provided with PTF/CPU topology support */
> + if (fc == 15 && !test_kvm_facility(vcpu->kvm, 11))
> + goto out_no_data;
>
> if (vcpu->run->s.regs.gprs[0] & 0x0fffff00
> || vcpu->run->s.regs.gprs[1] & 0xffff0000)
> @@ -910,6 +913,10 @@ static int handle_stsi(struct kvm_vcpu *vcpu)
> goto out_no_data;
> handle_stsi_3_2_2(vcpu, (void *) mem);
> break;
> + case 15:
> + trace_kvm_s390_handle_stsi(vcpu, fc, sel1, sel2, operand2);
> + insert_stsi_usr_data(vcpu, operand2, ar, fc, sel1, sel2);
> + return -EREMOTE;
> }
> if (kvm_s390_pv_cpu_is_protected(vcpu)) {
> memcpy((void *)sida_origin(vcpu->arch.sie_block), (void *)mem,
> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index dada78b92691..4f4fee697550 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
> @@ -503,6 +503,9 @@ static int shadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
> /* Host-protection-interruption introduced with ESOP */
> if (test_kvm_cpu_feat(vcpu->kvm, KVM_S390_VM_CPU_FEAT_ESOP))
> scb_s->ecb |= scb_o->ecb & ECB_HOSTPROTINT;
> + /* CPU Topology */
Maybe also add:
This facility only uses the utility field of the SCA and none of the cpu
entries that are problematic with the other interpretation facilities so
we can pass it through.
> + if (test_kvm_facility(vcpu->kvm, 11))
> + scb_s->ecb |= scb_o->ecb & ECB_PTF;
> /* transactional execution */
> if (test_kvm_facility(vcpu->kvm, 73) && wants_tx) {
> /* remap the prefix is tx is toggled on */
Quoting Pierre Morel (2022-06-20 14:54:36)
[...]
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index 766028d54a3e..bb54196d4ed6 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
[...]
> @@ -3403,6 +3426,8 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
> rc = kvm_s390_vcpu_setup(vcpu);
> if (rc)
> goto out_ucontrol_uninit;
> +
> + kvm_s390_sca_set_mtcr(vcpu->kvm);
We set the MTCR in the vcpu create. Does it also make sense to set it in kvm_arch_vcpu_destroy?
[...]
> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
> index 12c464c7cddf..77a692238585 100644
> --- a/arch/s390/kvm/priv.c
> +++ b/arch/s390/kvm/priv.c
> @@ -873,10 +873,13 @@ static int handle_stsi(struct kvm_vcpu *vcpu)
[...]
> + case 15:
> + trace_kvm_s390_handle_stsi(vcpu, fc, sel1, sel2, operand2);
> + insert_stsi_usr_data(vcpu, operand2, ar, fc, sel1, sel2);
> + return -EREMOTE;
Maybe the API documentation should clearly note that once you turn on KVM_CAP_S390_CPU_TOPOLOGY, you will get exits to userspace for STSI 15.x.y, regardless of whether KVM_CAP_S390_USER_STSI is on or off.
Other than that, looks good, hence:
Reviewed-by: Nico Boehr <[email protected]>
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 <[email protected]>
> ---
> 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
> + * @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)
> +{
> + struct bsca_block *sca = kvm->arch.sca; /* SCA version doesn't matter */
> +
> + ipte_lock(kvm);
Why do we need to take the ipte lock here and in patch 3?
> + sca->utility |= SCA_UTILITY_MTCR;
> + ipte_unlock(kvm);
> +}
[...]
On Mon, 20 Jun 2022 14:54:36 +0200
Pierre Morel <[email protected]> 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 <[email protected]>
> ---
> 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/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index 766028d54a3e..bb54196d4ed6 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -97,15 +97,19 @@ struct bsca_block {
> union ipte_control ipte_control;
> __u64 reserved[5];
> __u64 mcn;
> - __u64 reserved2;
> +#define SCA_UTILITY_MTCR 0x8000
> + __u16 utility;
> + __u8 reserved2[6];
> struct bsca_entry cpu[KVM_S390_BSCA_CPU_SLOTS];
> };
>
> struct esca_block {
> union ipte_control ipte_control;
> - __u64 reserved1[7];
> + __u64 reserved1[6];
> + __u16 utility;
> + __u8 reserved2[6];
> __u64 mcn[4];
> - __u64 reserved2[20];
> + __u64 reserved3[20];
> struct esca_entry cpu[KVM_S390_ESCA_CPU_SLOTS];
> };
>
> @@ -249,6 +253,7 @@ struct kvm_s390_sie_block {
> #define ECB_SPECI 0x08
> #define ECB_SRSI 0x04
> #define ECB_HOSTPROTINT 0x02
> +#define ECB_PTF 0x01
> __u8 ecb; /* 0x0061 */
> #define ECB2_CMMA 0x80
> #define ECB2_IEP 0x20
> 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
the format for kdoc is:
function_name - very short description
please add a very short description. something like:
kvm_s390_sca_set_mtcr - update mtcr to signal topology change
> + * @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)
> +{
> + struct bsca_block *sca = kvm->arch.sca; /* SCA version doesn't matter */
> +
> + ipte_lock(kvm);
> + sca->utility |= SCA_UTILITY_MTCR;
> + ipte_unlock(kvm);
> +}
> +
> static int kvm_s390_vm_set_attr(struct kvm *kvm, struct kvm_device_attr *attr)
> {
> int ret;
> @@ -3143,7 +3162,6 @@ __u64 kvm_s390_get_cpu_timer(struct kvm_vcpu *vcpu)
>
> void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> {
> -
> gmap_enable(vcpu->arch.enabled_gmap);
> kvm_s390_set_cpuflags(vcpu, CPUSTAT_RUNNING);
> if (vcpu->arch.cputm_enabled && !is_vcpu_idle(vcpu))
> @@ -3272,6 +3290,11 @@ static int kvm_s390_vcpu_setup(struct kvm_vcpu *vcpu)
> vcpu->arch.sie_block->ecb |= ECB_HOSTPROTINT;
> if (test_kvm_facility(vcpu->kvm, 9))
> vcpu->arch.sie_block->ecb |= ECB_SRSI;
> +
> + /* PTF needs guest facilities to enable interpretation */
> + if (test_kvm_facility(vcpu->kvm, 11))
> + vcpu->arch.sie_block->ecb |= ECB_PTF;
> +
> if (test_kvm_facility(vcpu->kvm, 73))
> vcpu->arch.sie_block->ecb |= ECB_TE;
> if (!kvm_is_ucontrol(vcpu->kvm))
> @@ -3403,6 +3426,8 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
> rc = kvm_s390_vcpu_setup(vcpu);
> if (rc)
> goto out_ucontrol_uninit;
> +
> + kvm_s390_sca_set_mtcr(vcpu->kvm);
> return 0;
>
> out_ucontrol_uninit:
> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
> index 12c464c7cddf..77a692238585 100644
> --- a/arch/s390/kvm/priv.c
> +++ b/arch/s390/kvm/priv.c
> @@ -873,10 +873,13 @@ static int handle_stsi(struct kvm_vcpu *vcpu)
> if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE)
> return kvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP);
>
> - if (fc > 3) {
> - kvm_s390_set_psw_cc(vcpu, 3);
> - return 0;
> - }
> + /* Bailout forbidden function codes */
> + if (fc > 3 && fc != 15)
> + goto out_no_data;
> +
> + /* fc 15 is provided with PTF/CPU topology support */
> + if (fc == 15 && !test_kvm_facility(vcpu->kvm, 11))
> + goto out_no_data;
>
> if (vcpu->run->s.regs.gprs[0] & 0x0fffff00
> || vcpu->run->s.regs.gprs[1] & 0xffff0000)
> @@ -910,6 +913,10 @@ static int handle_stsi(struct kvm_vcpu *vcpu)
> goto out_no_data;
> handle_stsi_3_2_2(vcpu, (void *) mem);
> break;
> + case 15:
> + trace_kvm_s390_handle_stsi(vcpu, fc, sel1, sel2, operand2);
> + insert_stsi_usr_data(vcpu, operand2, ar, fc, sel1, sel2);
> + return -EREMOTE;
> }
> if (kvm_s390_pv_cpu_is_protected(vcpu)) {
> memcpy((void *)sida_origin(vcpu->arch.sie_block), (void *)mem,
> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index dada78b92691..4f4fee697550 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
> @@ -503,6 +503,9 @@ static int shadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
> /* Host-protection-interruption introduced with ESOP */
> if (test_kvm_cpu_feat(vcpu->kvm, KVM_S390_VM_CPU_FEAT_ESOP))
> scb_s->ecb |= scb_o->ecb & ECB_HOSTPROTINT;
> + /* CPU Topology */
> + if (test_kvm_facility(vcpu->kvm, 11))
> + scb_s->ecb |= scb_o->ecb & ECB_PTF;
> /* transactional execution */
> if (test_kvm_facility(vcpu->kvm, 73) && wants_tx) {
> /* remap the prefix is tx is toggled on */
On 6/24/22 08:56, Nico Boehr wrote:
> Quoting Pierre Morel (2022-06-20 14:54:36)
> [...]
>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>> index 766028d54a3e..bb54196d4ed6 100644
>> --- a/arch/s390/include/asm/kvm_host.h
>> +++ b/arch/s390/include/asm/kvm_host.h
> [...]
>> @@ -3403,6 +3426,8 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>> rc = kvm_s390_vcpu_setup(vcpu);
>> if (rc)
>> goto out_ucontrol_uninit;
>> +
>> + kvm_s390_sca_set_mtcr(vcpu->kvm);
>
> We set the MTCR in the vcpu create. Does it also make sense to set it in kvm_arch_vcpu_destroy?
I think you are right.
Even we only destroy vCPU when we destroy the VM and I think that it is
not currently needed, it would be more logical to do so, and we will be
ready for the day we can unplug vCPUs.
Unless somebody has another opinion I add the entry.
>
> [...]
>> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
>> index 12c464c7cddf..77a692238585 100644
>> --- a/arch/s390/kvm/priv.c
>> +++ b/arch/s390/kvm/priv.c
>> @@ -873,10 +873,13 @@ static int handle_stsi(struct kvm_vcpu *vcpu)
> [...]
>> + case 15:
>> + trace_kvm_s390_handle_stsi(vcpu, fc, sel1, sel2, operand2);
>> + insert_stsi_usr_data(vcpu, operand2, ar, fc, sel1, sel2);
>> + return -EREMOTE;
>
> Maybe the API documentation should clearly note that once you turn on KVM_CAP_S390_CPU_TOPOLOGY, you will get exits to userspace for STSI 15.x.y, regardless of whether KVM_CAP_S390_USER_STSI is on or off.
>
> Other than that, looks good, hence:
>
> Reviewed-by: Nico Boehr <[email protected]>
>
--
Pierre Morel
IBM Lab Boeblingen
On 6/24/22 08:22, Janosch Frank 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 <[email protected]>
>> ---
>> 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/include/asm/kvm_host.h
>> b/arch/s390/include/asm/kvm_host.h
>> index 766028d54a3e..bb54196d4ed6 100644
>> --- a/arch/s390/include/asm/kvm_host.h
>> +++ b/arch/s390/include/asm/kvm_host.h
>> @@ -97,15 +97,19 @@ struct bsca_block {
>> union ipte_control ipte_control;
>> __u64 reserved[5];
>> __u64 mcn;
>> - __u64 reserved2;
>> +#define SCA_UTILITY_MTCR 0x8000
>
> I'm not too happy having this in the bsca but not in the esca. I'd
> suggest putting it outside the structs or to go with my next suggestion:
>
> Just make it a bit field struct and make that a member in bsca/esca.
> No messing about with ANDing, ORing etc.
>
> It's unfortunate that we only use one bit in that field but I'd still
> find it easier to read.
OK
>
>> + __u16 utility;
>> + __u8 reserved2[6];
>> struct bsca_entry cpu[KVM_S390_BSCA_CPU_SLOTS];
>> };
>> struct esca_block {
>> union ipte_control ipte_control;
>> - __u64 reserved1[7];
>> + __u64 reserved1[6];
>> + __u16 utility;
>> + __u8 reserved2[6];
>> __u64 mcn[4];
>> - __u64 reserved2[20];
>> + __u64 reserved3[20];
>> struct esca_entry cpu[KVM_S390_ESCA_CPU_SLOTS];
>> };
>> @@ -249,6 +253,7 @@ struct kvm_s390_sie_block {
>> #define ECB_SPECI 0x08
>> #define ECB_SRSI 0x04
>> #define ECB_HOSTPROTINT 0x02
>> +#define ECB_PTF 0x01
>> __u8 ecb; /* 0x0061 */
>> #define ECB2_CMMA 0x80
>> #define ECB2_IEP 0x20
>> 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
>> + * @kvm: guest KVM description
>> + *
>> + * Is only relevant if the topology facility is present,
>> + * the caller should check KVM facility 11
>
> I'm not sure that this statement make sense since you set the mctr in
> kvm_s390_vcpu_setup() unconditionally and don't check stfle 11.
>
> I think we can remove the second line from this.
>
>> + *
>> + * Updates the Multiprocessor Topology-Change-Report to signal
>> + * the guest with a topology change.
>
> Please swap those two comments
>
>> + */
>> +static void kvm_s390_sca_set_mtcr(struct kvm *kvm)
>> +{
>> + struct bsca_block *sca = kvm->arch.sca; /* SCA version doesn't
>> matter */
>
> Please put the comment above the statement and maybe extend it a bit:
> SCA version doesn't matter, the utility field always has the same offset.
>
>> +
>> + ipte_lock(kvm);
>> + sca->utility |= SCA_UTILITY_MTCR;
>> + ipte_unlock(kvm);
>> +}
>> +
>> static int kvm_s390_vm_set_attr(struct kvm *kvm, struct
>> kvm_device_attr *attr)
>> {
>> int ret;
>> @@ -3143,7 +3162,6 @@ __u64 kvm_s390_get_cpu_timer(struct kvm_vcpu *vcpu)
>> void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>> {
>> -
>
> Please remove that change
>
>> gmap_enable(vcpu->arch.enabled_gmap);
>> kvm_s390_set_cpuflags(vcpu, CPUSTAT_RUNNING);
>> if (vcpu->arch.cputm_enabled && !is_vcpu_idle(vcpu))
>> @@ -3272,6 +3290,11 @@ static int kvm_s390_vcpu_setup(struct kvm_vcpu
>> *vcpu)
>> vcpu->arch.sie_block->ecb |= ECB_HOSTPROTINT;
>> if (test_kvm_facility(vcpu->kvm, 9))
>> vcpu->arch.sie_block->ecb |= ECB_SRSI;
>> +
>> + /* PTF needs guest facilities to enable interpretation */
>> + if (test_kvm_facility(vcpu->kvm, 11))
>> + vcpu->arch.sie_block->ecb |= ECB_PTF;
>> +
>> if (test_kvm_facility(vcpu->kvm, 73))
>> vcpu->arch.sie_block->ecb |= ECB_TE;
>> if (!kvm_is_ucontrol(vcpu->kvm))
>> @@ -3403,6 +3426,8 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>> rc = kvm_s390_vcpu_setup(vcpu);
>> if (rc)
>> goto out_ucontrol_uninit;
>> +
>> + kvm_s390_sca_set_mtcr(vcpu->kvm);
>> return 0;
>> out_ucontrol_uninit:
>> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
>> index 12c464c7cddf..77a692238585 100644
>> --- a/arch/s390/kvm/priv.c
>> +++ b/arch/s390/kvm/priv.c
>> @@ -873,10 +873,13 @@ static int handle_stsi(struct kvm_vcpu *vcpu)
>> if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE)
>> return kvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP);
>> - if (fc > 3) {
>> - kvm_s390_set_psw_cc(vcpu, 3);
>> - return 0;
>> - }
>> + /* Bailout forbidden function codes */
>> + if (fc > 3 && fc != 15)
>> + goto out_no_data;
>> +
>> + /* fc 15 is provided with PTF/CPU topology support */
>> + if (fc == 15 && !test_kvm_facility(vcpu->kvm, 11))
>> + goto out_no_data;
>> if (vcpu->run->s.regs.gprs[0] & 0x0fffff00
>> || vcpu->run->s.regs.gprs[1] & 0xffff0000)
>> @@ -910,6 +913,10 @@ static int handle_stsi(struct kvm_vcpu *vcpu)
>> goto out_no_data;
>> handle_stsi_3_2_2(vcpu, (void *) mem);
>> break;
>> + case 15:
>> + trace_kvm_s390_handle_stsi(vcpu, fc, sel1, sel2, operand2);
>> + insert_stsi_usr_data(vcpu, operand2, ar, fc, sel1, sel2);
>> + return -EREMOTE;
>> }
>> if (kvm_s390_pv_cpu_is_protected(vcpu)) {
>> memcpy((void *)sida_origin(vcpu->arch.sie_block), (void *)mem,
>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>> index dada78b92691..4f4fee697550 100644
>> --- a/arch/s390/kvm/vsie.c
>> +++ b/arch/s390/kvm/vsie.c
>> @@ -503,6 +503,9 @@ static int shadow_scb(struct kvm_vcpu *vcpu,
>> struct vsie_page *vsie_page)
>> /* Host-protection-interruption introduced with ESOP */
>> if (test_kvm_cpu_feat(vcpu->kvm, KVM_S390_VM_CPU_FEAT_ESOP))
>> scb_s->ecb |= scb_o->ecb & ECB_HOSTPROTINT;
>> + /* CPU Topology */
>
> Maybe also add:
> This facility only uses the utility field of the SCA and none of the cpu
> entries that are problematic with the other interpretation facilities so
> we can pass it through.
>
>> + if (test_kvm_facility(vcpu->kvm, 11))
>> + scb_s->ecb |= scb_o->ecb & ECB_PTF;
>> /* transactional execution */
>> if (test_kvm_facility(vcpu->kvm, 73) && wants_tx) {
>> /* remap the prefix is tx is toggled on */
>
OK with all comments,
I make the changes.
Thanks,
Pierre
--
Pierre Morel
IBM Lab Boeblingen
On 6/24/22 17:09, 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 <[email protected]>
>> ---
>> 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
>> + * @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)
>> +{
>> + struct bsca_block *sca = kvm->arch.sca; /* SCA version doesn't matter */
>> +
>> + ipte_lock(kvm);
>
> Why do we need to take the ipte lock here and in patch 3?
That is a good question.
I fear I was tired as I understood that from the documentation, after
re-reading, we need an interlocked-update not an ipte lock update.
... I have to change that
>
>> + sca->utility |= SCA_UTILITY_MTCR;
>> + ipte_unlock(kvm);
>> +}
>
> [...]
>
--
Pierre Morel
IBM Lab Boeblingen
On 6/24/22 11:32, Claudio Imbrenda wrote:
> On Mon, 20 Jun 2022 14:54:36 +0200
> Pierre Morel <[email protected]> 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 <[email protected]>
>> ---
>> 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/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>> index 766028d54a3e..bb54196d4ed6 100644
>> --- a/arch/s390/include/asm/kvm_host.h
>> +++ b/arch/s390/include/asm/kvm_host.h
>> @@ -97,15 +97,19 @@ struct bsca_block {
>> union ipte_control ipte_control;
>> __u64 reserved[5];
>> __u64 mcn;
>> - __u64 reserved2;
>> +#define SCA_UTILITY_MTCR 0x8000
>> + __u16 utility;
>> + __u8 reserved2[6];
>> struct bsca_entry cpu[KVM_S390_BSCA_CPU_SLOTS];
>> };
>>
>> struct esca_block {
>> union ipte_control ipte_control;
>> - __u64 reserved1[7];
>> + __u64 reserved1[6];
>> + __u16 utility;
>> + __u8 reserved2[6];
>> __u64 mcn[4];
>> - __u64 reserved2[20];
>> + __u64 reserved3[20];
>> struct esca_entry cpu[KVM_S390_ESCA_CPU_SLOTS];
>> };
>>
>> @@ -249,6 +253,7 @@ struct kvm_s390_sie_block {
>> #define ECB_SPECI 0x08
>> #define ECB_SRSI 0x04
>> #define ECB_HOSTPROTINT 0x02
>> +#define ECB_PTF 0x01
>> __u8 ecb; /* 0x0061 */
>> #define ECB2_CMMA 0x80
>> #define ECB2_IEP 0x20
>> 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
>
> the format for kdoc is:
>
> function_name - very short description
>
> please add a very short description. something like:
>
> kvm_s390_sca_set_mtcr - update mtcr to signal topology change
OK, thanks,
Pierre
--
Pierre Morel
IBM Lab Boeblingen
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 <[email protected]>
> ---
> 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.
> + struct bsca_block *sca = kvm->arch.sca; /* SCA version doesn't matter */
> +
> + ipte_lock(kvm);
> + sca->utility |= SCA_UTILITY_MTCR;
> + ipte_unlock(kvm);
> +}
> +
[...]
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 <[email protected]>
>> ---
>> 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?
OK, for me, with a good name I can suppress a comment.
--
Pierre Morel
IBM Lab Boeblingen
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 <[email protected]>
>> ---
>> 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.
>
>> + 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
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 <[email protected]>
>>> ---
>>> 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.
>
>>
>>> + struct bsca_block *sca = kvm->arch.sca; /* SCA version doesn't matter */
>>> +
>>> + ipte_lock(kvm);
>>> + sca->utility |= SCA_UTILITY_MTCR;
>>> + ipte_unlock(kvm);
>>> +}
>>> +
>>
>> [...]
>>
>
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 <[email protected]>
>>>> ---
>>>> 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
On 6/24/22 08:56, Nico Boehr wrote:
> Quoting Pierre Morel (2022-06-20 14:54:36)
> [...]
>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>> index 766028d54a3e..bb54196d4ed6 100644
>> --- a/arch/s390/include/asm/kvm_host.h
>> +++ b/arch/s390/include/asm/kvm_host.h
> [...]
>> @@ -3403,6 +3426,8 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>> rc = kvm_s390_vcpu_setup(vcpu);
>> if (rc)
>> goto out_ucontrol_uninit;
>> +
>> + kvm_s390_sca_set_mtcr(vcpu->kvm);
>
> We set the MTCR in the vcpu create. Does it also make sense to set it in kvm_arch_vcpu_destroy?
>
> [...]
>> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
>> index 12c464c7cddf..77a692238585 100644
>> --- a/arch/s390/kvm/priv.c
>> +++ b/arch/s390/kvm/priv.c
>> @@ -873,10 +873,13 @@ static int handle_stsi(struct kvm_vcpu *vcpu)
> [...]
>> + case 15:
>> + trace_kvm_s390_handle_stsi(vcpu, fc, sel1, sel2, operand2);
>> + insert_stsi_usr_data(vcpu, operand2, ar, fc, sel1, sel2);
>> + return -EREMOTE;
>
> Maybe the API documentation should clearly note that once you turn on KVM_CAP_S390_CPU_TOPOLOGY, you will get exits to userspace for STSI 15.x.y, regardless of whether KVM_CAP_S390_USER_STSI is on or off.
Humm, right, I must change this, it is not right to have STSI(15) but
not STSI([123])
I will also have two other changes in this patch, the locking and add a
check for PV on STSI(15)
So I will respin in a short time.
>
> Other than that, looks good, hence:
>
> Reviewed-by: Nico Boehr <[email protected]>
>
--
Pierre Morel
IBM Lab Boeblingen
On 6/28/22 16:13, Pierre Morel wrote:
>
>
> 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 <[email protected]>
>>>>> ---
>>>>> 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.
Ah, ok you want to keep the bitfield.
[...]
>
> static void kvm_s390_sca_set_mtcr(struct kvm *kvm, int val)
If you use a bool val you can simply do new.mtcr = val; below.
> {
> struct bsca_block *sca = kvm->arch.sca;
> union sca_utility new, old;
>
> read_lock(&kvm->arch.sca_lock);
Don't forget to move the sca = kvm->arch.sca; under the lock here.
> 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);
>>>>> +}
>>>>> +
>>>>
>>>> [...]
>>>>
>>>
>>
>
On 6/28/22 17:01, Janis Schoetterl-Glausch wrote:
> On 6/28/22 16:13, Pierre Morel wrote:
>>
>>
>> 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 <[email protected]>
>>>>>> ---
>>>>>> 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.
>
> Ah, ok you want to keep the bitfield.
>
> [...]
>>
>> static void kvm_s390_sca_set_mtcr(struct kvm *kvm, int val)
>
> If you use a bool val you can simply do new.mtcr = val; below.
>> {
>> struct bsca_block *sca = kvm->arch.sca;
>> union sca_utility new, old;
>>
>> read_lock(&kvm->arch.sca_lock);
>
> Don't forget to move the sca = kvm->arch.sca; under the lock here.
>> 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);
>> }
right, thanks, and to unlock at the end :)
>>
>>>>
>>>>>
>>>>>> + 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