Re: [PATCH v6 2/8] target/s390x: add zpci-interp to cpu models

[Matthew Rosato <mjrosato@linux.ibm.com>]

On 6/1/22 5:52 AM, David Hildenbrand wrote:
> On 24.05.22 21:02, Matthew Rosato wrote:
>> The zpci-interp feature is used to specify whether zPCI interpretation is
>> to be used for this guest.
> 
> We have
> 
> DEF_FEAT(SIE_PFMFI, "pfmfi", SCLP_CONF_CHAR_EXT, 9, "SIE: PFMF
> interpretation facility")
> 
> and
> 
> DEF_FEAT(SIE_SIGPIF, "sigpif", SCLP_CPU, 12, "SIE: SIGP interpretation
> facility")
> 
> 
> Should we call this simply "zpcii" or "zpciif" (if the official name
> includes "Facility")
> 

This actually controls the use of 2 facilities which really only make 
sense together - Maybe just zpcii

>>
>> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
>> ---
>>   hw/s390x/s390-virtio-ccw.c          | 1 +
>>   target/s390x/cpu_features_def.h.inc | 1 +
>>   target/s390x/gen-features.c         | 2 ++
>>   target/s390x/kvm/kvm.c              | 1 +
>>   4 files changed, 5 insertions(+)
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 047cca0487..b33310a135 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -806,6 +806,7 @@ static void ccw_machine_7_0_instance_options(MachineState *machine)
>>       static const S390FeatInit qemu_cpu_feat = { S390_FEAT_LIST_QEMU_V7_0 };
>>   
>>       ccw_machine_7_1_instance_options(machine);
>> +    s390_cpudef_featoff_greater(14, 1, S390_FEAT_ZPCI_INTERP);
>>       s390_set_qemu_cpu_model(0x8561, 15, 1, qemu_cpu_feat);
>>   }
>>   
>> diff --git a/target/s390x/cpu_features_def.h.inc b/target/s390x/cpu_features_def.h.inc
>> index e86662bb3b..4ade3182aa 100644
>> --- a/target/s390x/cpu_features_def.h.inc
>> +++ b/target/s390x/cpu_features_def.h.inc
>> @@ -146,6 +146,7 @@ DEF_FEAT(SIE_CEI, "cei", SCLP_CPU, 43, "SIE: Conditional-external-interception f
>>   DEF_FEAT(DAT_ENH_2, "dateh2", MISC, 0, "DAT-enhancement facility 2")
>>   DEF_FEAT(CMM, "cmm", MISC, 0, "Collaborative-memory-management facility")
>>   DEF_FEAT(AP, "ap", MISC, 0, "AP instructions installed")
>> +DEF_FEAT(ZPCI_INTERP, "zpci-interp", MISC, 0, "zPCI interpretation")
> 
> How is this feature exposed to the guest, meaning, how can the guest
> sense support?
> 
> Just a gut feeling: does this toggle enable the host to use
> interpretation and the guest cannot really determine the difference
> whether it's enabled or not? Then, it's not a guest CPU feature. But
> let's hear first what this actually enables :)

This has changed a few times, but collectively we can determine on the 
host kernel if it is allowable based upon the availability of certain 
facility/sclp bits + the availability of an ioctl interface.

If all of these are available, the host kernel allows zPCI 
interpretation, with userspace able to toggle it on/off for the guest 
via this feature.  When allowed and enabled, 2 ECB bits then get set for 
each guest vcpu that enable the associated facilities.  The guest 
continues to use zPCI instructions in the same manner as before; the 
function handles it receives from CLP instructions will look different 
but are still used in the same manner.

We don't yet add vsie support of the facilities with this series, so the 
corresponding facility and sclp bits aren't forwarded to the guest.

> 
>>   
>>   /* Features exposed via the PLO instruction. */
>>   DEF_FEAT(PLO_CL, "plo-cl", PLO, 0, "PLO Compare and load (32 bit in general registers)")
>> diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
>> index c03ec2c9a9..f991646c01 100644
>> --- a/target/s390x/gen-features.c
>> +++ b/target/s390x/gen-features.c
>> @@ -554,6 +554,7 @@ static uint16_t full_GEN14_GA1[] = {
>>       S390_FEAT_HPMA2,
>>       S390_FEAT_SIE_KSS,
>>       S390_FEAT_GROUP_MULTIPLE_EPOCH_PTFF,
>> +    S390_FEAT_ZPCI_INTERP,
>>   };
>>   
>>   #define full_GEN14_GA2 EmptyFeat
>> @@ -650,6 +651,7 @@ static uint16_t default_GEN14_GA1[] = {
>>       S390_FEAT_GROUP_MSA_EXT_8,
>>       S390_FEAT_MULTIPLE_EPOCH,
>>       S390_FEAT_GROUP_MULTIPLE_EPOCH_PTFF,
>> +    S390_FEAT_ZPCI_INTERP,
> 
> I'm curious, should we really add this to the default model?
> 
> This implies that on any setup where we don't have zpci interpretation
> support (including missing kernel support), that a basic "-cpu z14" will
> no longer work with the new machine type.
> 
> If, OTOH, we expect this feature to be around in any sane installation,
> then it's good to include it in the
> 

 From a hardware perspective, everything will be available on z14 and 
later so it's only a question of missing host kernel support (or, you 
aren't running in a z14 LPAR).  As far as host kernel support, the 
expectation is that for a distro release where this QEMU support lands 
the associated kernel support would also be backported.  I guess that 
leaves some awkwardness if one upgrades their distro qemu to a new 
release version without picking up the kernel upgrade for some reason.. 
In that case, you're not totally stuck, you could still use -cpu 
z14,zpcii=off (or better yet pick up the associated kernel upgrade...) 
The intent is for exploitation of interpretation facilities to become 
the default on z14 and later, with the ability to turn it off offered as 
a fall-back / backwards compatibility.

If there's a better way to accomplish that, I'm open to suggestion.