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

[Collin Walling]

On 5/20/20 7:30 AM, Cornelia Huck wrote:
> 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.)
> 

I suppose we could. I'm working with the sync_regs idea that was
mentioned, and if it works the way I think it does, a lot the code on
the QEMU side will be disappearing.

Otherwise, I'll look into this. It should be rather harmless if QEMU
holds onto the data, but doesn't do anything with it.

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

True. If I understand how the sync_regs approach works, then vmstate
stuff should be going away, and the reset code will also be handled in KVM.

Otherwise, I'll make the change.

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

Can do.

>> +}
>> +
>>  #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" ?
>

That sounds a bit better :)


>> +    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);
> 
> (...)
> 
> 

Thanks for the review!

-- 
Regards,
Collin

Stay safe and stay healthy