Re: [qemu-s390x] [RFC PATCH 2/2] s390: add cpu feat and migration support for diagnose 318

[David Hildenbrand]

>      read_info->facilities = cpu_to_be64(SCLP_HAS_CPU_INFO |
>                                          SCLP_HAS_IOA_RECONFIG);
> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
> index f9db243..0828e68 100644
> --- a/include/hw/s390x/sclp.h
> +++ b/include/hw/s390x/sclp.h
> @@ -133,6 +133,8 @@ typedef struct ReadInfo {
>      uint16_t highest_cpu;
>      uint8_t  _reserved5[124 - 122];     /* 122-123 */
>      uint32_t hmfai;
> +    uint8_t  _reserved7[134 - 128];     /* 133-128 */

"128-133"

> +    uint8_t  fac134[1];

Is there any nicer name for this? Or does the AR also use this
terminology? Anything related to "configuration characteristics
extension 2" or so mentioned?

(Is this block purely for indicating DIAGs?)

>      struct CPUEntry entries[0];
>  } QEMU_PACKED ReadInfo;

You are changing the size of ReadInfo.

I remember that we had to limit the max # of VCPUs to 248 so this block
would not exceed 4k. If I am right. we can no longer support 248 VCPUs.

CPUEntry is 16 bytes.

ReadInfo was 128 bytes. Now it is 136 bytes.

128 + 16 * 248 = 4k

So with 136 bytes, we can only support a maximum of 247 VCPUs.

Please verify.


>  
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 8ed4823..68c4bed 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -421,6 +421,13 @@ void s390_crypto_reset(void)
>      }
>  }
>  
> +void s390_diag318_reset(void)
> +{
> +    if (kvm_enabled() && s390_has_feat(S390_FEAT_DIAG318)) {
> +        kvm_s390_set_cpc(0);

You probably need a dummy function in kvm-stub.c to make this compile
without CONFIG_KVM.

> +    }
> +}
> +
>  void s390_enable_css_support(S390CPU *cpu)
>  {
>      if (kvm_enabled()) {
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 6f8861e..1d7d1d8 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -123,6 +123,8 @@ struct CPUS390XState {
>      uint64_t gbea;
>      uint64_t pp;
>  
> +    uint64_t cpc;
> +
>      /* Fields up to this point are cleared by a CPU reset */
>      struct {} end_reset_fields;

The CPC is per VM, no? So this does definitely not belong into the CPU.

(also, you would migrate that value multiple times, leading to whoever
writes last winning)

>  
> @@ -716,6 +718,7 @@ static inline void s390_do_cpu_load_normal(CPUState *cs, 
> run_on_cpu_data arg)
>  
>  /* cpu.c */
>  void s390_crypto_reset(void);
> +void s390_diag318_reset(void);
>  int s390_set_memory_limit(uint64_t new_limit, uint64_t *hw_limit);
>  void s390_cmma_reset(void);
>  void s390_enable_css_support(S390CPU *cpu);
> diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
> index 172fb18..2af5d73 100644
> --- a/target/s390x/cpu_features.c
> +++ b/target/s390x/cpu_features.c
> @@ -119,6 +119,9 @@ static const S390FeatDef s390_features[] = {
>      FEAT_INIT("pfmfi", S390_FEAT_TYPE_SCLP_CONF_CHAR_EXT, 9, "SIE: PFMF 
> interpretation facility"),
>      FEAT_INIT("ibs", S390_FEAT_TYPE_SCLP_CONF_CHAR_EXT, 10, "SIE: 
> Interlock-and-broadcast-suppression facility"),
>  
> +    /* SCLP SCCB Byte 134 */
> +    FEAT_INIT("diag318", S390_FEAT_TYPE_SCLP_CONF_FAC134, 0, "SIE: Diagnose 
> 318"),> +
>      FEAT_INIT("sief2", S390_FEAT_TYPE_SCLP_CPU, 4, "SIE: interception format 
> 2 (Virtual SIE)"),
>      FEAT_INIT("skey", S390_FEAT_TYPE_SCLP_CPU, 5, "SIE: Storage-key 
> facility"),
>      FEAT_INIT("gpereh", S390_FEAT_TYPE_SCLP_CPU, 10, "SIE: Guest-PER 
> enhancement facility"),
> diff --git a/target/s390x/cpu_features.h b/target/s390x/cpu_features.h
> index effe790..b3161f5 100644
> --- a/target/s390x/cpu_features.h
> +++ b/target/s390x/cpu_features.h
> @@ -23,6 +23,7 @@ typedef enum {
>      S390_FEAT_TYPE_STFL,
>      S390_FEAT_TYPE_SCLP_CONF_CHAR,
>      S390_FEAT_TYPE_SCLP_CONF_CHAR_EXT,
> +    S390_FEAT_TYPE_SCLP_CONF_FAC134,
>      S390_FEAT_TYPE_SCLP_CPU,
>      S390_FEAT_TYPE_MISC,
>      S390_FEAT_TYPE_PLO,
> diff --git a/target/s390x/cpu_features_def.h b/target/s390x/cpu_features_def.h
> index ac2c947..a917a7e 100644
> --- a/target/s390x/cpu_features_def.h
> +++ b/target/s390x/cpu_features_def.h
> @@ -107,6 +107,9 @@ typedef enum {
>      S390_FEAT_SIE_PFMFI,
>      S390_FEAT_SIE_IBS,
>  
> +    /* Sclp Conf Fac134 */
> +    S390_FEAT_DIAG318,
> +
>      /* Sclp Cpu */
>      S390_FEAT_SIE_F2,
>      S390_FEAT_SIE_SKEY,
> diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
> index 384b61c..6d5e7ea 100644
> --- a/target/s390x/gen-features.c
> +++ b/target/s390x/gen-features.c
> @@ -447,6 +447,7 @@ static uint16_t full_GEN12_GA1[] = {
>      S390_FEAT_ADAPTER_INT_SUPPRESSION,
>      S390_FEAT_EDAT_2,
>      S390_FEAT_SIDE_EFFECT_ACCESS_ESOP2,
> +    S390_FEAT_DIAG318,
>  };
>  
>  static uint16_t full_GEN12_GA2[] = {
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 348e8cc..93324bd 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -742,6 +742,17 @@ int kvm_s390_set_clock_ext(uint8_t tod_high, uint64_t 
> tod_low)
>      return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attr);
>  }
>  
> +int kvm_s390_set_cpc(uint64_t cpc)
> +{
> +    struct kvm_device_attr attr = {
> +        .group = KVM_S390_VM_MISC,
> +        .attr = KVM_S390_VM_MISC_CPC,
> +        .addr = (uint64_t)&cpc,
> +    };
> +
> +    return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attr);
> +}
> +
>  /**
>   * kvm_s390_mem_op:
>   * @addr:      the logical start address in guest memory
> @@ -2134,6 +2145,7 @@ static int kvm_to_feat[][2] = {
>      { KVM_S390_VM_CPU_FEAT_PFMFI, S390_FEAT_SIE_PFMFI},
>      { KVM_S390_VM_CPU_FEAT_SIGPIF, S390_FEAT_SIE_SIGPIF},
>      { KVM_S390_VM_CPU_FEAT_KSS, S390_FEAT_SIE_KSS},
> +    { KVM_S390_VM_CPU_FEAT_DIAG318, S390_FEAT_DIAG318},

An alternative to this would be sensing if the device attribute is
available ... but this way works, too.

>  };
>  
>  static int query_cpu_feat(S390FeatBitmap features)
> diff --git a/target/s390x/kvm_s390x.h b/target/s390x/kvm_s390x.h
> index 6e52287..b7a53ba 100644
> --- a/target/s390x/kvm_s390x.h
> +++ b/target/s390x/kvm_s390x.h
> @@ -29,6 +29,7 @@ int kvm_s390_get_clock(uint8_t *tod_high, uint64_t 
> *tod_clock);
>  int kvm_s390_get_clock_ext(uint8_t *tod_high, uint64_t *tod_clock);
>  int kvm_s390_set_clock(uint8_t tod_high, uint64_t tod_clock);
>  int kvm_s390_set_clock_ext(uint8_t tod_high, uint64_t tod_clock);
> +int kvm_s390_set_cpc(uint64_t cpc);
>  void kvm_s390_enable_css_support(S390CPU *cpu);
>  int kvm_s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch,
>                                      int vq, bool assign);
> diff --git a/target/s390x/machine.c b/target/s390x/machine.c
> index cb792aa..f855531 100644
> --- a/target/s390x/machine.c
> +++ b/target/s390x/machine.c
> @@ -233,6 +233,51 @@ const VMStateDescription vmstate_etoken = {
>      }
>  };
>  
> +static int cpc_post_load(void *opaque, int version_id)
> +{
> +    S390CPU *cpu = opaque;
> +
> +    if (kvm_enabled()) {
> +        return kvm_s390_set_cpc(cpu->env.cpc);
> +    }
> +
> +    return 0;
> +}
> +
> +static int cpc_pre_save(void *opaque)
> +{
> +    S390CPU *cpu = opaque;
> +    struct kvm_device_attr attr = {
> +        .group = KVM_S390_VM_MISC,
> +        .attr = KVM_S390_VM_MISC_CPC,
> +        .addr = (uint64_t)&cpu->env.cpc,
> +    };
> +
> +    if (kvm_enabled()) {
> +        return kvm_vm_ioctl(kvm_state, KVM_GET_DEVICE_ATTR, &attr);
> +    }
> +

That should be factored out into kvm.c -> kvm_s390_get_cpc()

... but the CPU is the wrong place for this either way I think.

This should be migrated and stored along with the SCLP device or the
machine state.

-- 

Thanks,

David / dhildenb