| Message ID | 20180320130702.188689-1-borntraeger@de.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] s390x/cpumodel: fix feature groups and breakage of MSA8 | expand |
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; >
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; >> > > 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; }
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 >
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>
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.
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.
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;
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(+)