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

[Collin Walling]

On 09/01/2018 10:02 AM, David Hildenbrand wrote:
>>      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"
>

Oops! Thanks for catching that one.

>> +    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?

I failed to find anything in our documentation that labels this byte as
something more helpful. "conf_char_ext2" sounds like the right idea, though.

> 
> (Is this block purely for indicating DIAGs?)
> 

In its current definition, this byte is only used for DIAGs and some reserved 
fields.

I do not yet see this new byte added to the SCLP / Read SCP Info doc. I will ask
around for more information.

>>      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.

Unfortunately, yes. We lose a VCPU because of this SCCB extension...

> 
> 
>>  
>> 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.
> 

Yes, you are right. I will make this addition.

>> +    }
>> +}
>> +
>>  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)
> 

Ah, this makes sense. Looking at the debug logs, I do in fact see a "VM_EVENT"
for get/set of the CPC occurring twice.

>>  
>> @@ -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.
> 

Agreed. I'll move the migration code to something that makes more sense.

Thank you for the review.

-- 
Respectfully,
- Collin Walling