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 <borntrae...@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.
--
Thanks,
David / dhildenb
