When the host supports the CPU topology facility, the PTF
instruction with function code 2 is interpreted by the SIE,
provided that the userland hypervizor activates the interpretation
by using the KVM_CAP_S390_CPU_TOPOLOGY KVM extension.
The PTF instructions with function code 0 and 1 are intercepted
and must be emulated by the userland hypervizor.
During RESET all CPU of the configuration are placed in
horizontal polarity.
Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
include/hw/s390x/cpu-topology.h | 3 +
include/hw/s390x/s390-virtio-ccw.h | 6 ++
target/s390x/cpu.h | 1 +
hw/s390x/cpu-topology.c | 92 ++++++++++++++++++++++++++++++
target/s390x/cpu-sysemu.c | 16 ++++++
target/s390x/kvm/kvm.c | 11 ++++
6 files changed, 129 insertions(+)
diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
index 9571aa70e5..33e23d78b9 100644
--- a/include/hw/s390x/cpu-topology.h
+++ b/include/hw/s390x/cpu-topology.h
@@ -55,11 +55,13 @@ typedef struct S390Topology {
QTAILQ_HEAD(, S390TopologyEntry) list;
uint8_t *sockets;
CpuTopology *smp;
+ int polarity;
} S390Topology;
#ifdef CONFIG_KVM
bool s390_has_topology(void);
void s390_topology_set_cpu(MachineState *ms, S390CPU *cpu, Error **errp);
+void s390_topology_set_polarity(int polarity);
#else
static inline bool s390_has_topology(void)
{
@@ -68,6 +70,7 @@ static inline bool s390_has_topology(void)
static inline void s390_topology_set_cpu(MachineState *ms,
S390CPU *cpu,
Error **errp) {}
+static inline void s390_topology_set_polarity(int polarity) {}
#endif
extern S390Topology s390_topology;
diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
index 9bba21a916..c1d46e78af 100644
--- a/include/hw/s390x/s390-virtio-ccw.h
+++ b/include/hw/s390x/s390-virtio-ccw.h
@@ -30,6 +30,12 @@ struct S390CcwMachineState {
uint8_t loadparm[8];
};
+#define S390_PTF_REASON_NONE (0x00 << 8)
+#define S390_PTF_REASON_DONE (0x01 << 8)
+#define S390_PTF_REASON_BUSY (0x02 << 8)
+#define S390_TOPO_FC_MASK 0xffUL
+void s390_handle_ptf(S390CPU *cpu, uint8_t r1, uintptr_t ra);
+
struct S390CcwMachineClass {
/*< private >*/
MachineClass parent_class;
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 01ade07009..5da4041576 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -864,6 +864,7 @@ void s390_do_cpu_set_diag318(CPUState *cs, run_on_cpu_data arg);
int s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch_id,
int vq, bool assign);
void s390_cpu_topology_reset(void);
+void s390_cpu_topology_set(void);
#ifndef CONFIG_USER_ONLY
unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu);
#else
diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
index 438055c612..e6b4692581 100644
--- a/hw/s390x/cpu-topology.c
+++ b/hw/s390x/cpu-topology.c
@@ -97,6 +97,98 @@ static s390_topology_id s390_topology_from_cpu(S390CPU *cpu)
}
/**
+ * s390_topology_set_polarity
+ * @polarity: horizontal or vertical
+ *
+ * Changes the polarity of all the CPU in the configuration.
+ *
+ * If the dedicated CPU modifier attribute is set a vertical
+ * polarization is always high (Architecture).
+ * Otherwise we decide to set it as medium.
+ *
+ * Once done, advertise a topology change.
+ */
+void s390_topology_set_polarity(int polarity)
+{
+ S390TopologyEntry *entry;
+
+ QTAILQ_FOREACH(entry, &s390_topology.list, next) {
+ if (polarity == S390_TOPOLOGY_POLARITY_HORIZONTAL) {
+ entry->id.p = polarity;
+ } else {
+ if (entry->id.d) {
+ entry->id.p = S390_TOPOLOGY_POLARITY_VERTICAL_HIGH;
+ } else {
+ entry->id.p = S390_TOPOLOGY_POLARITY_VERTICAL_MEDIUM;
+ }
+ }
+ }
+ s390_cpu_topology_set();
+}
+
+/*
+ * s390_handle_ptf:
+ *
+ * @register 1: contains the function code
+ *
+ * Function codes 0 and 1 handle the CPU polarization.
+ * We assume an horizontal topology, the only one supported currently
+ * by Linux, consequently we answer to function code 0, requesting
+ * horizontal polarization that it is already the current polarization
+ * and reject vertical polarization request without further explanation.
+ *
+ * Function code 2 is handling topology changes and is interpreted
+ * by the SIE.
+ */
+void s390_handle_ptf(S390CPU *cpu, uint8_t r1, uintptr_t ra)
+{
+ CPUS390XState *env = &cpu->env;
+ uint64_t reg = env->regs[r1];
+ uint8_t fc = reg & S390_TOPO_FC_MASK;
+
+ if (!s390_has_feat(S390_FEAT_CONFIGURATION_TOPOLOGY)) {
+ s390_program_interrupt(env, PGM_OPERATION, ra);
+ return;
+ }
+
+ if (env->psw.mask & PSW_MASK_PSTATE) {
+ s390_program_interrupt(env, PGM_PRIVILEGED, ra);
+ return;
+ }
+
+ if (reg & ~S390_TOPO_FC_MASK) {
+ s390_program_interrupt(env, PGM_SPECIFICATION, ra);
+ return;
+ }
+
+ switch (fc) {
+ case 0: /* Horizontal polarization is already set */
+ if (s390_topology.polarity == S390_TOPOLOGY_POLARITY_HORIZONTAL) {
+ env->regs[r1] |= S390_PTF_REASON_DONE;
+ setcc(cpu, 2);
+ } else {
+ s390_topology_set_polarity(S390_TOPOLOGY_POLARITY_HORIZONTAL);
+ s390_topology.polarity = S390_TOPOLOGY_POLARITY_HORIZONTAL;
+ setcc(cpu, 0);
+ }
+ break;
+ case 1: /* Vertical polarization is not supported */
+ if (s390_topology.polarity != S390_TOPOLOGY_POLARITY_HORIZONTAL) {
+ env->regs[r1] |= S390_PTF_REASON_DONE;
+ setcc(cpu, 2);
+ } else {
+ s390_topology_set_polarity(S390_TOPOLOGY_POLARITY_VERTICAL_LOW);
+ s390_topology.polarity = S390_TOPOLOGY_POLARITY_VERTICAL_LOW;
+ setcc(cpu, 0);
+ }
+ break;
+ default:
+ /* Note that fc == 2 is interpreted by the SIE */
+ s390_program_interrupt(env, PGM_SPECIFICATION, ra);
+ }
+}
+
+ /**
* s390_topology_set_entry:
* @entry: Topology entry to setup
* @id: topology id to use for the setup
diff --git a/target/s390x/cpu-sysemu.c b/target/s390x/cpu-sysemu.c
index e27864c5f5..8643320947 100644
--- a/target/s390x/cpu-sysemu.c
+++ b/target/s390x/cpu-sysemu.c
@@ -37,6 +37,7 @@
#include "sysemu/sysemu.h"
#include "sysemu/tcg.h"
#include "hw/core/sysemu-cpu-ops.h"
+#include "hw/s390x/cpu-topology.h"
/* S390CPUClass::load_normal() */
static void s390_cpu_load_normal(CPUState *s)
@@ -311,6 +312,8 @@ void s390_cpu_topology_reset(void)
{
int ret;
+ s390_topology_set_polarity(S390_TOPOLOGY_POLARITY_HORIZONTAL);
+
if (kvm_enabled()) {
ret = kvm_s390_topology_set_mtcr(0);
if (ret) {
@@ -319,3 +322,16 @@ void s390_cpu_topology_reset(void)
}
}
}
+
+void s390_cpu_topology_set(void)
+{
+ int ret;
+
+ if (kvm_enabled()) {
+ ret = kvm_s390_topology_set_mtcr(1);
+ if (ret) {
+ error_report("Failed to set Modified Topology Change Report: %s",
+ strerror(-ret));
+ }
+ }
+}
diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index bc953151ce..fb63be41b7 100644
--- a/target/s390x/kvm/kvm.c
+++ b/target/s390x/kvm/kvm.c
@@ -96,6 +96,7 @@
#define PRIV_B9_EQBS 0x9c
#define PRIV_B9_CLP 0xa0
+#define PRIV_B9_PTF 0xa2
#define PRIV_B9_PCISTG 0xd0
#define PRIV_B9_PCILG 0xd2
#define PRIV_B9_RPCIT 0xd3
@@ -1464,6 +1465,13 @@ static int kvm_mpcifc_service_call(S390CPU *cpu, struct kvm_run *run)
}
}
+static void kvm_handle_ptf(S390CPU *cpu, struct kvm_run *run)
+{
+ uint8_t r1 = (run->s390_sieic.ipb >> 20) & 0x0f;
+
+ s390_handle_ptf(cpu, r1, RA_IGNORED);
+}
+
static int handle_b9(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1)
{
int r = 0;
@@ -1481,6 +1489,9 @@ static int handle_b9(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1)
case PRIV_B9_RPCIT:
r = kvm_rpcit_service_call(cpu, run);
break;
+ case PRIV_B9_PTF:
+ kvm_handle_ptf(cpu, run);
+ break;
case PRIV_B9_EQBS:
/* just inject exception */
r = -1;
--
2.31.1On Thu, 2023-01-05 at 15:53 +0100, Pierre Morel wrote:
> When the host supports the CPU topology facility, the PTF
> instruction with function code 2 is interpreted by the SIE,
> provided that the userland hypervizor activates the interpretation
> by using the KVM_CAP_S390_CPU_TOPOLOGY KVM extension.
>
> The PTF instructions with function code 0 and 1 are intercepted
> and must be emulated by the userland hypervizor.
>
> During RESET all CPU of the configuration are placed in
> horizontal polarity.
>
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
> include/hw/s390x/cpu-topology.h | 3 +
> include/hw/s390x/s390-virtio-ccw.h | 6 ++
> target/s390x/cpu.h | 1 +
> hw/s390x/cpu-topology.c | 92 ++++++++++++++++++++++++++++++
> target/s390x/cpu-sysemu.c | 16 ++++++
> target/s390x/kvm/kvm.c | 11 ++++
> 6 files changed, 129 insertions(+)
>
> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
> index 9571aa70e5..33e23d78b9 100644
> --- a/include/hw/s390x/cpu-topology.h
> +++ b/include/hw/s390x/cpu-topology.h
> @@ -55,11 +55,13 @@ typedef struct S390Topology {
> QTAILQ_HEAD(, S390TopologyEntry) list;
> uint8_t *sockets;
> CpuTopology *smp;
> + int polarity;
> } S390Topology;
>
> #ifdef CONFIG_KVM
> bool s390_has_topology(void);
> void s390_topology_set_cpu(MachineState *ms, S390CPU *cpu, Error **errp);
> +void s390_topology_set_polarity(int polarity);
> #else
> static inline bool s390_has_topology(void)
> {
> @@ -68,6 +70,7 @@ static inline bool s390_has_topology(void)
> static inline void s390_topology_set_cpu(MachineState *ms,
> S390CPU *cpu,
> Error **errp) {}
> +static inline void s390_topology_set_polarity(int polarity) {}
> #endif
> extern S390Topology s390_topology;
>
> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
> index 9bba21a916..c1d46e78af 100644
> --- a/include/hw/s390x/s390-virtio-ccw.h
> +++ b/include/hw/s390x/s390-virtio-ccw.h
> @@ -30,6 +30,12 @@ struct S390CcwMachineState {
> uint8_t loadparm[8];
> };
>
> +#define S390_PTF_REASON_NONE (0x00 << 8)
> +#define S390_PTF_REASON_DONE (0x01 << 8)
> +#define S390_PTF_REASON_BUSY (0x02 << 8)
> +#define S390_TOPO_FC_MASK 0xffUL
> +void s390_handle_ptf(S390CPU *cpu, uint8_t r1, uintptr_t ra);
> +
> struct S390CcwMachineClass {
> /*< private >*/
> MachineClass parent_class;
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 01ade07009..5da4041576 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -864,6 +864,7 @@ void s390_do_cpu_set_diag318(CPUState *cs, run_on_cpu_data arg);
> int s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch_id,
> int vq, bool assign);
> void s390_cpu_topology_reset(void);
> +void s390_cpu_topology_set(void);
I don't like this name much, it's nondescript.
s390_cpu_topology_set_modified ?
> #ifndef CONFIG_USER_ONLY
> unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu);
> #else
> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> index 438055c612..e6b4692581 100644
> --- a/hw/s390x/cpu-topology.c
> +++ b/hw/s390x/cpu-topology.c
> @@ -97,6 +97,98 @@ static s390_topology_id s390_topology_from_cpu(S390CPU *cpu)
> }
>
> /**
> + * s390_topology_set_polarity
> + * @polarity: horizontal or vertical
> + *
> + * Changes the polarity of all the CPU in the configuration.
> + *
> + * If the dedicated CPU modifier attribute is set a vertical
> + * polarization is always high (Architecture).
> + * Otherwise we decide to set it as medium.
> + *
> + * Once done, advertise a topology change.
> + */
> +void s390_topology_set_polarity(int polarity)
I don't like that this function ignores what kind of vertical polarization is passed,
it's confusing.
That seems like a further reason to split horizontal/vertical from the entitlement.
> +{
> + S390TopologyEntry *entry;
I also expected this function to set s390_topology.polarization, but it doesn't.
> +
> + QTAILQ_FOREACH(entry, &s390_topology.list, next) {
> + if (polarity == S390_TOPOLOGY_POLARITY_HORIZONTAL) {
> + entry->id.p = polarity;
> + } else {
> + if (entry->id.d) {
> + entry->id.p = S390_TOPOLOGY_POLARITY_VERTICAL_HIGH;
> + } else {
> + entry->id.p = S390_TOPOLOGY_POLARITY_VERTICAL_MEDIUM;
> + }
> + }
> + }
> + s390_cpu_topology_set();
> +}
> +
> +/*
> + * s390_handle_ptf:
> + *
> + * @register 1: contains the function code
> + *
> + * Function codes 0 and 1 handle the CPU polarization.
> + * We assume an horizontal topology, the only one supported currently
> + * by Linux, consequently we answer to function code 0, requesting
> + * horizontal polarization that it is already the current polarization
> + * and reject vertical polarization request without further explanation.
This comment is outdated, right? Same for those in the function body.
> + *
> + * Function code 2 is handling topology changes and is interpreted
> + * by the SIE.
> + */
> +void s390_handle_ptf(S390CPU *cpu, uint8_t r1, uintptr_t ra)
> +{
> + CPUS390XState *env = &cpu->env;
> + uint64_t reg = env->regs[r1];
> + uint8_t fc = reg & S390_TOPO_FC_MASK;
> +
> + if (!s390_has_feat(S390_FEAT_CONFIGURATION_TOPOLOGY)) {
> + s390_program_interrupt(env, PGM_OPERATION, ra);
> + return;
> + }
> +
> + if (env->psw.mask & PSW_MASK_PSTATE) {
> + s390_program_interrupt(env, PGM_PRIVILEGED, ra);
> + return;
> + }
> +
> + if (reg & ~S390_TOPO_FC_MASK) {
> + s390_program_interrupt(env, PGM_SPECIFICATION, ra);
> + return;
> + }
> +
> + switch (fc) {
> + case 0: /* Horizontal polarization is already set */
> + if (s390_topology.polarity == S390_TOPOLOGY_POLARITY_HORIZONTAL) {
> + env->regs[r1] |= S390_PTF_REASON_DONE;
> + setcc(cpu, 2);
> + } else {
> + s390_topology_set_polarity(S390_TOPOLOGY_POLARITY_HORIZONTAL);
> + s390_topology.polarity = S390_TOPOLOGY_POLARITY_HORIZONTAL;
> + setcc(cpu, 0);
> + }
> + break;
> + case 1: /* Vertical polarization is not supported */
> + if (s390_topology.polarity != S390_TOPOLOGY_POLARITY_HORIZONTAL) {
> + env->regs[r1] |= S390_PTF_REASON_DONE;
> + setcc(cpu, 2);
> + } else {
> + s390_topology_set_polarity(S390_TOPOLOGY_POLARITY_VERTICAL_LOW);
This is why I said it's confusing, nothing gets set to LOW.
> + s390_topology.polarity = S390_TOPOLOGY_POLARITY_VERTICAL_LOW;
Why LOW here?
> + setcc(cpu, 0);
> + }
> + break;
> + default:
> + /* Note that fc == 2 is interpreted by the SIE */
> + s390_program_interrupt(env, PGM_SPECIFICATION, ra);
> + }
You can simplify this by doing:
int new_polarity;
switch (fc) {
case 0:
new_polarity = S390_TOPOLOGY_POLARITY_HORIZONTAL;
break;
case 1:
new_polarity = S390_TOPOLOGY_POLARITY_VERTICAL_?;
break;
default:
/* Note that fc == 2 is interpreted by the SIE */
s390_program_interrupt(env, PGM_SPECIFICATION, ra);
return;
}
if same polarity:
rc done, rejected
else
set polarity, initiated
Might be a good idea to turn the polarity values into an enum.
> +}
[...]
On 1/16/23 19:24, Nina Schoetterl-Glausch wrote:
> On Thu, 2023-01-05 at 15:53 +0100, Pierre Morel wrote:
>> When the host supports the CPU topology facility, the PTF
>> instruction with function code 2 is interpreted by the SIE,
>> provided that the userland hypervizor activates the interpretation
>> by using the KVM_CAP_S390_CPU_TOPOLOGY KVM extension.
>>
>> The PTF instructions with function code 0 and 1 are intercepted
>> and must be emulated by the userland hypervizor.
>>
>> During RESET all CPU of the configuration are placed in
>> horizontal polarity.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>> include/hw/s390x/cpu-topology.h | 3 +
>> include/hw/s390x/s390-virtio-ccw.h | 6 ++
>> target/s390x/cpu.h | 1 +
>> hw/s390x/cpu-topology.c | 92 ++++++++++++++++++++++++++++++
>> target/s390x/cpu-sysemu.c | 16 ++++++
>> target/s390x/kvm/kvm.c | 11 ++++
>> 6 files changed, 129 insertions(+)
>>
>> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
>> index 9571aa70e5..33e23d78b9 100644
>> --- a/include/hw/s390x/cpu-topology.h
>> +++ b/include/hw/s390x/cpu-topology.h
>> @@ -55,11 +55,13 @@ typedef struct S390Topology {
>> QTAILQ_HEAD(, S390TopologyEntry) list;
>> uint8_t *sockets;
>> CpuTopology *smp;
>> + int polarity;
>> } S390Topology;
>>
>> #ifdef CONFIG_KVM
>> bool s390_has_topology(void);
>> void s390_topology_set_cpu(MachineState *ms, S390CPU *cpu, Error **errp);
>> +void s390_topology_set_polarity(int polarity);
>> #else
>> static inline bool s390_has_topology(void)
>> {
>> @@ -68,6 +70,7 @@ static inline bool s390_has_topology(void)
>> static inline void s390_topology_set_cpu(MachineState *ms,
>> S390CPU *cpu,
>> Error **errp) {}
>> +static inline void s390_topology_set_polarity(int polarity) {}
>> #endif
>> extern S390Topology s390_topology;
>>
>> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
>> index 9bba21a916..c1d46e78af 100644
>> --- a/include/hw/s390x/s390-virtio-ccw.h
>> +++ b/include/hw/s390x/s390-virtio-ccw.h
>> @@ -30,6 +30,12 @@ struct S390CcwMachineState {
>> uint8_t loadparm[8];
>> };
>>
>> +#define S390_PTF_REASON_NONE (0x00 << 8)
>> +#define S390_PTF_REASON_DONE (0x01 << 8)
>> +#define S390_PTF_REASON_BUSY (0x02 << 8)
>> +#define S390_TOPO_FC_MASK 0xffUL
>> +void s390_handle_ptf(S390CPU *cpu, uint8_t r1, uintptr_t ra);
>> +
>> struct S390CcwMachineClass {
>> /*< private >*/
>> MachineClass parent_class;
>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>> index 01ade07009..5da4041576 100644
>> --- a/target/s390x/cpu.h
>> +++ b/target/s390x/cpu.h
>> @@ -864,6 +864,7 @@ void s390_do_cpu_set_diag318(CPUState *cs, run_on_cpu_data arg);
>> int s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch_id,
>> int vq, bool assign);
>> void s390_cpu_topology_reset(void);
>> +void s390_cpu_topology_set(void);
>
> I don't like this name much, it's nondescript.
> s390_cpu_topology_set_modified ?
>
>> #ifndef CONFIG_USER_ONLY
>> unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu);
>> #else
>> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
>> index 438055c612..e6b4692581 100644
>> --- a/hw/s390x/cpu-topology.c
>> +++ b/hw/s390x/cpu-topology.c
>> @@ -97,6 +97,98 @@ static s390_topology_id s390_topology_from_cpu(S390CPU *cpu)
>> }
>>
>> /**
>> + * s390_topology_set_polarity
>> + * @polarity: horizontal or vertical
>> + *
>> + * Changes the polarity of all the CPU in the configuration.
>> + *
>> + * If the dedicated CPU modifier attribute is set a vertical
>> + * polarization is always high (Architecture).
>> + * Otherwise we decide to set it as medium.
>> + *
>> + * Once done, advertise a topology change.
>> + */
>> +void s390_topology_set_polarity(int polarity)
>
> I don't like that this function ignores what kind of vertical polarization is passed,
> it's confusing.
> That seems like a further reason to split horizontal/vertical from the entitlement.
>
Yes.
I will also make the ordering of the TLE at the last moment instead of
doing it during hotplug or QAPI command.
This is something Cedric proposed some time ago and I think it is right,
it will be easier to do it once in STSI than on every change of topology,
hotplug, QAPI and polarization change with PTF.
Regards,
Pierre
--
Pierre Morel
IBM Lab Boeblingen
On 1/16/23 19:24, Nina Schoetterl-Glausch wrote:
> On Thu, 2023-01-05 at 15:53 +0100, Pierre Morel wrote:
>> When the host supports the CPU topology facility, the PTF
>> instruction with function code 2 is interpreted by the SIE,
>> provided that the userland hypervizor activates the interpretation
>> by using the KVM_CAP_S390_CPU_TOPOLOGY KVM extension.
>>
>> The PTF instructions with function code 0 and 1 are intercepted
>> and must be emulated by the userland hypervizor.
>>
>> During RESET all CPU of the configuration are placed in
>> horizontal polarity.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>> include/hw/s390x/cpu-topology.h | 3 +
>> include/hw/s390x/s390-virtio-ccw.h | 6 ++
>> target/s390x/cpu.h | 1 +
>> hw/s390x/cpu-topology.c | 92 ++++++++++++++++++++++++++++++
>> target/s390x/cpu-sysemu.c | 16 ++++++
>> target/s390x/kvm/kvm.c | 11 ++++
>> 6 files changed, 129 insertions(+)
>>
>> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
>> index 9571aa70e5..33e23d78b9 100644
>> --- a/include/hw/s390x/cpu-topology.h
>> +++ b/include/hw/s390x/cpu-topology.h
>> @@ -55,11 +55,13 @@ typedef struct S390Topology {
>> QTAILQ_HEAD(, S390TopologyEntry) list;
>> uint8_t *sockets;
>> CpuTopology *smp;
>> + int polarity;
>> } S390Topology;
>>
>> #ifdef CONFIG_KVM
>> bool s390_has_topology(void);
>> void s390_topology_set_cpu(MachineState *ms, S390CPU *cpu, Error **errp);
>> +void s390_topology_set_polarity(int polarity);
>> #else
>> static inline bool s390_has_topology(void)
>> {
>> @@ -68,6 +70,7 @@ static inline bool s390_has_topology(void)
>> static inline void s390_topology_set_cpu(MachineState *ms,
>> S390CPU *cpu,
>> Error **errp) {}
>> +static inline void s390_topology_set_polarity(int polarity) {}
>> #endif
>> extern S390Topology s390_topology;
>>
>> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
>> index 9bba21a916..c1d46e78af 100644
>> --- a/include/hw/s390x/s390-virtio-ccw.h
>> +++ b/include/hw/s390x/s390-virtio-ccw.h
>> @@ -30,6 +30,12 @@ struct S390CcwMachineState {
>> uint8_t loadparm[8];
>> };
>>
>> +#define S390_PTF_REASON_NONE (0x00 << 8)
>> +#define S390_PTF_REASON_DONE (0x01 << 8)
>> +#define S390_PTF_REASON_BUSY (0x02 << 8)
>> +#define S390_TOPO_FC_MASK 0xffUL
>> +void s390_handle_ptf(S390CPU *cpu, uint8_t r1, uintptr_t ra);
>> +
>> struct S390CcwMachineClass {
>> /*< private >*/
>> MachineClass parent_class;
>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>> index 01ade07009..5da4041576 100644
>> --- a/target/s390x/cpu.h
>> +++ b/target/s390x/cpu.h
>> @@ -864,6 +864,7 @@ void s390_do_cpu_set_diag318(CPUState *cs, run_on_cpu_data arg);
>> int s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch_id,
>> int vq, bool assign);
>> void s390_cpu_topology_reset(void);
>> +void s390_cpu_topology_set(void);
>
> I don't like this name much, it's nondescript.
> s390_cpu_topology_set_modified ?
yes, better.
>
>> #ifndef CONFIG_USER_ONLY
>> unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu);
>> #else
>> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
>> index 438055c612..e6b4692581 100644
>> --- a/hw/s390x/cpu-topology.c
>> +++ b/hw/s390x/cpu-topology.c
>> @@ -97,6 +97,98 @@ static s390_topology_id s390_topology_from_cpu(S390CPU *cpu)
>> }
>>
>> /**
>> + * s390_topology_set_polarity
>> + * @polarity: horizontal or vertical
>> + *
>> + * Changes the polarity of all the CPU in the configuration.
>> + *
>> + * If the dedicated CPU modifier attribute is set a vertical
>> + * polarization is always high (Architecture).
>> + * Otherwise we decide to set it as medium.
>> + *
>> + * Once done, advertise a topology change.
>> + */
>> +void s390_topology_set_polarity(int polarity)
>
> I don't like that this function ignores what kind of vertical polarization is passed,
> it's confusing.
> That seems like a further reason to split horizontal/vertical from the entitlement.
OK, you are right.
I remove this function and put the s390_cpu_topology_set() inside the
handle_ptf()
>
>> +{
>> + S390TopologyEntry *entry;
>
> I also expected this function to set s390_topology.polarization, but it doesn't.
>> +
>> + QTAILQ_FOREACH(entry, &s390_topology.list, next) {
>> + if (polarity == S390_TOPOLOGY_POLARITY_HORIZONTAL) {
>> + entry->id.p = polarity;
>> + } else {
>> + if (entry->id.d) {
>> + entry->id.p = S390_TOPOLOGY_POLARITY_VERTICAL_HIGH;
>> + } else {
>> + entry->id.p = S390_TOPOLOGY_POLARITY_VERTICAL_MEDIUM;
>> + }
>> + }
>> + }
>> + s390_cpu_topology_set();
>> +}
>> +
>> +/*
>> + * s390_handle_ptf:
>> + *
>> + * @register 1: contains the function code
>> + *
>> + * Function codes 0 and 1 handle the CPU polarization.
>> + * We assume an horizontal topology, the only one supported currently
>> + * by Linux, consequently we answer to function code 0, requesting
>> + * horizontal polarization that it is already the current polarization
>> + * and reject vertical polarization request without further explanation.
>
> This comment is outdated, right? Same for those in the function body.
>
>> + *
>> + * Function code 2 is handling topology changes and is interpreted
>> + * by the SIE.
>> + */
>> +void s390_handle_ptf(S390CPU *cpu, uint8_t r1, uintptr_t ra)
>> +{
>> + CPUS390XState *env = &cpu->env;
>> + uint64_t reg = env->regs[r1];
>> + uint8_t fc = reg & S390_TOPO_FC_MASK;
>> +
>> + if (!s390_has_feat(S390_FEAT_CONFIGURATION_TOPOLOGY)) {
>> + s390_program_interrupt(env, PGM_OPERATION, ra);
>> + return;
>> + }
>> +
>> + if (env->psw.mask & PSW_MASK_PSTATE) {
>> + s390_program_interrupt(env, PGM_PRIVILEGED, ra);
>> + return;
>> + }
>> +
>> + if (reg & ~S390_TOPO_FC_MASK) {
>> + s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>> + return;
>> + }
>> +
>> + switch (fc) {
>> + case 0: /* Horizontal polarization is already set */
>> + if (s390_topology.polarity == S390_TOPOLOGY_POLARITY_HORIZONTAL) {
>> + env->regs[r1] |= S390_PTF_REASON_DONE;
>> + setcc(cpu, 2);
>> + } else {
>> + s390_topology_set_polarity(S390_TOPOLOGY_POLARITY_HORIZONTAL);
>> + s390_topology.polarity = S390_TOPOLOGY_POLARITY_HORIZONTAL;
>> + setcc(cpu, 0);
>> + }
>> + break;
>> + case 1: /* Vertical polarization is not supported */
>> + if (s390_topology.polarity != S390_TOPOLOGY_POLARITY_HORIZONTAL) {
>> + env->regs[r1] |= S390_PTF_REASON_DONE;
>> + setcc(cpu, 2);
>> + } else {
>> + s390_topology_set_polarity(S390_TOPOLOGY_POLARITY_VERTICAL_LOW);
>
> This is why I said it's confusing, nothing gets set to LOW.
>
>> + s390_topology.polarity = S390_TOPOLOGY_POLARITY_VERTICAL_LOW;
>
> Why LOW here?
I wanted something not being S390_TOPOLOGY_POLARITY_HORIZONTAL and did
not want to define a S390_TOPOLOGY_POLARITY_VERTICAL.
OK I define S390_TOPOLOGY_POLARITY_HORIZONTAL=0 and ..._VERTICAL=1
>
>> + setcc(cpu, 0);
>> + }
>> + break;
>> + default:
>> + /* Note that fc == 2 is interpreted by the SIE */
>> + s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>> + }
>
> You can simplify this by doing:
>
> int new_polarity;
> switch (fc) {
> case 0:
> new_polarity = S390_TOPOLOGY_POLARITY_HORIZONTAL;
> break;
> case 1:
> new_polarity = S390_TOPOLOGY_POLARITY_VERTICAL_?;
> break;
> default:
> /* Note that fc == 2 is interpreted by the SIE */
> s390_program_interrupt(env, PGM_SPECIFICATION, ra);
> return;
> }
>
> if same polarity:
> rc done, rejected
> else
> set polarity, initiated
>
> Might be a good idea to turn the polarity values into an enum.
>
>> +}
> [...]
>
Even I never really understood the added value of an enum I can do this.
Thanks,
regards,
Pierre
--
Pierre Morel
IBM Lab Boeblingen
© 2016 - 2026 Red Hat, Inc.