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

[Pierre Morel]

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