| Message ID | 20200910093655.255774-9-walling@linux.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | s390: Extended-Length SCCB & DIAGNOSE 0x318 | expand |
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
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.
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); + } + 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[]; } QEMU_PACKED ReadInfo; diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h index 035427521c..f875ebf0f4 100644 --- a/target/s390x/cpu.h +++ b/target/s390x/cpu.h @@ -112,6 +112,8 @@ struct CPUS390XState { uint16_t external_call_addr; DECLARE_BITMAP(emergency_signals, S390_MAX_CPUS); + uint64_t diag318_info; + /* Fields up to this point are cleared by a CPU reset */ struct {} end_reset_fields; diff --git a/target/s390x/cpu_features.h b/target/s390x/cpu_features.h index 2a29475493..ef52ffce83 100644 --- a/target/s390x/cpu_features.h +++ b/target/s390x/cpu_features.h @@ -23,6 +23,7 @@ typedef enum { S390_FEAT_TYPE_STFL, S390_FEAT_TYPE_SCLP_CONF_CHAR, S390_FEAT_TYPE_SCLP_CONF_CHAR_EXT, + S390_FEAT_TYPE_SCLP_FAC134, S390_FEAT_TYPE_SCLP_CPU, S390_FEAT_TYPE_MISC, S390_FEAT_TYPE_PLO, diff --git a/target/s390x/cpu_features_def.h.inc b/target/s390x/cpu_features_def.h.inc index 1c04cc18f4..f82b4b5ec1 100644 --- a/target/s390x/cpu_features_def.h.inc +++ b/target/s390x/cpu_features_def.h.inc @@ -122,6 +122,9 @@ DEF_FEAT(SIE_CMMA, "cmma", SCLP_CONF_CHAR_EXT, 1, "SIE: Collaborative-memory-man DEF_FEAT(SIE_PFMFI, "pfmfi", SCLP_CONF_CHAR_EXT, 9, "SIE: PFMF interpretation facility") DEF_FEAT(SIE_IBS, "ibs", SCLP_CONF_CHAR_EXT, 10, "SIE: Interlock-and-broadcast-suppression facility") +/* Features exposed via SCLP SCCB Facilities byte 134 (bit numbers relative to byte-134) */ +DEF_FEAT(DIAG_318, "diag318", SCLP_FAC134, 0, "Control program name and version codes") + /* Features exposed via SCLP CPU info. */ DEF_FEAT(SIE_F2, "sief2", SCLP_CPU, 4, "SIE: interception format 2 (Virtual SIE)") DEF_FEAT(SIE_SKEY, "skey", SCLP_CPU, 5, "SIE: Storage-key facility") diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c index c2af226174..9d615f13e7 100644 --- a/target/s390x/cpu_models.c +++ b/target/s390x/cpu_models.c @@ -824,6 +824,7 @@ static void check_consistency(const S390CPUModel *model) { S390_FEAT_PTFF_STOE, S390_FEAT_MULTIPLE_EPOCH }, { S390_FEAT_PTFF_STOUE, S390_FEAT_MULTIPLE_EPOCH }, { S390_FEAT_AP_QUEUE_INTERRUPT_CONTROL, S390_FEAT_AP }, + { S390_FEAT_DIAG_318, S390_FEAT_EXTENDED_LENGTH_SCCB }, }; int i; diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c index 6857f657fb..a1f0a6f3c6 100644 --- a/target/s390x/gen-features.c +++ b/target/s390x/gen-features.c @@ -523,6 +523,7 @@ static uint16_t full_GEN12_GA1[] = { S390_FEAT_AP_FACILITIES_TEST, S390_FEAT_AP, S390_FEAT_EXTENDED_LENGTH_SCCB, + S390_FEAT_DIAG_318, }; 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; + } +} + #define DIAG_KVM_CODE_MASK 0x000000000000ffff static int handle_diag(S390CPU *cpu, struct kvm_run *run, uint32_t ipb) @@ -1620,6 +1643,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: + handle_diag_318(cpu, run); + break; case DIAG_KVM_HYPERCALL: r = handle_hypercall(cpu, run); break; @@ -2464,6 +2490,11 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp) */ set_bit(S390_FEAT_EXTENDED_LENGTH_SCCB, model->features); + /* DIAGNOSE 0x318 is not supported under protected virtualization */ + if (!s390_is_pv() && kvm_check_extension(kvm_state, KVM_CAP_S390_DIAG318)) { + 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); diff --git a/target/s390x/machine.c b/target/s390x/machine.c index 549bb6c280..5b4e82f1ab 100644 --- a/target/s390x/machine.c +++ b/target/s390x/machine.c @@ -234,6 +234,22 @@ const VMStateDescription vmstate_etoken = { } }; +static bool diag318_needed(void *opaque) +{ + return s390_has_feat(S390_FEAT_DIAG_318); +} + +const VMStateDescription vmstate_diag318 = { + .name = "cpu/diag318", + .version_id = 1, + .minimum_version_id = 1, + .needed = diag318_needed, + .fields = (VMStateField[]) { + VMSTATE_UINT64(env.diag318_info, S390CPU), + VMSTATE_END_OF_LIST() + } +}; + const VMStateDescription vmstate_s390_cpu = { .name = "cpu", .post_load = cpu_post_load, @@ -270,6 +286,7 @@ const VMStateDescription vmstate_s390_cpu = { &vmstate_gscb, &vmstate_bpbc, &vmstate_etoken, + &vmstate_diag318, NULL }, };