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

[Pierre Morel <pmorel@linux.ibm.com>]

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