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

[David Hildenbrand]

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.

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

-- 

Thanks,

David / dhildenb