Re: [qemu-s390x] [PATCH 02/10] s390x/cpumodel: remove CSSKE from base model

[David Hildenbrand]

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 <address@hidden>
>>> ---
>>>  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