| Message ID | 20190418113110.160664-3-borntraeger@de.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x: new guest features | expand |
On 18.04.19 13:31, Christian Borntraeger wrote: > conditional sske is deprecated and a distant future machine (will be one > where the IBC will not allow to fully go back to z14) will remove this > feature. To prepare for this and allow for the z14 and older cpu model > to still run on systems without csske, remove csske from the base (and will csske feature be a default feature for zNext? Or is it not available *at all*. In case it is not available, baselining and cpu model comparison have to be thought about "ignoring csske". > thus the default models for z10..z14). For compat machines we have to > add those back. Base models are machine-independent. That means, changing base models is not supported. Once we introduce new models like here, we can set the new base models into stone. > > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > hw/s390x/s390-virtio-ccw.c | 2 ++ > target/s390x/cpu_models.c | 21 +++++++++++++++++++++ > target/s390x/cpu_models.h | 1 + > target/s390x/gen-features.c | 1 - > 4 files changed, 24 insertions(+), 1 deletion(-) > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index d11069b860..3415948b2c 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -648,6 +648,8 @@ bool css_migration_enabled(void) > > static void ccw_machine_4_0_instance_options(MachineState *machine) > { > + /* re-enable csske for compat machines in the base model */ > + s390_cpumodel_fixup_csske(); > } > > static void ccw_machine_4_0_class_options(MachineClass *mc) > diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c > index eb125d4d0d..4e5aa879f3 100644 > --- a/target/s390x/cpu_models.c > +++ b/target/s390x/cpu_models.c > @@ -93,6 +93,27 @@ static S390FeatBitmap qemu_max_cpu_feat; > /* features part of a base model but not relevant for finding a base model */ > S390FeatBitmap ignored_base_feat; > > +/* > + * We removed CSSKE from the base features. This is a hook for compat machines > + * to put this back for gen10..gen14. As the base model is also part of the > + * default model to have to fixup both bitfields > + */ > +void s390_cpumodel_fixup_csske(void) > +{ > + int i; > + > + for (i = 0; i < ARRAY_SIZE(s390_cpu_defs); i++) { > + const S390CPUDef *def = &s390_cpu_defs[i]; > + > + if (def->gen < 10 || def->gen > 14) { > + continue; > + } > + > + set_bit(S390_FEAT_CONDITIONAL_SSKE, (unsigned long *)&def->base_feat); > + set_bit(S390_FEAT_CONDITIONAL_SSKE, (unsigned long *)&def->default_feat); > + } > +} I think that can be avoided by smarter generation of the models, which I would prefer. > + > void s390_cpudef_featoff(uint8_t gen, uint8_t ec_ga, S390Feat feat) > { > const S390CPUDef *def; > diff --git a/target/s390x/cpu_models.h b/target/s390x/cpu_models.h > index 174a99e561..b2e37bc8cf 100644 > --- a/target/s390x/cpu_models.h > +++ b/target/s390x/cpu_models.h > @@ -73,6 +73,7 @@ struct S390CPUModel { > #define ibc_gen(x) (x == 0 ? 0 : ((x >> 4) + S390_GEN_Z10)) > #define ibc_ec_ga(x) (x & 0xf) > > +void s390_cpumodel_fixup_csske(void); > void s390_cpudef_featoff(uint8_t gen, uint8_t ec_ga, S390Feat feat); > void s390_cpudef_featoff_greater(uint8_t gen, uint8_t ec_ga, S390Feat feat); > void s390_cpudef_group_featoff_greater(uint8_t gen, uint8_t ec_ga, > diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c > index e4739a6b9f..bea2f80c49 100644 > --- a/target/s390x/gen-features.c > +++ b/target/s390x/gen-features.c > @@ -301,7 +301,6 @@ static uint16_t base_GEN9_GA1[] = { > #define base_GEN9_GA3 EmptyFeat > > static uint16_t base_GEN10_GA1[] = { > - S390_FEAT_CONDITIONAL_SSKE, > S390_FEAT_PARSING_ENH, > S390_FEAT_MOVE_WITH_OPTIONAL_SPEC, > S390_FEAT_EXTRACT_CPU_TIME, > Instead of doing that, can we rather start generating the next generation "fresh", listing all base model features it contains instead of doing it incrementally? Could end up "nicer" In target/s390x/cpu_models.c we have: "... For now, base features of a following release are always a subset of base features of the previous release. Same is correct for the other feature sets." This is especially relevant for "s390_find_cpu_def", and goes into the direction of baselining, as previously mentioned. Luckily, we already have "ignored_base_feat", maybe we can simply add csske there and have it working.
On 18.04.19 14:45, David Hildenbrand wrote: > On 18.04.19 13:31, Christian Borntraeger wrote: >> conditional sske is deprecated and a distant future machine (will be one >> where the IBC will not allow to fully go back to z14) will remove this >> feature. To prepare for this and allow for the z14 and older cpu model >> to still run on systems without csske, remove csske from the base (and > > will csske feature be a default feature for zNext? Or is it not > available *at all*. > > In case it is not available, baselining and cpu model comparison have to > be thought about "ignoring csske". > >> thus the default models for z10..z14). For compat machines we have to >> add those back. > > Base models are machine-independent. That means, changing base models is > not supported. Why is that? for the expansion? > Once we introduce new models like here, we can set the > new base models into stone. the new model is easy (and yes I could only disable CSSKE in the base model for gen15 but not for gen14. The problem is that without some kind of fixup for older base models like z10-z14 expansion will fall back to z9 on anything that no longer has csske. > >> [....] >> > > Instead of doing that, can we rather start generating the next > generation "fresh", listing all base model features it contains instead > of doing it incrementally? Could end up "nicer" > > > In target/s390x/cpu_models.c we have: > > "... For now, base features of a following release are always a subset > of base features of the previous release. Same is correct for the other > feature sets." > > This is especially relevant for "s390_find_cpu_def", and goes into the > direction of baselining, as previously mentioned. > > Luckily, we already have "ignored_base_feat", maybe we can simply add > csske there and have it working. Yes, maybe we could ignore csske.
On 18.04.19 15:00, Christian Borntraeger wrote: > > > On 18.04.19 14:45, David Hildenbrand wrote: >> On 18.04.19 13:31, Christian Borntraeger wrote: >>> conditional sske is deprecated and a distant future machine (will be one >>> where the IBC will not allow to fully go back to z14) will remove this >>> feature. To prepare for this and allow for the z14 and older cpu model >>> to still run on systems without csske, remove csske from the base (and >> >> will csske feature be a default feature for zNext? Or is it not >> available *at all*. >> >> In case it is not available, baselining and cpu model comparison have to >> be thought about "ignoring csske". >> >>> thus the default models for z10..z14). For compat machines we have to >>> add those back. >> >> Base models are machine-independent. That means, changing base models is >> not supported. > > Why is that? for the expansion? Yes, and baselining. It's what the QAPI interface promises. > >> Once we introduce new models like here, we can set the >> new base models into stone. > > the new model is easy (and yes I could only disable CSSKE in the base > model for gen15 but not for gen14. > > The problem is that without some kind of fixup for older base models like > z10-z14 expansion will fall back to z9 on anything that no longer has > csske. I'd prefer to do generate the "right" base models right away and not fix them up at runtime. They should never change, so fixing them up at runtime feels wrong. >> Luckily, we already have "ignored_base_feat", maybe we can simply add >> csske there and have it working. > > Yes, maybe we could ignore csske. > Yes, that should at least avoid falling back to a z9.
On 18.04.19 14:45, David Hildenbrand wrote: > On 18.04.19 13:31, Christian Borntraeger wrote: >> conditional sske is deprecated and a distant future machine (will be one >> where the IBC will not allow to fully go back to z14) will remove this >> feature. To prepare for this and allow for the z14 and older cpu model >> to still run on systems without csske, remove csske from the base (and > > will csske feature be a default feature for zNext? Or is it not > available *at all*. > > In case it is not available, baselining and cpu model comparison have to > be thought about "ignoring csske". > >> thus the default models for z10..z14). For compat machines we have to >> add those back. > > Base models are machine-independent. That means, changing base models is > not supported. Once we introduce new models like here, we can set the > new base models into stone. > >> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >> --- >> hw/s390x/s390-virtio-ccw.c | 2 ++ >> target/s390x/cpu_models.c | 21 +++++++++++++++++++++ >> target/s390x/cpu_models.h | 1 + >> target/s390x/gen-features.c | 1 - >> 4 files changed, 24 insertions(+), 1 deletion(-) >> >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >> index d11069b860..3415948b2c 100644 >> --- a/hw/s390x/s390-virtio-ccw.c >> +++ b/hw/s390x/s390-virtio-ccw.c >> @@ -648,6 +648,8 @@ bool css_migration_enabled(void) >> >> static void ccw_machine_4_0_instance_options(MachineState *machine) >> { >> + /* re-enable csske for compat machines in the base model */ >> + s390_cpumodel_fixup_csske(); >> } >> >> static void ccw_machine_4_0_class_options(MachineClass *mc) >> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c >> index eb125d4d0d..4e5aa879f3 100644 >> --- a/target/s390x/cpu_models.c >> +++ b/target/s390x/cpu_models.c >> @@ -93,6 +93,27 @@ static S390FeatBitmap qemu_max_cpu_feat; >> /* features part of a base model but not relevant for finding a base model */ >> S390FeatBitmap ignored_base_feat; >> >> +/* >> + * We removed CSSKE from the base features. This is a hook for compat machines >> + * to put this back for gen10..gen14. As the base model is also part of the >> + * default model to have to fixup both bitfields >> + */ >> +void s390_cpumodel_fixup_csske(void) >> +{ >> + int i; >> + >> + for (i = 0; i < ARRAY_SIZE(s390_cpu_defs); i++) { >> + const S390CPUDef *def = &s390_cpu_defs[i]; >> + >> + if (def->gen < 10 || def->gen > 14) { >> + continue; >> + } >> + >> + set_bit(S390_FEAT_CONDITIONAL_SSKE, (unsigned long *)&def->base_feat); >> + set_bit(S390_FEAT_CONDITIONAL_SSKE, (unsigned long *)&def->default_feat); >> + } >> +} > > I think that can be avoided by smarter generation of the models, which I > would prefer. > Very rough, but something like this (I have to find a nice way to use the qemu clear_bit). diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c index 15cd9836c4..21c786127a 100644 --- a/target/s390x/gen-features.c +++ b/target/s390x/gen-features.c @@ -13,6 +13,7 @@ #include <inttypes.h> #include <stdio.h> +#include <string.h> #include "cpu_features_def.h" #define ARRAY_SIZE(array) (sizeof(array) / sizeof(array[0])) @@ -833,6 +834,16 @@ static void set_bits(uint64_t list[], BitSpec bits) } } +#define BIT_MASK(nr) (1UL << ((nr) % 64)) +#define BIT_WORD(nr) ((nr) / 64) +static inline void clear_bit(long nr, uint64_t *addr) +{ + unsigned long mask = BIT_MASK(nr); + unsigned long *p = addr + BIT_WORD(nr); + + *p &= ~mask; +} + static void print_feature_defs(void) { uint64_t base_feat[S390_FEAT_MAX / 64 + 1] = {}; @@ -843,6 +854,11 @@ static void print_feature_defs(void) printf("\n/* CPU model feature list data */\n"); for (i = 0; i < ARRAY_SIZE(CpuFeatDef); i++) { + /* With gen15 CSSKE is deprecated */ + if (strcmp(CpuFeatDef[i].name, "S390_FEAT_LIST_GEN15_GA1") == 0) { + clear_bit(S390_FEAT_CONDITIONAL_SSKE, base_feat); + clear_bit(S390_FEAT_CONDITIONAL_SSKE, default_feat); + } set_bits(base_feat, CpuFeatDef[i].base_bits); /* add the base to the default features */ set_bits(default_feat, CpuFeatDef[i].base_bits);
On 18.04.19 16:01, Christian Borntraeger wrote: > > > On 18.04.19 14:45, David Hildenbrand wrote: >> On 18.04.19 13:31, Christian Borntraeger wrote: >>> conditional sske is deprecated and a distant future machine (will be one >>> where the IBC will not allow to fully go back to z14) will remove this >>> feature. To prepare for this and allow for the z14 and older cpu model >>> to still run on systems without csske, remove csske from the base (and >> >> will csske feature be a default feature for zNext? Or is it not >> available *at all*. >> >> In case it is not available, baselining and cpu model comparison have to >> be thought about "ignoring csske". >> >>> thus the default models for z10..z14). For compat machines we have to >>> add those back. >> >> Base models are machine-independent. That means, changing base models is >> not supported. Once we introduce new models like here, we can set the >> new base models into stone. >> >>> >>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >>> --- >>> hw/s390x/s390-virtio-ccw.c | 2 ++ >>> target/s390x/cpu_models.c | 21 +++++++++++++++++++++ >>> target/s390x/cpu_models.h | 1 + >>> target/s390x/gen-features.c | 1 - >>> 4 files changed, 24 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >>> index d11069b860..3415948b2c 100644 >>> --- a/hw/s390x/s390-virtio-ccw.c >>> +++ b/hw/s390x/s390-virtio-ccw.c >>> @@ -648,6 +648,8 @@ bool css_migration_enabled(void) >>> >>> static void ccw_machine_4_0_instance_options(MachineState *machine) >>> { >>> + /* re-enable csske for compat machines in the base model */ >>> + s390_cpumodel_fixup_csske(); >>> } >>> >>> static void ccw_machine_4_0_class_options(MachineClass *mc) >>> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c >>> index eb125d4d0d..4e5aa879f3 100644 >>> --- a/target/s390x/cpu_models.c >>> +++ b/target/s390x/cpu_models.c >>> @@ -93,6 +93,27 @@ static S390FeatBitmap qemu_max_cpu_feat; >>> /* features part of a base model but not relevant for finding a base model */ >>> S390FeatBitmap ignored_base_feat; >>> >>> +/* >>> + * We removed CSSKE from the base features. This is a hook for compat machines >>> + * to put this back for gen10..gen14. As the base model is also part of the >>> + * default model to have to fixup both bitfields >>> + */ >>> +void s390_cpumodel_fixup_csske(void) >>> +{ >>> + int i; >>> + >>> + for (i = 0; i < ARRAY_SIZE(s390_cpu_defs); i++) { >>> + const S390CPUDef *def = &s390_cpu_defs[i]; >>> + >>> + if (def->gen < 10 || def->gen > 14) { >>> + continue; >>> + } >>> + >>> + set_bit(S390_FEAT_CONDITIONAL_SSKE, (unsigned long *)&def->base_feat); >>> + set_bit(S390_FEAT_CONDITIONAL_SSKE, (unsigned long *)&def->default_feat); >>> + } >>> +} >> >> I think that can be avoided by smarter generation of the models, which I >> would prefer. >> > > Very rough, but something like this (I have to find a nice way to use the qemu > clear_bit). We work on uint64_t, not unsigned long, therefore using existing bitmap operations would be theoretically wrong (and even make compilation more complicated). Just use a custom helper. > > diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c > index 15cd9836c4..21c786127a 100644 > --- a/target/s390x/gen-features.c > +++ b/target/s390x/gen-features.c > @@ -13,6 +13,7 @@ > > #include <inttypes.h> > #include <stdio.h> > +#include <string.h> > #include "cpu_features_def.h" > > #define ARRAY_SIZE(array) (sizeof(array) / sizeof(array[0])) > @@ -833,6 +834,16 @@ static void set_bits(uint64_t list[], BitSpec bits) > } > } > > +#define BIT_MASK(nr) (1UL << ((nr) % 64)) > +#define BIT_WORD(nr) ((nr) / 64) > +static inline void clear_bit(long nr, uint64_t *addr) > +{ > + unsigned long mask = BIT_MASK(nr); > + unsigned long *p = addr + BIT_WORD(nr); > + > + *p &= ~mask; > +} > + Looking at set_bits(), clear_bit() could be as simple as list[nr / 64] &= ~(1ULL << (nr % 64)); > static void print_feature_defs(void) > { > uint64_t base_feat[S390_FEAT_MAX / 64 + 1] = {}; > @@ -843,6 +854,11 @@ static void print_feature_defs(void) > printf("\n/* CPU model feature list data */\n"); > > for (i = 0; i < ARRAY_SIZE(CpuFeatDef); i++) { > + /* With gen15 CSSKE is deprecated */ > + if (strcmp(CpuFeatDef[i].name, "S390_FEAT_LIST_GEN15_GA1") == 0) { > + clear_bit(S390_FEAT_CONDITIONAL_SSKE, base_feat); > + clear_bit(S390_FEAT_CONDITIONAL_SSKE, default_feat); > + } > set_bits(base_feat, CpuFeatDef[i].base_bits); > /* add the base to the default features */ > set_bits(default_feat, CpuFeatDef[i].base_bits); > That's even better. We could also add clear_bits() with deprecation lists, similar to set_bits.
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index d11069b860..3415948b2c 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -648,6 +648,8 @@ bool css_migration_enabled(void) static void ccw_machine_4_0_instance_options(MachineState *machine) { + /* re-enable csske for compat machines in the base model */ + s390_cpumodel_fixup_csske(); } static void ccw_machine_4_0_class_options(MachineClass *mc) diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c index eb125d4d0d..4e5aa879f3 100644 --- a/target/s390x/cpu_models.c +++ b/target/s390x/cpu_models.c @@ -93,6 +93,27 @@ static S390FeatBitmap qemu_max_cpu_feat; /* features part of a base model but not relevant for finding a base model */ S390FeatBitmap ignored_base_feat; +/* + * We removed CSSKE from the base features. This is a hook for compat machines + * to put this back for gen10..gen14. As the base model is also part of the + * default model to have to fixup both bitfields + */ +void s390_cpumodel_fixup_csske(void) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(s390_cpu_defs); i++) { + const S390CPUDef *def = &s390_cpu_defs[i]; + + if (def->gen < 10 || def->gen > 14) { + continue; + } + + set_bit(S390_FEAT_CONDITIONAL_SSKE, (unsigned long *)&def->base_feat); + set_bit(S390_FEAT_CONDITIONAL_SSKE, (unsigned long *)&def->default_feat); + } +} + void s390_cpudef_featoff(uint8_t gen, uint8_t ec_ga, S390Feat feat) { const S390CPUDef *def; diff --git a/target/s390x/cpu_models.h b/target/s390x/cpu_models.h index 174a99e561..b2e37bc8cf 100644 --- a/target/s390x/cpu_models.h +++ b/target/s390x/cpu_models.h @@ -73,6 +73,7 @@ struct S390CPUModel { #define ibc_gen(x) (x == 0 ? 0 : ((x >> 4) + S390_GEN_Z10)) #define ibc_ec_ga(x) (x & 0xf) +void s390_cpumodel_fixup_csske(void); void s390_cpudef_featoff(uint8_t gen, uint8_t ec_ga, S390Feat feat); void s390_cpudef_featoff_greater(uint8_t gen, uint8_t ec_ga, S390Feat feat); void s390_cpudef_group_featoff_greater(uint8_t gen, uint8_t ec_ga, diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c index e4739a6b9f..bea2f80c49 100644 --- a/target/s390x/gen-features.c +++ b/target/s390x/gen-features.c @@ -301,7 +301,6 @@ static uint16_t base_GEN9_GA1[] = { #define base_GEN9_GA3 EmptyFeat static uint16_t base_GEN10_GA1[] = { - S390_FEAT_CONDITIONAL_SSKE, S390_FEAT_PARSING_ENH, S390_FEAT_MOVE_WITH_OPTIONAL_SPEC, S390_FEAT_EXTRACT_CPU_TIME,
conditional sske is deprecated and a distant future machine (will be one where the IBC will not allow to fully go back to z14) will remove this feature. To prepare for this and allow for the z14 and older cpu model to still run on systems without csske, remove csske from the base (and thus the default models for z10..z14). For compat machines we have to add those back. Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> --- hw/s390x/s390-virtio-ccw.c | 2 ++ target/s390x/cpu_models.c | 21 +++++++++++++++++++++ target/s390x/cpu_models.h | 1 + target/s390x/gen-features.c | 1 - 4 files changed, 24 insertions(+), 1 deletion(-)