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

[Thomas Huth]

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