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

[Christian Borntraeger]

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).

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);