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 <wall...@linux.ibm.com>
>> Acked-by: Janosch Frank <fran...@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
