| Message ID | 20180117141849.65757-3-borntraeger@de.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x/kvm: implement new hardware/firmware features | expand |
On 17.01.2018 15:18, Christian Borntraeger wrote: > We need to handle the bpb control on reset and migration. Normally > stfle.82 is transparent (and the normal guest part works without > hypervisor activity). To prevent any issues we require full > host kernel support for this feature. Actually it is not transparent because we need hypervisor support to get VSIE running... or what am I missing? (or were you talking about bit 81?)
On Wed, 17 Jan 2018 15:18:48 +0100 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > We need to handle the bpb control on reset and migration. Normally > stfle.82 is transparent (and the normal guest part works without > hypervisor activity). To prevent any issues we require full > host kernel support for this feature. > > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > target/s390x/cpu.c | 1 + > target/s390x/cpu.h | 1 + > target/s390x/cpu_features.c | 1 + > target/s390x/cpu_features_def.h | 1 + > target/s390x/gen-features.c | 1 + > target/s390x/kvm.c | 16 ++++++++++++++++ > target/s390x/machine.c | 17 +++++++++++++++++ > 7 files changed, 38 insertions(+) > > diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c > index ae3cee9..1577b2c 100644 > --- a/target/s390x/cpu.c > +++ b/target/s390x/cpu.c > @@ -89,6 +89,7 @@ static void s390_cpu_reset(CPUState *s) > CPUS390XState *env = &cpu->env; > > env->pfault_token = -1UL; > + env->bpbc = 0; > scc->parent_reset(s); > cpu->env.sigp_order = 0; > s390_cpu_set_state(CPU_STATE_STOPPED, cpu); > diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h > index 1a8b6b9..8514905 100644 > --- a/target/s390x/cpu.h > +++ b/target/s390x/cpu.h > @@ -93,6 +93,7 @@ struct CPUS390XState { > > uint32_t fpc; /* floating-point control register */ > uint32_t cc_op; > + uint8_t bpbc; /* branch prediction blocking */ > > float_status fpu_status; /* passed to softfloat lib */ > > diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c > index 31a4676..5d1c210 100644 > --- a/target/s390x/cpu_features.c > +++ b/target/s390x/cpu_features.c > @@ -89,6 +89,7 @@ static const S390FeatDef s390_features[] = { > FEAT_INIT("msa4-base", S390_FEAT_TYPE_STFL, 77, "Message-security-assist-extension-4 facility (excluding subfunctions)"), > FEAT_INIT("edat2", S390_FEAT_TYPE_STFL, 78, "Enhanced-DAT facility 2"), > FEAT_INIT("dfppc", S390_FEAT_TYPE_STFL, 80, "Decimal-floating-point packed-conversion facility"), > + FEAT_INIT("bpb", S390_FEAT_TYPE_STFL, 82, "Branch Prediction Blocking"), No nice "facility" suffix? :) > FEAT_INIT("vx", S390_FEAT_TYPE_STFL, 129, "Vector facility"), > FEAT_INIT("iep", S390_FEAT_TYPE_STFL, 130, "Instruction-execution-protection facility"), > FEAT_INIT("sea_esop2", S390_FEAT_TYPE_STFL, 131, "Side-effect-access facility and Enhanced-suppression-on-protection facility 2"), > diff --git a/target/s390x/cpu_features_def.h b/target/s390x/cpu_features_def.h > index 4b6d4e9..4487cfd 100644 > --- a/target/s390x/cpu_features_def.h > +++ b/target/s390x/cpu_features_def.h > @@ -80,6 +80,7 @@ typedef enum { > S390_FEAT_MSA_EXT_4, > S390_FEAT_EDAT_2, > S390_FEAT_DFP_PACKED_CONVERSION, > + S390_FEAT_BPB, > S390_FEAT_VECTOR, > S390_FEAT_INSTRUCTION_EXEC_PROT, > S390_FEAT_SIDE_EFFECT_ACCESS_ESOP2, > diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c > index b24f6ad..95ee870 100644 > --- a/target/s390x/gen-features.c > +++ b/target/s390x/gen-features.c > @@ -357,6 +357,7 @@ static uint16_t full_GEN7_GA1[] = { > S390_FEAT_SIE_GPERE, > S390_FEAT_SIE_IB, > S390_FEAT_SIE_CEI, > + S390_FEAT_BPB, Will this be provided that far back on real hardware? > }; > > static uint16_t full_GEN7_GA2[] = { > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c > index 6a18a41..3cd4fab 100644 > --- a/target/s390x/kvm.c > +++ b/target/s390x/kvm.c > @@ -490,6 +490,11 @@ int kvm_arch_put_registers(CPUState *cs, int level) > cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_GSCB; > } > > + if (can_sync_regs(cs, KVM_SYNC_BPBC)) { > + cs->kvm_run->s.regs.bpbc = env->bpbc; > + cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_BPBC; > + } > + > /* Finally the prefix */ > if (can_sync_regs(cs, KVM_SYNC_PREFIX)) { > cs->kvm_run->s.regs.prefix = env->psa; > @@ -600,6 +605,10 @@ int kvm_arch_get_registers(CPUState *cs) > memcpy(env->gscb, cs->kvm_run->s.regs.gscb, 32); > } > > + if (can_sync_regs(cs, KVM_SYNC_BPBC)) { > + env->bpbc = cs->kvm_run->s.regs.bpbc; > + } > + > /* pfault parameters */ > if (can_sync_regs(cs, KVM_SYNC_PFAULT)) { > env->pfault_token = cs->kvm_run->s.regs.pft; > @@ -2278,6 +2287,13 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp) > clear_bit(S390_FEAT_CMM_NT, model->features); > } > > + /* stfle.82 is a transparent bit. As there is some state attached > + * anyway we only enable this bit if the host kernel can handle > + * migrate and reset */ Having a transparent bit with some state attached seems a bit odd... > + if (!kvm_check_extension(kvm_state, KVM_CAP_S390_BPB)) { > + clear_bit(S390_FEAT_BPB, model->features); > + } > + > /* We emulate a zPCI bus and AEN, therefore we don't need HW support */ > if (pci_available) { > set_bit(S390_FEAT_ZPCI, model->features);
On 01/17/2018 03:30 PM, David Hildenbrand wrote: > On 17.01.2018 15:18, Christian Borntraeger wrote: >> We need to handle the bpb control on reset and migration. Normally >> stfle.82 is transparent (and the normal guest part works without >> hypervisor activity). To prevent any issues we require full >> host kernel support for this feature. > > Actually it is not transparent because we need hypervisor support to get > VSIE running... or what am I missing? (or were you talking about bit 81?) When you pass along the bit as a transparent bit (just enable it if the host has it) it will work in nested guests. Its only that after reset or vsie you have a short period of time where you work in a stale state. Anyway we are aware that bit82 should have been a non-transparent bit, consider it a quirk in the architecture. Thats why I handle this feature not in patch 3.
On 17.01.2018 15:37, Cornelia Huck wrote: > On Wed, 17 Jan 2018 15:18:48 +0100 > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > >> We need to handle the bpb control on reset and migration. Normally >> stfle.82 is transparent (and the normal guest part works without >> hypervisor activity). To prevent any issues we require full >> host kernel support for this feature. >> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >> --- >> target/s390x/cpu.c | 1 + >> target/s390x/cpu.h | 1 + >> target/s390x/cpu_features.c | 1 + >> target/s390x/cpu_features_def.h | 1 + >> target/s390x/gen-features.c | 1 + >> target/s390x/kvm.c | 16 ++++++++++++++++ >> target/s390x/machine.c | 17 +++++++++++++++++ >> 7 files changed, 38 insertions(+) >> >> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c >> index ae3cee9..1577b2c 100644 >> --- a/target/s390x/cpu.c >> +++ b/target/s390x/cpu.c >> @@ -89,6 +89,7 @@ static void s390_cpu_reset(CPUState *s) >> CPUS390XState *env = &cpu->env; >> >> env->pfault_token = -1UL; >> + env->bpbc = 0; >> scc->parent_reset(s); >> cpu->env.sigp_order = 0; >> s390_cpu_set_state(CPU_STATE_STOPPED, cpu); >> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h >> index 1a8b6b9..8514905 100644 >> --- a/target/s390x/cpu.h >> +++ b/target/s390x/cpu.h >> @@ -93,6 +93,7 @@ struct CPUS390XState { >> >> uint32_t fpc; /* floating-point control register */ >> uint32_t cc_op; >> + uint8_t bpbc; /* branch prediction blocking */ >> >> float_status fpu_status; /* passed to softfloat lib */ >> >> diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c >> index 31a4676..5d1c210 100644 >> --- a/target/s390x/cpu_features.c >> +++ b/target/s390x/cpu_features.c >> @@ -89,6 +89,7 @@ static const S390FeatDef s390_features[] = { >> FEAT_INIT("msa4-base", S390_FEAT_TYPE_STFL, 77, "Message-security-assist-extension-4 facility (excluding subfunctions)"), >> FEAT_INIT("edat2", S390_FEAT_TYPE_STFL, 78, "Enhanced-DAT facility 2"), >> FEAT_INIT("dfppc", S390_FEAT_TYPE_STFL, 80, "Decimal-floating-point packed-conversion facility"), >> + FEAT_INIT("bpb", S390_FEAT_TYPE_STFL, 82, "Branch Prediction Blocking"), > > No nice "facility" suffix? :) > >> FEAT_INIT("vx", S390_FEAT_TYPE_STFL, 129, "Vector facility"), >> FEAT_INIT("iep", S390_FEAT_TYPE_STFL, 130, "Instruction-execution-protection facility"), >> FEAT_INIT("sea_esop2", S390_FEAT_TYPE_STFL, 131, "Side-effect-access facility and Enhanced-suppression-on-protection facility 2"), >> diff --git a/target/s390x/cpu_features_def.h b/target/s390x/cpu_features_def.h >> index 4b6d4e9..4487cfd 100644 >> --- a/target/s390x/cpu_features_def.h >> +++ b/target/s390x/cpu_features_def.h >> @@ -80,6 +80,7 @@ typedef enum { >> S390_FEAT_MSA_EXT_4, >> S390_FEAT_EDAT_2, >> S390_FEAT_DFP_PACKED_CONVERSION, >> + S390_FEAT_BPB, >> S390_FEAT_VECTOR, >> S390_FEAT_INSTRUCTION_EXEC_PROT, >> S390_FEAT_SIDE_EFFECT_ACCESS_ESOP2, >> diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c >> index b24f6ad..95ee870 100644 >> --- a/target/s390x/gen-features.c >> +++ b/target/s390x/gen-features.c >> @@ -357,6 +357,7 @@ static uint16_t full_GEN7_GA1[] = { >> S390_FEAT_SIE_GPERE, >> S390_FEAT_SIE_IB, >> S390_FEAT_SIE_CEI, >> + S390_FEAT_BPB, > > Will this be provided that far back on real hardware? > >> }; >> >> static uint16_t full_GEN7_GA2[] = { >> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c >> index 6a18a41..3cd4fab 100644 >> --- a/target/s390x/kvm.c >> +++ b/target/s390x/kvm.c >> @@ -490,6 +490,11 @@ int kvm_arch_put_registers(CPUState *cs, int level) >> cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_GSCB; >> } >> >> + if (can_sync_regs(cs, KVM_SYNC_BPBC)) { >> + cs->kvm_run->s.regs.bpbc = env->bpbc; >> + cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_BPBC; >> + } >> + >> /* Finally the prefix */ >> if (can_sync_regs(cs, KVM_SYNC_PREFIX)) { >> cs->kvm_run->s.regs.prefix = env->psa; >> @@ -600,6 +605,10 @@ int kvm_arch_get_registers(CPUState *cs) >> memcpy(env->gscb, cs->kvm_run->s.regs.gscb, 32); >> } >> >> + if (can_sync_regs(cs, KVM_SYNC_BPBC)) { >> + env->bpbc = cs->kvm_run->s.regs.bpbc; >> + } >> + >> /* pfault parameters */ >> if (can_sync_regs(cs, KVM_SYNC_PFAULT)) { >> env->pfault_token = cs->kvm_run->s.regs.pft; >> @@ -2278,6 +2287,13 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp) >> clear_bit(S390_FEAT_CMM_NT, model->features); >> } >> >> + /* stfle.82 is a transparent bit. As there is some state attached >> + * anyway we only enable this bit if the host kernel can handle >> + * migrate and reset */ > > Having a transparent bit with some state attached seems a bit odd... > And exactly for this reason I tend to nack patch nr 3 (if that is of any weight :) ). As soon as we enable bits for CPU models, we guarantee that migration works. While introducing this change we already had one example where this was not the case. Not good. (and remember having another such exception) It is easier to patch a feature in than silently enabling *anything* somebody thinks is transparent (but its not). Especially not for the host model. The expanded host model is migration safe. And as we saw, in the unlikely event of such heavy changes, we need to touch fw/linux/qemu either way. But there is more I dislike about the approach in patch 3: 1. feature names. We need aliases. Different QEMU versions on the same hw might end up not understanding what a feature means. (old one: only knows stfl_123, new one knows stfl_123 a.k.a crazy_feat) 2. CPU model compatibility checks. We suddenly support _in this QEMU version_ CPU features for CPU models that were never defined. E.g. somewhen in the future I could say -z14,stfl_123=on just because it is a transparent feature but maybe completely fanced by ibc. Not good. I can understand that we need something like that for development. I propose something like a special cpu model for that (similar to host-passthrough).
On 01/17/2018 03:37 PM, Cornelia Huck wrote: > On Wed, 17 Jan 2018 15:18:48 +0100 > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > >> We need to handle the bpb control on reset and migration. Normally >> stfle.82 is transparent (and the normal guest part works without >> hypervisor activity). To prevent any issues we require full >> host kernel support for this feature. >> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >> --- >> target/s390x/cpu.c | 1 + >> target/s390x/cpu.h | 1 + >> target/s390x/cpu_features.c | 1 + >> target/s390x/cpu_features_def.h | 1 + >> target/s390x/gen-features.c | 1 + >> target/s390x/kvm.c | 16 ++++++++++++++++ >> target/s390x/machine.c | 17 +++++++++++++++++ >> 7 files changed, 38 insertions(+) >> >> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c >> index ae3cee9..1577b2c 100644 >> --- a/target/s390x/cpu.c >> +++ b/target/s390x/cpu.c >> @@ -89,6 +89,7 @@ static void s390_cpu_reset(CPUState *s) >> CPUS390XState *env = &cpu->env; >> >> env->pfault_token = -1UL; >> + env->bpbc = 0; >> scc->parent_reset(s); >> cpu->env.sigp_order = 0; >> s390_cpu_set_state(CPU_STATE_STOPPED, cpu); >> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h >> index 1a8b6b9..8514905 100644 >> --- a/target/s390x/cpu.h >> +++ b/target/s390x/cpu.h >> @@ -93,6 +93,7 @@ struct CPUS390XState { >> >> uint32_t fpc; /* floating-point control register */ >> uint32_t cc_op; >> + uint8_t bpbc; /* branch prediction blocking */ >> >> float_status fpu_status; /* passed to softfloat lib */ >> >> diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c >> index 31a4676..5d1c210 100644 >> --- a/target/s390x/cpu_features.c >> +++ b/target/s390x/cpu_features.c >> @@ -89,6 +89,7 @@ static const S390FeatDef s390_features[] = { >> FEAT_INIT("msa4-base", S390_FEAT_TYPE_STFL, 77, "Message-security-assist-extension-4 facility (excluding subfunctions)"), >> FEAT_INIT("edat2", S390_FEAT_TYPE_STFL, 78, "Enhanced-DAT facility 2"), >> FEAT_INIT("dfppc", S390_FEAT_TYPE_STFL, 80, "Decimal-floating-point packed-conversion facility"), >> + FEAT_INIT("bpb", S390_FEAT_TYPE_STFL, 82, "Branch Prediction Blocking"), > > No nice "facility" suffix? :) assist I think. > >> FEAT_INIT("vx", S390_FEAT_TYPE_STFL, 129, "Vector facility"), >> FEAT_INIT("iep", S390_FEAT_TYPE_STFL, 130, "Instruction-execution-protection facility"), >> FEAT_INIT("sea_esop2", S390_FEAT_TYPE_STFL, 131, "Side-effect-access facility and Enhanced-suppression-on-protection facility 2"), >> diff --git a/target/s390x/cpu_features_def.h b/target/s390x/cpu_features_def.h >> index 4b6d4e9..4487cfd 100644 >> --- a/target/s390x/cpu_features_def.h >> +++ b/target/s390x/cpu_features_def.h >> @@ -80,6 +80,7 @@ typedef enum { >> S390_FEAT_MSA_EXT_4, >> S390_FEAT_EDAT_2, >> S390_FEAT_DFP_PACKED_CONVERSION, >> + S390_FEAT_BPB, >> S390_FEAT_VECTOR, >> S390_FEAT_INSTRUCTION_EXEC_PROT, >> S390_FEAT_SIDE_EFFECT_ACCESS_ESOP2, >> diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c >> index b24f6ad..95ee870 100644 >> --- a/target/s390x/gen-features.c >> +++ b/target/s390x/gen-features.c >> @@ -357,6 +357,7 @@ static uint16_t full_GEN7_GA1[] = { >> S390_FEAT_SIE_GPERE, >> S390_FEAT_SIE_IB, >> S390_FEAT_SIE_CEI, >> + S390_FEAT_BPB, > > Will this be provided that far back on real hardware? I theory this could be backported to every machine (but its pretty unlikely). Since its just in the full model (and no default model) and kvm will only provide it if the host has it, this is the most flexible approach if this is backported further than we expected. > >> }; >> >> static uint16_t full_GEN7_GA2[] = { >> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c >> index 6a18a41..3cd4fab 100644 >> --- a/target/s390x/kvm.c >> +++ b/target/s390x/kvm.c >> @@ -490,6 +490,11 @@ int kvm_arch_put_registers(CPUState *cs, int level) >> cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_GSCB; >> } >> >> + if (can_sync_regs(cs, KVM_SYNC_BPBC)) { >> + cs->kvm_run->s.regs.bpbc = env->bpbc; >> + cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_BPBC; >> + } >> + >> /* Finally the prefix */ >> if (can_sync_regs(cs, KVM_SYNC_PREFIX)) { >> cs->kvm_run->s.regs.prefix = env->psa; >> @@ -600,6 +605,10 @@ int kvm_arch_get_registers(CPUState *cs) >> memcpy(env->gscb, cs->kvm_run->s.regs.gscb, 32); >> } >> >> + if (can_sync_regs(cs, KVM_SYNC_BPBC)) { >> + env->bpbc = cs->kvm_run->s.regs.bpbc; >> + } >> + >> /* pfault parameters */ >> if (can_sync_regs(cs, KVM_SYNC_PFAULT)) { >> env->pfault_token = cs->kvm_run->s.regs.pft; >> @@ -2278,6 +2287,13 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp) >> clear_bit(S390_FEAT_CMM_NT, model->features); >> } >> >> + /* stfle.82 is a transparent bit. As there is some state attached >> + * anyway we only enable this bit if the host kernel can handle >> + * migrate and reset */ > > Having a transparent bit with some state attached seems a bit odd... Yes, its a quirk,but the firmware fixes are already out.
On 01/17/2018 03:50 PM, David Hildenbrand wrote: > On 17.01.2018 15:37, Cornelia Huck wrote: >> On Wed, 17 Jan 2018 15:18:48 +0100 >> Christian Borntraeger <borntraeger@de.ibm.com> wrote: >> >>> We need to handle the bpb control on reset and migration. Normally >>> stfle.82 is transparent (and the normal guest part works without >>> hypervisor activity). To prevent any issues we require full >>> host kernel support for this feature. >>> >>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >>> --- >>> target/s390x/cpu.c | 1 + >>> target/s390x/cpu.h | 1 + >>> target/s390x/cpu_features.c | 1 + >>> target/s390x/cpu_features_def.h | 1 + >>> target/s390x/gen-features.c | 1 + >>> target/s390x/kvm.c | 16 ++++++++++++++++ >>> target/s390x/machine.c | 17 +++++++++++++++++ >>> 7 files changed, 38 insertions(+) >>> >>> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c >>> index ae3cee9..1577b2c 100644 >>> --- a/target/s390x/cpu.c >>> +++ b/target/s390x/cpu.c >>> @@ -89,6 +89,7 @@ static void s390_cpu_reset(CPUState *s) >>> CPUS390XState *env = &cpu->env; >>> >>> env->pfault_token = -1UL; >>> + env->bpbc = 0; >>> scc->parent_reset(s); >>> cpu->env.sigp_order = 0; >>> s390_cpu_set_state(CPU_STATE_STOPPED, cpu); >>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h >>> index 1a8b6b9..8514905 100644 >>> --- a/target/s390x/cpu.h >>> +++ b/target/s390x/cpu.h >>> @@ -93,6 +93,7 @@ struct CPUS390XState { >>> >>> uint32_t fpc; /* floating-point control register */ >>> uint32_t cc_op; >>> + uint8_t bpbc; /* branch prediction blocking */ >>> >>> float_status fpu_status; /* passed to softfloat lib */ >>> >>> diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c >>> index 31a4676..5d1c210 100644 >>> --- a/target/s390x/cpu_features.c >>> +++ b/target/s390x/cpu_features.c >>> @@ -89,6 +89,7 @@ static const S390FeatDef s390_features[] = { >>> FEAT_INIT("msa4-base", S390_FEAT_TYPE_STFL, 77, "Message-security-assist-extension-4 facility (excluding subfunctions)"), >>> FEAT_INIT("edat2", S390_FEAT_TYPE_STFL, 78, "Enhanced-DAT facility 2"), >>> FEAT_INIT("dfppc", S390_FEAT_TYPE_STFL, 80, "Decimal-floating-point packed-conversion facility"), >>> + FEAT_INIT("bpb", S390_FEAT_TYPE_STFL, 82, "Branch Prediction Blocking"), >> >> No nice "facility" suffix? :) >> >>> FEAT_INIT("vx", S390_FEAT_TYPE_STFL, 129, "Vector facility"), >>> FEAT_INIT("iep", S390_FEAT_TYPE_STFL, 130, "Instruction-execution-protection facility"), >>> FEAT_INIT("sea_esop2", S390_FEAT_TYPE_STFL, 131, "Side-effect-access facility and Enhanced-suppression-on-protection facility 2"), >>> diff --git a/target/s390x/cpu_features_def.h b/target/s390x/cpu_features_def.h >>> index 4b6d4e9..4487cfd 100644 >>> --- a/target/s390x/cpu_features_def.h >>> +++ b/target/s390x/cpu_features_def.h >>> @@ -80,6 +80,7 @@ typedef enum { >>> S390_FEAT_MSA_EXT_4, >>> S390_FEAT_EDAT_2, >>> S390_FEAT_DFP_PACKED_CONVERSION, >>> + S390_FEAT_BPB, >>> S390_FEAT_VECTOR, >>> S390_FEAT_INSTRUCTION_EXEC_PROT, >>> S390_FEAT_SIDE_EFFECT_ACCESS_ESOP2, >>> diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c >>> index b24f6ad..95ee870 100644 >>> --- a/target/s390x/gen-features.c >>> +++ b/target/s390x/gen-features.c >>> @@ -357,6 +357,7 @@ static uint16_t full_GEN7_GA1[] = { >>> S390_FEAT_SIE_GPERE, >>> S390_FEAT_SIE_IB, >>> S390_FEAT_SIE_CEI, >>> + S390_FEAT_BPB, >> >> Will this be provided that far back on real hardware? >> >>> }; >>> >>> static uint16_t full_GEN7_GA2[] = { >>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c >>> index 6a18a41..3cd4fab 100644 >>> --- a/target/s390x/kvm.c >>> +++ b/target/s390x/kvm.c >>> @@ -490,6 +490,11 @@ int kvm_arch_put_registers(CPUState *cs, int level) >>> cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_GSCB; >>> } >>> >>> + if (can_sync_regs(cs, KVM_SYNC_BPBC)) { >>> + cs->kvm_run->s.regs.bpbc = env->bpbc; >>> + cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_BPBC; >>> + } >>> + >>> /* Finally the prefix */ >>> if (can_sync_regs(cs, KVM_SYNC_PREFIX)) { >>> cs->kvm_run->s.regs.prefix = env->psa; >>> @@ -600,6 +605,10 @@ int kvm_arch_get_registers(CPUState *cs) >>> memcpy(env->gscb, cs->kvm_run->s.regs.gscb, 32); >>> } >>> >>> + if (can_sync_regs(cs, KVM_SYNC_BPBC)) { >>> + env->bpbc = cs->kvm_run->s.regs.bpbc; >>> + } >>> + >>> /* pfault parameters */ >>> if (can_sync_regs(cs, KVM_SYNC_PFAULT)) { >>> env->pfault_token = cs->kvm_run->s.regs.pft; >>> @@ -2278,6 +2287,13 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp) >>> clear_bit(S390_FEAT_CMM_NT, model->features); >>> } >>> >>> + /* stfle.82 is a transparent bit. As there is some state attached >>> + * anyway we only enable this bit if the host kernel can handle >>> + * migrate and reset */ >> >> Having a transparent bit with some state attached seems a bit odd... >> > > And exactly for this reason I tend to nack patch nr 3 (if that is of any > weight :) ). I have communicated the mistake to asll relevant parties - it will not happen again (famous last words). > > As soon as we enable bits for CPU models, we guarantee that migration > works. While introducing this change we already had one example where > this was not the case. Not good. (and remember having another such > exception) The point is migration continues to work. In fact I had a different version of this patch set that did it the other way around. Keep 82 a transparent and add a new cpu misc facility that takes care of the migration state. > > It is easier to patch a feature in than silently enabling *anything* > somebody thinks is transparent (but its not). Especially not for the > host model. The expanded host model is migration safe. I really do not care about -cpu host (host-passthrough) for migration safety, because its not. And as you said: host-model (expanded) will work. > > And as we saw, in the unlikely event of such heavy changes, we need to > touch fw/linux/qemu either way. > > But there is more I dislike about the approach in patch 3: > > 1. feature names. We need aliases. Different QEMU versions on the same > hw might end up not understanding what a feature means. (old one: only > knows stfl_123, new one knows stfl_123 a.k.a crazy_feat) I plan to keep the old names. e.g. stfle131 is better than sea_esop2. > > 2. CPU model compatibility checks. We suddenly support _in this QEMU > version_ CPU features for CPU models that were never defined. we defined it. It just have a canonical name stfle* > > E.g. somewhen in the future I could say -z14,stfl_123=on > > just because it is a transparent feature but maybe completely fanced by > ibc. Not good. > > I can understand that we need something like that for development. I > propose something like a special cpu model for that (similar to > host-passthrough). >
>> And exactly for this reason I tend to nack patch nr 3 (if that is of any >> weight :) ). > > I have communicated the mistake to asll relevant parties - it will not happen again > (famous last words). An I already saw it happen in the past. (I think I really have to dig out that one feature to make a point :P ). Mistakes happen, but we don't have to propagate them to customers if we can catch them early :) > >> >> As soon as we enable bits for CPU models, we guarantee that migration >> works. While introducing this change we already had one example where >> this was not the case. Not good. (and remember having another such >> exception) > > The point is migration continues to work. In fact I had a different version > of this patch set that did it the other way around. Keep 82 a transparent > and add a new cpu misc facility that takes care of the migration state. >> >> It is easier to patch a feature in than silently enabling *anything* >> somebody thinks is transparent (but its not). Especially not for the >> host model. The expanded host model is migration safe. > > I really do not care about -cpu host (host-passthrough) for migration safety, > because its not. And as you said: host-model (expanded) will work. > It will if the world would be perfect. expand "-cpu host" -> -cpu z14-base,stfle_82=on stfle_82 would now not be properly migrated. Yes, it might work somehow right now. But it is not clean. >> >> And as we saw, in the unlikely event of such heavy changes, we need to >> touch fw/linux/qemu either way. >> >> But there is more I dislike about the approach in patch 3: >> >> 1. feature names. We need aliases. Different QEMU versions on the same >> hw might end up not understanding what a feature means. (old one: only >> knows stfl_123, new one knows stfl_123 a.k.a crazy_feat) > > I plan to keep the old names. e.g. stfle131 is better than sea_esop2. Oh god no. With vx, te, iep one at least has a rough idea what is happening. -cpu z14-base,stfle123,stfle234,stfle323 ... :( This all smells like a huge hack for a scenario that happened once. I prefer to do it the clean way. Enable only what you checked works and what you can actually give a name. Especially we will lose the ability to know which bit was valid for which hardware generation - which is key when working with IBC. I am not sure if giving all that up is worth it.
On 01/17/2018 04:10 PM, David Hildenbrand wrote: > >>> And exactly for this reason I tend to nack patch nr 3 (if that is of any >>> weight :) ). >> >> I have communicated the mistake to asll relevant parties - it will not happen again >> (famous last words). > > An I already saw it happen in the past. (I think I really have to dig > out that one feature to make a point :P ). Mistakes happen, but we don't > have to propagate them to customers if we can catch them early :) > >> >>> >>> As soon as we enable bits for CPU models, we guarantee that migration >>> works. While introducing this change we already had one example where >>> this was not the case. Not good. (and remember having another such >>> exception) >> >> The point is migration continues to work. In fact I had a different version >> of this patch set that did it the other way around. Keep 82 a transparent >> and add a new cpu misc facility that takes care of the migration state. >>> >>> It is easier to patch a feature in than silently enabling *anything* >>> somebody thinks is transparent (but its not). Especially not for the >>> host model. The expanded host model is migration safe. >> >> I really do not care about -cpu host (host-passthrough) for migration safety, >> because its not. And as you said: host-model (expanded) will work. >> > > It will if the world would be perfect. > > expand "-cpu host" -> -cpu z14-base,stfle_82=on > > stfle_82 would now not be properly migrated. Yes, it might work somehow > right now. But it is not clean. > >>> >>> And as we saw, in the unlikely event of such heavy changes, we need to >>> touch fw/linux/qemu either way. >>> >>> But there is more I dislike about the approach in patch 3: >>> >>> 1. feature names. We need aliases. Different QEMU versions on the same >>> hw might end up not understanding what a feature means. (old one: only >>> knows stfl_123, new one knows stfl_123 a.k.a crazy_feat) >> >> I plan to keep the old names. e.g. stfle131 is better than sea_esop2. > > > Oh god no. With vx, te, iep one at least has a rough idea what is happening. > > -cpu z14-base,stfle123,stfle234,stfle323 ... :( > > > This all smells like a huge hack for a scenario that happened once. I > prefer to do it the clean way. Enable only what you checked works and > what you can actually give a name. > > Especially we will lose the ability to know which bit was valid for > which hardware generation - which is key when working with IBC. > > I am not sure if giving all that up is worth it. > I will spin up a second patch that enables stfle81 and name it "ppa15". We can then discuss patch 3 on the slow path with enough time to think about this.
On 17.01.2018 17:04, Christian Borntraeger wrote: > > > On 01/17/2018 04:10 PM, David Hildenbrand wrote: >> >>>> And exactly for this reason I tend to nack patch nr 3 (if that is of any >>>> weight :) ). >>> >>> I have communicated the mistake to asll relevant parties - it will not happen again >>> (famous last words). >> >> An I already saw it happen in the past. (I think I really have to dig >> out that one feature to make a point :P ). Mistakes happen, but we don't >> have to propagate them to customers if we can catch them early :) >> >>> >>>> >>>> As soon as we enable bits for CPU models, we guarantee that migration >>>> works. While introducing this change we already had one example where >>>> this was not the case. Not good. (and remember having another such >>>> exception) >>> >>> The point is migration continues to work. In fact I had a different version >>> of this patch set that did it the other way around. Keep 82 a transparent >>> and add a new cpu misc facility that takes care of the migration state. >>>> >>>> It is easier to patch a feature in than silently enabling *anything* >>>> somebody thinks is transparent (but its not). Especially not for the >>>> host model. The expanded host model is migration safe. >>> >>> I really do not care about -cpu host (host-passthrough) for migration safety, >>> because its not. And as you said: host-model (expanded) will work. >>> >> >> It will if the world would be perfect. >> >> expand "-cpu host" -> -cpu z14-base,stfle_82=on >> >> stfle_82 would now not be properly migrated. Yes, it might work somehow >> right now. But it is not clean. >> >>>> >>>> And as we saw, in the unlikely event of such heavy changes, we need to >>>> touch fw/linux/qemu either way. >>>> >>>> But there is more I dislike about the approach in patch 3: >>>> >>>> 1. feature names. We need aliases. Different QEMU versions on the same >>>> hw might end up not understanding what a feature means. (old one: only >>>> knows stfl_123, new one knows stfl_123 a.k.a crazy_feat) >>> >>> I plan to keep the old names. e.g. stfle131 is better than sea_esop2. >> >> >> Oh god no. With vx, te, iep one at least has a rough idea what is happening. >> >> -cpu z14-base,stfle123,stfle234,stfle323 ... :( >> >> >> This all smells like a huge hack for a scenario that happened once. I >> prefer to do it the clean way. Enable only what you checked works and >> what you can actually give a name. >> >> Especially we will lose the ability to know which bit was valid for >> which hardware generation - which is key when working with IBC. >> >> I am not sure if giving all that up is worth it. >> > > I will spin up a second patch that enables stfle81 and name it "ppa15". > We can then discuss patch 3 on the slow path with enough time to think > about this. > christian++ :)
On 01/17/2018 04:10 PM, David Hildenbrand wrote: >>> As soon as we enable bits for CPU models, we guarantee that migration >>> works. While introducing this change we already had one example where >>> this was not the case. Not good. (and remember having another such >>> exception) >> The point is migration continues to work. In fact I had a different version >> of this patch set that did it the other way around. Keep 82 a transparent >> and add a new cpu misc facility that takes care of the migration state. >>> It is easier to patch a feature in than silently enabling *anything* >>> somebody thinks is transparent (but its not). Especially not for the >>> host model. The expanded host model is migration safe. >> I really do not care about -cpu host (host-passthrough) for migration safety, >> because its not. And as you said: host-model (expanded) will work. >> > It will if the world would be perfect. > > expand "-cpu host" -> -cpu z14-base,stfle_82=on > > stfle_82 would now not be properly migrated. Yes, it might work somehow > right now. But it is not clean. > Based on the code (did not test) I would expect to see this in the scenario I think you are talking about David: 0. Libvirt uses cpu mode host (not host-passtrough). 1. Source: -"cpu host" expands to "-cpu z14-base,stfle_82=on" as you said the whole environment (host cpu, kvm, qemu) is supporting 82 inclusive migration 2. Target: libvirt requires "-cpu z14-base,stfle_82=on" when trying to migrate 2.1. If the target environment as a whole does not fully support 82 the model checking refuses the migration. It does not matter if the reason is QEMU missing patch #2 or lack of HW support or whatever. 2.2 If the target environment as a whole does fully support 82 the model migration works, and is safe. If the target QEMU does not have patch #2 we will end up here. But we may end up here for other reasons. Both outcomes are works as designed, as far as my understanding of the design goes. Or am I wrong? Assumed my reasoning above is correct, I really don't get what is not clean. Except 'blindly' trusting the hardware that it works as advertised (and fixing the mess only if it turns out that there is a mess). On a meta level I think I understand your concerns David to some extent. But for my taste here you are too concerned about the user shooting herself into the foot (because equipment malfunction or user error). Let me also note, that while we might very well end up propagating bugs to the user, we also give the user the means to mitigate these (e.g. by turning the buggy features explicitly off like -cpu host,stfle_82=off). Regards, Halil
On 17.01.2018 17:07, Halil Pasic wrote: > > > On 01/17/2018 04:10 PM, David Hildenbrand wrote: >>>> As soon as we enable bits for CPU models, we guarantee that migration >>>> works. While introducing this change we already had one example where >>>> this was not the case. Not good. (and remember having another such >>>> exception) >>> The point is migration continues to work. In fact I had a different version >>> of this patch set that did it the other way around. Keep 82 a transparent >>> and add a new cpu misc facility that takes care of the migration state. >>>> It is easier to patch a feature in than silently enabling *anything* >>>> somebody thinks is transparent (but its not). Especially not for the >>>> host model. The expanded host model is migration safe. >>> I really do not care about -cpu host (host-passthrough) for migration safety, >>> because its not. And as you said: host-model (expanded) will work. >>> >> It will if the world would be perfect. >> >> expand "-cpu host" -> -cpu z14-base,stfle_82=on >> >> stfle_82 would now not be properly migrated. Yes, it might work somehow >> right now. But it is not clean. >> > Please don't only focus on this case. I had different reasons why this is a bad idea (especially IBC and feature names). And this is still my position. > Based on the code (did not test) I would expect to see this in the > scenario I think you are talking about David: > > 0. Libvirt uses cpu mode host (not host-passtrough). > 1. Source: -"cpu host" expands to "-cpu z14-base,stfle_82=on" as you said > the whole environment (host cpu, kvm, qemu) is supporting 82 inclusive migration > 2. Target: libvirt requires "-cpu z14-base,stfle_82=on" when trying to migrate > 2.1. If the target environment as a whole does not fully support 82 the model > checking refuses the migration. It does not matter if the reason is QEMU missing > patch #2 or lack of HW support or whatever. Let's assume stfle_999, because this is obviously not a problem for 82. If source and target blindly forward a feature they think is migration safe, but is not (again, I am being conservative here as I was given a counter example in the very same patch set), migration does not fail but the guest might see a difference, because some state is not properly migrated. > > Assumed my reasoning above is correct, I really don't get what is not clean. > Except 'blindly' trusting the hardware that it works as advertised (and > fixing the mess only if it turns out that there is a mess). > > On a meta level I think I understand your concerns David to some extent. > But for my taste here you are too concerned about the user shooting herself > into the foot (because equipment malfunction or user error). Using the host model in QEMU shoots you in the foot. And it shouldn't. Better safe than sorry. > > Let me also note, that while we might very well end up propagating bugs > to the user, we also give the user the means to mitigate these (e.g. > by turning the buggy features explicitly off like -cpu host,stfle_82=off). Shoot first and then ask questions? :D I don't see any valid reason for this risk. Forwarding "transparent" features from KVM to QEMU - perfect idea. Forwarding "transparent" features from QEMU to the guest - not a good idea. New HW -> new features -> new QEMU CPU model. Patching in CPU features is really "out of order" ( ;) ) > > Regards, > Halil >
On Wed, 17 Jan 2018 17:04:05 +0100 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > I will spin up a second patch that enables stfle81 and name it "ppa15". > We can then discuss patch 3 on the slow path with enough time to think > about this. Sounds good to me. I really don't want to rush this, but still get the 81/82 bits to guests.
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c index ae3cee9..1577b2c 100644 --- a/target/s390x/cpu.c +++ b/target/s390x/cpu.c @@ -89,6 +89,7 @@ static void s390_cpu_reset(CPUState *s) CPUS390XState *env = &cpu->env; env->pfault_token = -1UL; + env->bpbc = 0; scc->parent_reset(s); cpu->env.sigp_order = 0; s390_cpu_set_state(CPU_STATE_STOPPED, cpu); diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h index 1a8b6b9..8514905 100644 --- a/target/s390x/cpu.h +++ b/target/s390x/cpu.h @@ -93,6 +93,7 @@ struct CPUS390XState { uint32_t fpc; /* floating-point control register */ uint32_t cc_op; + uint8_t bpbc; /* branch prediction blocking */ float_status fpu_status; /* passed to softfloat lib */ diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c index 31a4676..5d1c210 100644 --- a/target/s390x/cpu_features.c +++ b/target/s390x/cpu_features.c @@ -89,6 +89,7 @@ static const S390FeatDef s390_features[] = { FEAT_INIT("msa4-base", S390_FEAT_TYPE_STFL, 77, "Message-security-assist-extension-4 facility (excluding subfunctions)"), FEAT_INIT("edat2", S390_FEAT_TYPE_STFL, 78, "Enhanced-DAT facility 2"), FEAT_INIT("dfppc", S390_FEAT_TYPE_STFL, 80, "Decimal-floating-point packed-conversion facility"), + FEAT_INIT("bpb", S390_FEAT_TYPE_STFL, 82, "Branch Prediction Blocking"), FEAT_INIT("vx", S390_FEAT_TYPE_STFL, 129, "Vector facility"), FEAT_INIT("iep", S390_FEAT_TYPE_STFL, 130, "Instruction-execution-protection facility"), FEAT_INIT("sea_esop2", S390_FEAT_TYPE_STFL, 131, "Side-effect-access facility and Enhanced-suppression-on-protection facility 2"), diff --git a/target/s390x/cpu_features_def.h b/target/s390x/cpu_features_def.h index 4b6d4e9..4487cfd 100644 --- a/target/s390x/cpu_features_def.h +++ b/target/s390x/cpu_features_def.h @@ -80,6 +80,7 @@ typedef enum { S390_FEAT_MSA_EXT_4, S390_FEAT_EDAT_2, S390_FEAT_DFP_PACKED_CONVERSION, + S390_FEAT_BPB, S390_FEAT_VECTOR, S390_FEAT_INSTRUCTION_EXEC_PROT, S390_FEAT_SIDE_EFFECT_ACCESS_ESOP2, diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c index b24f6ad..95ee870 100644 --- a/target/s390x/gen-features.c +++ b/target/s390x/gen-features.c @@ -357,6 +357,7 @@ static uint16_t full_GEN7_GA1[] = { S390_FEAT_SIE_GPERE, S390_FEAT_SIE_IB, S390_FEAT_SIE_CEI, + S390_FEAT_BPB, }; static uint16_t full_GEN7_GA2[] = { diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c index 6a18a41..3cd4fab 100644 --- a/target/s390x/kvm.c +++ b/target/s390x/kvm.c @@ -490,6 +490,11 @@ int kvm_arch_put_registers(CPUState *cs, int level) cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_GSCB; } + if (can_sync_regs(cs, KVM_SYNC_BPBC)) { + cs->kvm_run->s.regs.bpbc = env->bpbc; + cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_BPBC; + } + /* Finally the prefix */ if (can_sync_regs(cs, KVM_SYNC_PREFIX)) { cs->kvm_run->s.regs.prefix = env->psa; @@ -600,6 +605,10 @@ int kvm_arch_get_registers(CPUState *cs) memcpy(env->gscb, cs->kvm_run->s.regs.gscb, 32); } + if (can_sync_regs(cs, KVM_SYNC_BPBC)) { + env->bpbc = cs->kvm_run->s.regs.bpbc; + } + /* pfault parameters */ if (can_sync_regs(cs, KVM_SYNC_PFAULT)) { env->pfault_token = cs->kvm_run->s.regs.pft; @@ -2278,6 +2287,13 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp) clear_bit(S390_FEAT_CMM_NT, model->features); } + /* stfle.82 is a transparent bit. As there is some state attached + * anyway we only enable this bit if the host kernel can handle + * migrate and reset */ + if (!kvm_check_extension(kvm_state, KVM_CAP_S390_BPB)) { + clear_bit(S390_FEAT_BPB, model->features); + } + /* We emulate a zPCI bus and AEN, therefore we don't need HW support */ if (pci_available) { set_bit(S390_FEAT_ZPCI, model->features); diff --git a/target/s390x/machine.c b/target/s390x/machine.c index b78f326..4aac456 100644 --- a/target/s390x/machine.c +++ b/target/s390x/machine.c @@ -194,6 +194,22 @@ const VMStateDescription vmstate_gscb = { } }; +static bool bpbc_needed(void *opaque) +{ + return s390_has_feat(S390_FEAT_BPB); +} + +const VMStateDescription vmstate_bpbc = { + .name = "cpu/bpbc", + .version_id = 1, + .minimum_version_id = 1, + .needed = bpbc_needed, + .fields = (VMStateField[]) { + VMSTATE_UINT8(env.bpbc, S390CPU), + VMSTATE_END_OF_LIST() + } +}; + const VMStateDescription vmstate_s390_cpu = { .name = "cpu", .post_load = cpu_post_load, @@ -228,6 +244,7 @@ const VMStateDescription vmstate_s390_cpu = { &vmstate_riccb, &vmstate_exval, &vmstate_gscb, + &vmstate_bpbc, NULL }, };
We need to handle the bpb control on reset and migration. Normally stfle.82 is transparent (and the normal guest part works without hypervisor activity). To prevent any issues we require full host kernel support for this feature. Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> --- target/s390x/cpu.c | 1 + target/s390x/cpu.h | 1 + target/s390x/cpu_features.c | 1 + target/s390x/cpu_features_def.h | 1 + target/s390x/gen-features.c | 1 + target/s390x/kvm.c | 16 ++++++++++++++++ target/s390x/machine.c | 17 +++++++++++++++++ 7 files changed, 38 insertions(+)