Re: [PATCH v5 8/8] s390: guest support for diagnose 0x318

[Collin Walling <walling@linux.ibm.com>]

On 9/11/20 11:08 AM, Thomas Huth wrote:
> On 10/09/2020 11.36, Collin Walling wrote:
>> DIAGNOSE 0x318 (diag318) is an s390 instruction that allows the storage
>> of diagnostic information that is collected by the firmware in the case
>> of hardware/firmware service events.
>>
>> QEMU handles the instruction by storing the info in the CPU state. A
>> subsequent register sync will communicate the data to the hypervisor.
>>
>> QEMU handles the migration via a VM State Description.
>>
>> This feature depends on the Extended-Length SCCB (els) feature. If
>> els is not present, then a warning will be printed and the SCLP bit
>> that allows the Linux kernel to execute the instruction will not be
>> set.
>>
>> Availability of this instruction is determined by byte 134 (aka fac134)
>> bit 0 of the SCLP Read Info block. This coincidentally expands into the
>> space used for CPU entries, which means VMs running with the diag318
>> capability may not be able to read information regarding all CPUs
>> unless the guest kernel supports an extended-length SCCB.
>>
>> This feature is not supported in protected virtualization mode.
>>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> Acked-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  hw/s390x/sclp.c                     |  5 +++++
>>  include/hw/s390x/sclp.h             |  3 +++
>>  target/s390x/cpu.h                  |  2 ++
>>  target/s390x/cpu_features.h         |  1 +
>>  target/s390x/cpu_features_def.h.inc |  3 +++
>>  target/s390x/cpu_models.c           |  1 +
>>  target/s390x/gen-features.c         |  1 +
>>  target/s390x/kvm.c                  | 31 +++++++++++++++++++++++++++++
>>  target/s390x/machine.c              | 17 ++++++++++++++++
>>  9 files changed, 64 insertions(+)
>>
>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>> index 87d468087b..ad5d70e14d 100644
>> --- a/hw/s390x/sclp.c
>> +++ b/hw/s390x/sclp.c
>> @@ -139,6 +139,11 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>>      s390_get_feat_block(S390_FEAT_TYPE_SCLP_CONF_CHAR_EXT,
>>                           read_info->conf_char_ext);
>>  
>> +    if (s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB)) {
>> +        s390_get_feat_block(S390_FEAT_TYPE_SCLP_FAC134,
>> +                            &read_info->fac134);
>> +    }
> 
> Wasn't this feature also possible if there are less than 240 CPUs? Or do
> I mix that up with something else? ... well, maybe it's best anyway if
> we only allow this when ELS is enabled.
> 
>>      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 141e57f765..516f949109 100644
>> --- a/include/hw/s390x/sclp.h
>> +++ b/include/hw/s390x/sclp.h
>> @@ -133,6 +133,9 @@ typedef struct ReadInfo {
>>      uint16_t highest_cpu;
>>      uint8_t  _reserved5[124 - 122];     /* 122-123 */
>>      uint32_t hmfai;
>> +    uint8_t  _reserved7[134 - 128];     /* 128-133 */
>> +    uint8_t  fac134;
>> +    uint8_t  _reserved8[144 - 135];     /* 135-143 */
>>      struct CPUEntry entries[];
> 
> Could you please add a comment after entries[] like,
> 
>  /* only here with "ELS", it's at offset 128 otherwise */
> 
> ?
> 
>>  } QEMU_PACKED ReadInfo;
>>  
> [...]
>>  static uint16_t full_GEN12_GA2[] = {
>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>> index a2d5ad78f6..b79feeba9f 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -105,6 +105,7 @@
>>  
>>  #define DIAG_TIMEREVENT                 0x288
>>  #define DIAG_IPL                        0x308
>> +#define DIAG_SET_CONTROL_PROGRAM_CODES  0x318
>>  #define DIAG_KVM_HYPERCALL              0x500
>>  #define DIAG_KVM_BREAKPOINT             0x501
>>  
>> @@ -602,6 +603,11 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>>          cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_ETOKEN;
>>      }
>>  
>> +    if (can_sync_regs(cs, KVM_SYNC_DIAG318)) {
>> +        cs->kvm_run->s.regs.diag318 = env->diag318_info;
>> +        cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_DIAG318;
>> +    }
>> +
>>      /* Finally the prefix */
>>      if (can_sync_regs(cs, KVM_SYNC_PREFIX)) {
>>          cs->kvm_run->s.regs.prefix = env->psa;
>> @@ -741,6 +747,10 @@ int kvm_arch_get_registers(CPUState *cs)
>>          }
>>      }
>>  
>> +    if (can_sync_regs(cs, KVM_SYNC_DIAG318)) {
>> +        env->diag318_info = cs->kvm_run->s.regs.diag318;
>> +    }
>> +
>>      return 0;
>>  }
>>  
>> @@ -1601,6 +1611,19 @@ static int handle_sw_breakpoint(S390CPU *cpu, struct kvm_run *run)
>>      return -ENOENT;
>>  }
>>  
>> +static void handle_diag_318(S390CPU *cpu, struct kvm_run *run)
>> +{
>> +    uint64_t reg = (run->s390_sieic.ipa & 0x00f0) >> 4;
>> +    uint64_t diag318_info = run->s.regs.gprs[reg];
>> +
>> +    cpu->env.diag318_info = diag318_info;
>> +
>> +    if (can_sync_regs(CPU(cpu), KVM_SYNC_DIAG318)) {
>> +        run->s.regs.diag318 = diag318_info;
>> +        run->kvm_dirty_regs |= KVM_SYNC_DIAG318;
>> +    }
>> +}
> 
> What's supposed to happen if a guest calls diag 318 but the
> S390_FEAT_DIAG_318 has not been enabled? Shouldn't there be a program
> interrupt in that case?
> 
>  Thomas
> 
> 

Right... an edge case where a guest kernel tries to erroneously call
diag 318 without checking the SCLP bit first. I'll add this fence.

-- 
Regards,
Collin

Stay safe and stay healthy