[Qemu-devel] [PATCH 1/1] s390x/cpumodel: fix feature groups and breakage of MSA8

[Qemu-devel] [PATCH 1/1] s390x/cpumodel: fix feature groups and breakage of MSA8

[Christian Borntraeger]

Since commit 46a99c9f73c7 ("s390x/cpumodel: model PTFF subfunctions
for Multiple-epoch facility") -cpu help no longer shows the MSA8
feature group. Turns out that we forgot to add the new MEPOCH_PTFF
group enum.

Fixes: 46a99c9f73c7 ("s390x/cpumodel: model PTFF subfunctions for Multiple-epoch facility")
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 target/s390x/cpu_features.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/s390x/cpu_features.h b/target/s390x/cpu_features.h
index e306aa7ab2..968b12fdfe 100644
--- a/target/s390x/cpu_features.h
+++ b/target/s390x/cpu_features.h
@@ -78,6 +78,7 @@ typedef enum {
     S390_FEAT_GROUP_MSA_EXT_6,
     S390_FEAT_GROUP_MSA_EXT_7,
     S390_FEAT_GROUP_MSA_EXT_8,
+    S390_FEAT_GROUP_MULTIPLE_EPOCH_PTFF,
     S390_FEAT_GROUP_MAX,
 } S390FeatGroup;
 
-- 
2.14.3

Re: [Qemu-devel] [PATCH 1/1] s390x/cpumodel: fix feature groups and breakage of MSA8

[Christian Borntraeger]

David, Jason, Michael,

the cpumodel code is somewhat fragile as we have to add maintain things
in multiple places. I would like to have more robust code, e.g. by either
generating more or by having build bug_ons or something like that. 
Any idea is highly welcome.

Christian


On 03/20/2018 02:07 PM, Christian Borntraeger wrote:
> Since commit 46a99c9f73c7 ("s390x/cpumodel: model PTFF subfunctions
> for Multiple-epoch facility") -cpu help no longer shows the MSA8
> feature group. Turns out that we forgot to add the new MEPOCH_PTFF
> group enum.
> 
> Fixes: 46a99c9f73c7 ("s390x/cpumodel: model PTFF subfunctions for Multiple-epoch facility")
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  target/s390x/cpu_features.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/target/s390x/cpu_features.h b/target/s390x/cpu_features.h
> index e306aa7ab2..968b12fdfe 100644
> --- a/target/s390x/cpu_features.h
> +++ b/target/s390x/cpu_features.h
> @@ -78,6 +78,7 @@ typedef enum {
>      S390_FEAT_GROUP_MSA_EXT_6,
>      S390_FEAT_GROUP_MSA_EXT_7,
>      S390_FEAT_GROUP_MSA_EXT_8,
> +    S390_FEAT_GROUP_MULTIPLE_EPOCH_PTFF,
>      S390_FEAT_GROUP_MAX,
>  } S390FeatGroup;
> 

Re: [Qemu-devel] [PATCH 1/1] s390x/cpumodel: fix feature groups and breakage of MSA8

[Michael Mueller]

[-- Attachment #1: Type: text/plain, Size: 1373 bytes --]

I could imagine something like this, see attachment.

Michael

On 20.03.18 14:17, Christian Borntraeger wrote:
> David, Jason, Michael,
> 
> the cpumodel code is somewhat fragile as we have to add maintain things
> in multiple places. I would like to have more robust code, e.g. by either
> generating more or by having build bug_ons or something like that.
> Any idea is highly welcome.
> 
> Christian
> 
> 
> On 03/20/2018 02:07 PM, Christian Borntraeger wrote:
>> Since commit 46a99c9f73c7 ("s390x/cpumodel: model PTFF subfunctions
>> for Multiple-epoch facility") -cpu help no longer shows the MSA8
>> feature group. Turns out that we forgot to add the new MEPOCH_PTFF
>> group enum.
>>
>> Fixes: 46a99c9f73c7 ("s390x/cpumodel: model PTFF subfunctions for Multiple-epoch facility")
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>   target/s390x/cpu_features.h | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/target/s390x/cpu_features.h b/target/s390x/cpu_features.h
>> index e306aa7ab2..968b12fdfe 100644
>> --- a/target/s390x/cpu_features.h
>> +++ b/target/s390x/cpu_features.h
>> @@ -78,6 +78,7 @@ typedef enum {
>>       S390_FEAT_GROUP_MSA_EXT_6,
>>       S390_FEAT_GROUP_MSA_EXT_7,
>>       S390_FEAT_GROUP_MSA_EXT_8,
>> +    S390_FEAT_GROUP_MULTIPLE_EPOCH_PTFF,
>>       S390_FEAT_GROUP_MAX,
>>   } S390FeatGroup;
>>
> 
> 

[-- Attachment #2: 0001-target-s390x-enum-type-S390FeatGroup-now-gets-genera.patch --]
[-- Type: text/plain, Size: 4436 bytes --]

From 978d0b480824248586149ed67dca7b31c6a5e5d8 Mon Sep 17 00:00:00 2001
From: Michael Mueller <mimu@linux.vnet.ibm.com>
Date: Tue, 20 Mar 2018 15:56:18 +0100
Subject: [PATCH] target/s390x: enum type S390FeatGroup now gets generated

The enumeration type S390FeatGroup is now gererated as well.
This shall simplify the definition of new feature groups
without modifying existing code.

Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
---
 target/s390x/cpu_features.c |  1 -
 target/s390x/cpu_features.h | 18 +-----------------
 target/s390x/gen-features.c | 18 +++++++++++++++++-
 3 files changed, 18 insertions(+), 19 deletions(-)

diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
index 3b9e2745e9..d623db34a6 100644
--- a/target/s390x/cpu_features.c
+++ b/target/s390x/cpu_features.c
@@ -13,7 +13,6 @@
 #include "qemu/osdep.h"
 #include "qemu/module.h"
 #include "cpu_features.h"
-#include "gen-features.h"
 
 #define FEAT_INIT(_name, _type, _bit, _desc) \
     {                                                \
diff --git a/target/s390x/cpu_features.h b/target/s390x/cpu_features.h
index e306aa7ab2..effe790271 100644
--- a/target/s390x/cpu_features.h
+++ b/target/s390x/cpu_features.h
@@ -16,6 +16,7 @@
 
 #include "qemu/bitmap.h"
 #include "cpu_features_def.h"
+#include "gen-features.h"
 
 /* CPU features are announced via different ways */
 typedef enum {
@@ -64,23 +65,6 @@ void s390_add_from_feat_block(S390FeatBitmap features, S390FeatType type,
 void s390_feat_bitmap_to_ascii(const S390FeatBitmap features, void *opaque,
                                void (*fn)(const char *name, void *opaque));
 
-/* static groups that will never change */
-typedef enum {
-    S390_FEAT_GROUP_PLO,
-    S390_FEAT_GROUP_TOD_CLOCK_STEERING,
-    S390_FEAT_GROUP_GEN13_PTFF_ENH,
-    S390_FEAT_GROUP_MSA,
-    S390_FEAT_GROUP_MSA_EXT_1,
-    S390_FEAT_GROUP_MSA_EXT_2,
-    S390_FEAT_GROUP_MSA_EXT_3,
-    S390_FEAT_GROUP_MSA_EXT_4,
-    S390_FEAT_GROUP_MSA_EXT_5,
-    S390_FEAT_GROUP_MSA_EXT_6,
-    S390_FEAT_GROUP_MSA_EXT_7,
-    S390_FEAT_GROUP_MSA_EXT_8,
-    S390_FEAT_GROUP_MAX,
-} S390FeatGroup;
-
 /* Definition of a CPU feature group */
 typedef struct {
     const char *name;       /* name exposed to the user */
diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
index 0cdbc15378..94602f4386 100644
--- a/target/s390x/gen-features.c
+++ b/target/s390x/gen-features.c
@@ -659,6 +659,7 @@ static CpuFeatDefSpec CpuFeatDef[] = {
 #define FEAT_GROUP_INITIALIZER(_name)                  \
     {                                                  \
         .name = "S390_FEAT_GROUP_LIST_" #_name,        \
+        .enum_name = "S390_FEAT_GROUP_" #_name,        \
         .bits =                                        \
             { .data = group_##_name,                   \
               .len = ARRAY_SIZE(group_##_name) },      \
@@ -666,6 +667,7 @@ static CpuFeatDefSpec CpuFeatDef[] = {
 
 typedef struct {
     const char *name;
+    const char *enum_name;
     BitSpec bits;
 } FeatGroupDefSpec;
 
@@ -676,7 +678,6 @@ static FeatGroupDefSpec FeatGroupDef[] = {
     FEAT_GROUP_INITIALIZER(PLO),
     FEAT_GROUP_INITIALIZER(TOD_CLOCK_STEERING),
     FEAT_GROUP_INITIALIZER(GEN13_PTFF),
-    FEAT_GROUP_INITIALIZER(MULTIPLE_EPOCH_PTFF),
     FEAT_GROUP_INITIALIZER(MSA),
     FEAT_GROUP_INITIALIZER(MSA_EXT_1),
     FEAT_GROUP_INITIALIZER(MSA_EXT_2),
@@ -686,6 +687,7 @@ static FeatGroupDefSpec FeatGroupDef[] = {
     FEAT_GROUP_INITIALIZER(MSA_EXT_6),
     FEAT_GROUP_INITIALIZER(MSA_EXT_7),
     FEAT_GROUP_INITIALIZER(MSA_EXT_8),
+    FEAT_GROUP_INITIALIZER(MULTIPLE_EPOCH_PTFF),
 };
 
 #define QEMU_FEAT_INITIALIZER(_name)                   \
@@ -808,6 +810,19 @@ static void print_feature_group_defs(void)
     }
 }
 
+static void print_feature_group_enum_type(void)
+{
+    int i;
+
+    printf("\n/* CPU feature group enum type */\n");
+    printf("typedef enum {\n");
+    for (i = 0; i < ARRAY_SIZE(FeatGroupDef); i++) {
+        printf("\t%s,\n", FeatGroupDef[i].enum_name);
+    }
+    printf("\tS390_FEAT_GROUP_MAX,\n");
+    printf("} S390FeatGroup;\n");
+}
+
 int main(int argc, char *argv[])
 {
     printf("/*\n"
@@ -824,6 +839,7 @@ int main(int argc, char *argv[])
     print_feature_defs();
     print_feature_group_defs();
     print_qemu_feature_defs();
+    print_feature_group_enum_type();
     printf("\n#endif\n");
     return 0;
 }
-- 
2.13.4

Re: [Qemu-devel] [qemu-s390x] [PATCH 1/1] s390x/cpumodel: fix feature groups and breakage of MSA8

[Christian Borntraeger]

On 03/20/2018 04:06 PM, Michael Mueller wrote:
> I could imagine something like this, see attachment.

Unless Conny is willing to take that also for 2.12, this looks like 
a good thing to do after 2.12. (I suggest to go with my mininal
patch for 2.12)

> 
> Michael
> 
> On 20.03.18 14:17, Christian Borntraeger wrote:
>> David, Jason, Michael,
>>
>> the cpumodel code is somewhat fragile as we have to add maintain things
>> in multiple places. I would like to have more robust code, e.g. by either
>> generating more or by having build bug_ons or something like that.
>> Any idea is highly welcome.
>>
>> Christian
>>
>>
>> On 03/20/2018 02:07 PM, Christian Borntraeger wrote:
>>> Since commit 46a99c9f73c7 ("s390x/cpumodel: model PTFF subfunctions
>>> for Multiple-epoch facility") -cpu help no longer shows the MSA8
>>> feature group. Turns out that we forgot to add the new MEPOCH_PTFF
>>> group enum.
>>>
>>> Fixes: 46a99c9f73c7 ("s390x/cpumodel: model PTFF subfunctions for Multiple-epoch facility")
>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>> ---
>>>   target/s390x/cpu_features.h | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/target/s390x/cpu_features.h b/target/s390x/cpu_features.h
>>> index e306aa7ab2..968b12fdfe 100644
>>> --- a/target/s390x/cpu_features.h
>>> +++ b/target/s390x/cpu_features.h
>>> @@ -78,6 +78,7 @@ typedef enum {
>>>       S390_FEAT_GROUP_MSA_EXT_6,
>>>       S390_FEAT_GROUP_MSA_EXT_7,
>>>       S390_FEAT_GROUP_MSA_EXT_8,
>>> +    S390_FEAT_GROUP_MULTIPLE_EPOCH_PTFF,
>>>       S390_FEAT_GROUP_MAX,
>>>   } S390FeatGroup;
>>>
>>
>>
> 
> 0001-target-s390x-enum-type-S390FeatGroup-now-gets-genera.patch
> 
> 
> From 978d0b480824248586149ed67dca7b31c6a5e5d8 Mon Sep 17 00:00:00 2001
> From: Michael Mueller <mimu@linux.vnet.ibm.com>
> Date: Tue, 20 Mar 2018 15:56:18 +0100
> Subject: [PATCH] target/s390x: enum type S390FeatGroup now gets generated
> 
> The enumeration type S390FeatGroup is now gererated as well.
> This shall simplify the definition of new feature groups
> without modifying existing code.
> 
> Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
> ---
>  target/s390x/cpu_features.c |  1 -
>  target/s390x/cpu_features.h | 18 +-----------------
>  target/s390x/gen-features.c | 18 +++++++++++++++++-
>  3 files changed, 18 insertions(+), 19 deletions(-)
> 
> diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
> index 3b9e2745e9..d623db34a6 100644
> --- a/target/s390x/cpu_features.c
> +++ b/target/s390x/cpu_features.c
> @@ -13,7 +13,6 @@
>  #include "qemu/osdep.h"
>  #include "qemu/module.h"
>  #include "cpu_features.h"
> -#include "gen-features.h"
>  
>  #define FEAT_INIT(_name, _type, _bit, _desc) \
>      {                                                \
> diff --git a/target/s390x/cpu_features.h b/target/s390x/cpu_features.h
> index e306aa7ab2..effe790271 100644
> --- a/target/s390x/cpu_features.h
> +++ b/target/s390x/cpu_features.h
> @@ -16,6 +16,7 @@
>  
>  #include "qemu/bitmap.h"
>  #include "cpu_features_def.h"
> +#include "gen-features.h"
>  
>  /* CPU features are announced via different ways */
>  typedef enum {
> @@ -64,23 +65,6 @@ void s390_add_from_feat_block(S390FeatBitmap features, S390FeatType type,
>  void s390_feat_bitmap_to_ascii(const S390FeatBitmap features, void *opaque,
>                                 void (*fn)(const char *name, void *opaque));
>  
> -/* static groups that will never change */
> -typedef enum {
> -    S390_FEAT_GROUP_PLO,
> -    S390_FEAT_GROUP_TOD_CLOCK_STEERING,
> -    S390_FEAT_GROUP_GEN13_PTFF_ENH,
> -    S390_FEAT_GROUP_MSA,
> -    S390_FEAT_GROUP_MSA_EXT_1,
> -    S390_FEAT_GROUP_MSA_EXT_2,
> -    S390_FEAT_GROUP_MSA_EXT_3,
> -    S390_FEAT_GROUP_MSA_EXT_4,
> -    S390_FEAT_GROUP_MSA_EXT_5,
> -    S390_FEAT_GROUP_MSA_EXT_6,
> -    S390_FEAT_GROUP_MSA_EXT_7,
> -    S390_FEAT_GROUP_MSA_EXT_8,
> -    S390_FEAT_GROUP_MAX,
> -} S390FeatGroup;
> -
>  /* Definition of a CPU feature group */
>  typedef struct {
>      const char *name;       /* name exposed to the user */
> diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
> index 0cdbc15378..94602f4386 100644
> --- a/target/s390x/gen-features.c
> +++ b/target/s390x/gen-features.c
> @@ -659,6 +659,7 @@ static CpuFeatDefSpec CpuFeatDef[] = {
>  #define FEAT_GROUP_INITIALIZER(_name)                  \
>      {                                                  \
>          .name = "S390_FEAT_GROUP_LIST_" #_name,        \
> +        .enum_name = "S390_FEAT_GROUP_" #_name,        \
>          .bits =                                        \
>              { .data = group_##_name,                   \
>                .len = ARRAY_SIZE(group_##_name) },      \
> @@ -666,6 +667,7 @@ static CpuFeatDefSpec CpuFeatDef[] = {
>  
>  typedef struct {
>      const char *name;
> +    const char *enum_name;
>      BitSpec bits;
>  } FeatGroupDefSpec;
>  
> @@ -676,7 +678,6 @@ static FeatGroupDefSpec FeatGroupDef[] = {
>      FEAT_GROUP_INITIALIZER(PLO),
>      FEAT_GROUP_INITIALIZER(TOD_CLOCK_STEERING),
>      FEAT_GROUP_INITIALIZER(GEN13_PTFF),
> -    FEAT_GROUP_INITIALIZER(MULTIPLE_EPOCH_PTFF),
>      FEAT_GROUP_INITIALIZER(MSA),
>      FEAT_GROUP_INITIALIZER(MSA_EXT_1),
>      FEAT_GROUP_INITIALIZER(MSA_EXT_2),
> @@ -686,6 +687,7 @@ static FeatGroupDefSpec FeatGroupDef[] = {
>      FEAT_GROUP_INITIALIZER(MSA_EXT_6),
>      FEAT_GROUP_INITIALIZER(MSA_EXT_7),
>      FEAT_GROUP_INITIALIZER(MSA_EXT_8),
> +    FEAT_GROUP_INITIALIZER(MULTIPLE_EPOCH_PTFF),
>  };
>  
>  #define QEMU_FEAT_INITIALIZER(_name)                   \
> @@ -808,6 +810,19 @@ static void print_feature_group_defs(void)
>      }
>  }
>  
> +static void print_feature_group_enum_type(void)
> +{
> +    int i;
> +
> +    printf("\n/* CPU feature group enum type */\n");
> +    printf("typedef enum {\n");
> +    for (i = 0; i < ARRAY_SIZE(FeatGroupDef); i++) {
> +        printf("\t%s,\n", FeatGroupDef[i].enum_name);
> +    }
> +    printf("\tS390_FEAT_GROUP_MAX,\n");
> +    printf("} S390FeatGroup;\n");
> +}
> +
>  int main(int argc, char *argv[])
>  {
>      printf("/*\n"
> @@ -824,6 +839,7 @@ int main(int argc, char *argv[])
>      print_feature_defs();
>      print_feature_group_defs();
>      print_qemu_feature_defs();
> +    print_feature_group_enum_type();
>      printf("\n#endif\n");
>      return 0;
>  }
> -- 2.13.4
> 

Re: [Qemu-devel] [PATCH 1/1] s390x/cpumodel: fix feature groups and breakage of MSA8

[David Hildenbrand]

On 20.03.2018 14:07, Christian Borntraeger wrote:
> Since commit 46a99c9f73c7 ("s390x/cpumodel: model PTFF subfunctions
> for Multiple-epoch facility") -cpu help no longer shows the MSA8
> feature group. Turns out that we forgot to add the new MEPOCH_PTFF
> group enum.
> 
> Fixes: 46a99c9f73c7 ("s390x/cpumodel: model PTFF subfunctions for Multiple-epoch facility")
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  target/s390x/cpu_features.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/target/s390x/cpu_features.h b/target/s390x/cpu_features.h
> index e306aa7ab2..968b12fdfe 100644
> --- a/target/s390x/cpu_features.h
> +++ b/target/s390x/cpu_features.h
> @@ -78,6 +78,7 @@ typedef enum {
>      S390_FEAT_GROUP_MSA_EXT_6,
>      S390_FEAT_GROUP_MSA_EXT_7,
>      S390_FEAT_GROUP_MSA_EXT_8,
> +    S390_FEAT_GROUP_MULTIPLE_EPOCH_PTFF,
>      S390_FEAT_GROUP_MAX,
>  } S390FeatGroup;
>  
> 

Indeed, thanks!

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH 1/1] s390x/cpumodel: fix feature groups and breakage of MSA8

[David Hildenbrand]

On 20.03.2018 14:17, Christian Borntraeger wrote:
> David, Jason, Michael,
> 
> the cpumodel code is somewhat fragile as we have to add maintain things
> in multiple places. I would like to have more robust code, e.g. by either
> generating more or by having build bug_ons or something like that. 
> Any idea is highly welcome.
> 
> Christian

Yes, error prone. I was also wondering if we should explicitly address
array items when initializing - e.g. in target/s390x/cpu_features.c the
"indexed by feature number for easy lookup" part. More LOC but less
error prone.

As Mimu also suggested, if we can generate something automatically, we
should do that.


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH 1/1] s390x/cpumodel: fix feature groups and breakage of MSA8

[Christian Borntraeger]

On 03/20/2018 02:07 PM, Christian Borntraeger wrote:
> Since commit 46a99c9f73c7 ("s390x/cpumodel: model PTFF subfunctions
> for Multiple-epoch facility") -cpu help no longer shows the MSA8
> feature group. Turns out that we forgot to add the new MEPOCH_PTFF
> group enum.
> 
> Fixes: 46a99c9f73c7 ("s390x/cpumodel: model PTFF subfunctions for Multiple-epoch facility")
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  target/s390x/cpu_features.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/target/s390x/cpu_features.h b/target/s390x/cpu_features.h
> index e306aa7ab2..968b12fdfe 100644
> --- a/target/s390x/cpu_features.h
> +++ b/target/s390x/cpu_features.h
> @@ -78,6 +78,7 @@ typedef enum {
>      S390_FEAT_GROUP_MSA_EXT_6,
>      S390_FEAT_GROUP_MSA_EXT_7,
>      S390_FEAT_GROUP_MSA_EXT_8,
> +    S390_FEAT_GROUP_MULTIPLE_EPOCH_PTFF,
>      S390_FEAT_GROUP_MAX,
>  } S390FeatGroup;
> 

applied to my s390-next branch.