Re: [PATCH v14 06/11] s390x/cpu topology: interception of PTF instruction

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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@xxxxxxxxxxxxx>
---
  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



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux