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

[Cornelia Huck]

On Fri, 15 May 2020 18:20:32 -0400
Collin Walling <address@hidden> wrote:

> DIAGNOSE 0x318 (diag 318) allows the storage of diagnostic data that
> is collected by the firmware in the case of hardware/firmware service
> events.
> 
> The instruction is invoked in the Linux kernel and is handled,
> migrated, and reset (modified clear and load normal) by QEMU.
> KVM assists with the get/set of the relevent data via IOCTLs.
> 
> 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 diag 318
> 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.

I think it should be easy enough to wire it up for !kvm as well
(although I'm not sure how useful it would be there -- mostly for
seeing what a guest does with it, I guess.)

> 
> Signed-off-by: Collin Walling <address@hidden>
> ---
>  hw/s390x/s390-virtio-ccw.c          | 45 +++++++++++++++++++++++++++++
>  hw/s390x/sclp.c                     |  5 ++++
>  include/hw/s390x/s390-virtio-ccw.h  |  1 +
>  include/hw/s390x/sclp.h             |  3 ++
>  target/s390x/cpu.c                  | 23 +++++++++++++++
>  target/s390x/cpu.h                  |  4 +++
>  target/s390x/cpu_features.h         |  1 +
>  target/s390x/cpu_features_def.inc.h |  3 ++
>  target/s390x/cpu_models.c           |  1 +
>  target/s390x/gen-features.c         |  1 +
>  target/s390x/kvm-stub.c             | 10 +++++++
>  target/s390x/kvm.c                  | 40 +++++++++++++++++++++++++
>  target/s390x/kvm_s390x.h            |  2 ++
>  13 files changed, 139 insertions(+)
> 

(...)

> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index f2ccf0a06a..367a54c173 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -446,6 +446,29 @@ void s390_enable_css_support(S390CPU *cpu)
>          kvm_s390_enable_css_support(cpu);
>      }
>  }
> +
> +void s390_get_diag_318_info(uint64_t *info)
> +{
> +    if (kvm_enabled()) {
> +        kvm_s390_get_diag_318_info(info);
> +    }
> +}
> +
> +void s390_set_diag_318_info(uint64_t info)
> +{
> +    if (kvm_enabled()) {
> +        kvm_s390_set_diag_318_info(info);
> +    }
> +}
> +
> +bool s390_diag_318_is_allowed(void)
> +{
> +    if (kvm_enabled()) {

I would recommend not tying this explicitly to kvm -- assuming that the
feature checks should be enough.

> +        return s390_has_feat(S390_FEAT_DIAG_318) &&
> +               s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB);
> +    }
> +    return false;
> +}
>  #endif
>  
>  static gchar *s390_gdb_arch_name(CPUState *cs)

(...)

> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 380fb81822..5d7dc60c85 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
>  
> @@ -814,6 +815,28 @@ 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_get_diag_318_info(uint64_t *info)
> +{
> +    struct kvm_device_attr attr = {
> +        .group = KVM_S390_VM_MISC,
> +        .attr = KVM_S390_VM_MISC_DIAG_318,
> +        .addr = (uint64_t)info,
> +    };
> +
> +    return kvm_vm_ioctl(kvm_state, KVM_GET_DEVICE_ATTR, &attr);
> +}
> +
> +int kvm_s390_set_diag_318_info(uint64_t info)
> +{
> +    struct kvm_device_attr attr = {
> +        .group = KVM_S390_VM_MISC,
> +        .attr = KVM_S390_VM_MISC_DIAG_318,
> +        .addr = (uint64_t)&info,
> +    };
> +
> +    return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attr);
> +}
> +
>  /**
>   * kvm_s390_mem_op:
>   * @addr:      the logical start address in guest memory
> @@ -1601,6 +1624,14 @@ static int handle_sw_breakpoint(S390CPU *cpu, struct 
> kvm_run *run)
>      return -ENOENT;
>  }
>  
> +static void kvm_handle_diag_318(struct kvm_run *run)
> +{
> +    uint64_t reg = (run->s390_sieic.ipa & 0x00f0) >> 4;
> +    uint64_t info = run->s.regs.gprs[reg];
> +
> +    kvm_s390_set_diag_318_info(info);

Follow the other diag handlers and rather call a common
handle_diag_318() function, which will in turn call
s390_set_diag_318_info()? While that feels like a bit of extra churn at
a glance, it allows non-kvm access to this diag more easily.

> +}
> +
>  #define DIAG_KVM_CODE_MASK 0x000000000000ffff
>  
>  static int handle_diag(S390CPU *cpu, struct kvm_run *run, uint32_t ipb)
> @@ -1620,6 +1651,9 @@ static int handle_diag(S390CPU *cpu, struct kvm_run 
> *run, uint32_t ipb)
>      case DIAG_IPL:
>          kvm_handle_diag_308(cpu, run);
>          break;
> +    case DIAG_SET_CONTROL_PROGRAM_CODES:
> +        kvm_handle_diag_318(run);
> +        break;
>      case DIAG_KVM_HYPERCALL:
>          r = handle_hypercall(cpu, run);
>          break;
> @@ -2460,6 +2494,12 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, 
> Error **errp)
>      /* Extended-Length SCCB is handled entirely within QEMU */
>      set_bit(S390_FEAT_EXTENDED_LENGTH_SCCB, model->features);
>  
> +    /* Allow diag 0x318 iff KVM supported and not in PV mode */

"iff supported by KVM" ?

> +    if (!s390_is_pv() && kvm_vm_check_attr(kvm_state,
> +        KVM_S390_VM_MISC, KVM_S390_VM_MISC_DIAG_318)) {
> +        set_bit(S390_FEAT_DIAG_318, model->features);
> +    }
> +
>      /* strip of features that are not part of the maximum model */
>      bitmap_and(model->features, model->features, model->def->full_feat,
>                 S390_FEAT_MAX);

(...)