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

[David Hildenbrand]

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")

> 
> 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 :)

>  
>  /* 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

-- 
Thanks,

David / dhildenb