diff mbox

[2/2] s390: squash facilities_src.h into gen_facilities.c

Message ID 1478403928-20799-2-git-send-email-yamada.masahiro@socionext.com
State Accepted
Commit d1f7e8f85b5125f3785db473938f94f5f0d02c51
Headers show

Commit Message

Masahiro Yamada Nov. 6, 2016, 3:45 a.m. UTC 
We generally expect headers in arch/$(ARCH)/include/asm directory
are included from kernel sources, but facilities_src.h is not;
it is included from the arch/s390/tools/gen_facilities.c tool.

There is no reason to expose this header to the public include path.
Furthermore, facilities_src.h makes sure to be included only from
gen_facilities.c by the following:

  #ifndef S390_GEN_FACILITIES_C
  #error "This file can only be included by gen_facilities.c"
  #endif

This check can be removed by merging the two files.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

---

 arch/s390/include/asm/facilities_src.h | 80 ----------------------------------
 arch/s390/tools/gen_facilities.c       | 76 ++++++++++++++++++++++++++++++--
 2 files changed, 73 insertions(+), 83 deletions(-)
 delete mode 100644 arch/s390/include/asm/facilities_src.h

-- 
1.9.1

Comments

Heiko Carstens Nov. 7, 2016, 7:03 a.m. UTC | #1 
On Sun, Nov 06, 2016 at 12:45:28PM +0900, Masahiro Yamada wrote:
> We generally expect headers in arch/$(ARCH)/include/asm directory

> are included from kernel sources, but facilities_src.h is not;

> it is included from the arch/s390/tools/gen_facilities.c tool.

> 

> There is no reason to expose this header to the public include path.

> Furthermore, facilities_src.h makes sure to be included only from

> gen_facilities.c by the following:

> 

>   #ifndef S390_GEN_FACILITIES_C

>   #error "This file can only be included by gen_facilities.c"

>   #endif

> 

> This check can be removed by merging the two files.

> 

> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

> ---


Both patches:
Acked-by: Heiko Carstens <heiko.carstens@de.ibm.com>


Martin, can you please pick them up?
Martin Schwidefsky Nov. 7, 2016, 9:50 a.m. UTC | #2 
On Mon, 7 Nov 2016 08:03:22 +0100
Heiko Carstens <heiko.carstens@de.ibm.com> wrote:

> On Sun, Nov 06, 2016 at 12:45:28PM +0900, Masahiro Yamada wrote:

> > We generally expect headers in arch/$(ARCH)/include/asm directory

> > are included from kernel sources, but facilities_src.h is not;

> > it is included from the arch/s390/tools/gen_facilities.c tool.

> > 

> > There is no reason to expose this header to the public include path.

> > Furthermore, facilities_src.h makes sure to be included only from

> > gen_facilities.c by the following:

> > 

> >   #ifndef S390_GEN_FACILITIES_C

> >   #error "This file can only be included by gen_facilities.c"

> >   #endif

> > 

> > This check can be removed by merging the two files.

> > 

> > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

> > ---

> 

> Both patches:

> Acked-by: Heiko Carstens <heiko.carstens@de.ibm.com>

> 

> Martin, can you please pick them up?


I will add these two patches to the merge branch for 4.10.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.
Heiko Carstens Nov. 7, 2016, 1:38 p.m. UTC | #3 
On Mon, Nov 07, 2016 at 02:13:06PM +0100, Paul Bolle wrote:
> It took me some time to figure out that gen_facilities is only used to

> generate a small header file (generated/facilities.h). And that header's only

> goal is to define FACILITIES_ALS and FACILITIES_KVM.

> 

> Pasted below is an attempt to use asm/facilities.h instead of

> generated/facilities.h. That allows to drop arch/s390/tools/ entirely. I don't

> actually have an s390 machine at hand to test this, but this does build and

> the preprocessed code this generates looks sane.

> 

> (Yes, asm/facilities.h might need another level of preprocessor defines to

> become actually readable.)


The whole point of the tool is that we can use the bit numbers that are
specified within the architecture document, with the additional odd IBM bit
numbering scheme. Where the most significant bit has bit number 0(!).

> diff --git a/arch/s390/include/asm/facilities.h b/arch/s390/include/asm/facilities.h

> new file mode 100644

> index 000000000000..c87f18d29217

> --- /dev/null

> +++ b/arch/s390/include/asm/facilities.h

> @@ -0,0 +1,43 @@

> +#ifndef __ASM_FACILITIES_H

> +#define __ASM_FACILITIES_H

> +

> +#define FACILITIES_ALS \

> +	_AC(IS_BUILTIN(CONFIG_HAVE_MARCH_Z900_FEATURES), UL) << 0 |	/* N3 instructions */ \


So, this is wrong. It should be " << 63" to match the existing code.

> +#define FACILITIES_KVM \

> +	_BITUL(0)  |	/* N3 instructions */ \

> +	_BITUL(1)  |	/* z/Arch mode installed */ \

> +	_BITUL(2)  |	/* z/Arch mode active */ \

> +	_BITUL(3)  |	/* DAT-enhancement */ \

> +	_BITUL(4)  |	/* idte segment table */ \

> +	_BITUL(5)  |	/* idte region table */ \

> +	_BITUL(6)  |	/* ASN-and-LX reuse */ \

> +	_BITUL(7)  |	/* stfle */ \

> +	_BITUL(8)  |	/* enhanced-DAT 1 */ \

> +	_BITUL(9)  |	/* sense-running-status */ \

> +	_BITUL(10) |	/* conditional sske */ \

> +	_BITUL(13) |	/* ipte-range */ \

> +	_BITUL(14)  	/* nonquiescing key-setting */ \

> +	, \

> +	_BITUL(9)  |	/* transactional execution */ \

> +	_BITUL(11) |	/* access-exception-fetch/store indication */ \


And this is exactly what I want to avoid: start counting from zero again if
we cross a double word. It _must_ read 73, 75, otherwise this becomes the
unmaintainable and error prone mess we had before.

I just want a list with bit numbers and the rest must be created
automatically.
Paul Bolle Nov. 8, 2016, 9:18 a.m. UTC | #4 
On Mon, 2016-11-07 at 14:38 +0100, Heiko Carstens wrote:
> On Mon, Nov 07, 2016 at 02:13:06PM +0100, Paul Bolle wrote:

> > --- /dev/null

> > +++ b/arch/s390/include/asm/facilities.h

> > @@ -0,0 +1,43 @@

> > +#ifndef __ASM_FACILITIES_H

> > +#define __ASM_FACILITIES_H

> > +

> > +#define FACILITIES_ALS \

> > +	_AC(IS_BUILTIN(CONFIG_HAVE_MARCH_Z900_FEATURES), UL) << 0 |	/* N3 instructions */ \

> 

> So, this is wrong. It should be " << 63" to match the existing code.


Bother.

> > +#define FACILITIES_KVM \

> > +	_BITUL(0)  |	/* N3 instructions */ \

> > +	_BITUL(1)  |	/* z/Arch mode installed */ \

> > +	_BITUL(2)  |	/* z/Arch mode active */ \

> > +	_BITUL(3)  |	/* DAT-enhancement */ \

> > +	_BITUL(4)  |	/* idte segment table */ \

> > +	_BITUL(5)  |	/* idte region table */ \

> > +	_BITUL(6)  |	/* ASN-and-LX reuse */ \

> > +	_BITUL(7)  |	/* stfle */ \

> > +	_BITUL(8)  |	/* enhanced-DAT 1 */ \

> > +	_BITUL(9)  |	/* sense-running-status */ \

> > +	_BITUL(10) |	/* conditional sske */ \

> > +	_BITUL(13) |	/* ipte-range */ \

> > +	_BITUL(14)  	/* nonquiescing key-setting */ \

> > +	, \

> > +	_BITUL(9)  |	/* transactional execution */ \

> > +	_BITUL(11) |	/* access-exception-fetch/store indication */ \

> 

> And this is exactly what I want to avoid: start counting from zero again if

> we cross a double word. It _must_ read 73, 75, otherwise this becomes the

> unmaintainable and error prone mess we had before.

> 

> I just want a list with bit numbers and the rest must be created

> automatically.


This sounds like a can of worms. Better keep it closed.

Thanks,


Paul Bolle
diff mbox

Patch

diff --git a/arch/s390/include/asm/facilities_src.h b/arch/s390/include/asm/facilities_src.h
deleted file mode 100644
index 8b1a692..0000000
--- a/arch/s390/include/asm/facilities_src.h
+++ /dev/null
@@ -1,80 +0,0 @@ 
-/*
- *    Copyright IBM Corp. 2015
- */
-
-#ifndef S390_GEN_FACILITIES_C
-#error "This file can only be included by gen_facilities.c"
-#endif
-
-struct facility_def {
-	char *name;
-	int *bits;
-};
-
-static struct facility_def facility_defs[] = {
-	{
-		/*
-		 * FACILITIES_ALS contains the list of facilities that are
-		 * required to run a kernel that is compiled e.g. with
-		 * -march=<machine>.
-		 */
-		.name = "FACILITIES_ALS",
-		.bits = (int[]){
-#ifdef CONFIG_HAVE_MARCH_Z900_FEATURES
-			0,  /* N3 instructions */
-			1,  /* z/Arch mode installed */
-#endif
-#ifdef CONFIG_HAVE_MARCH_Z990_FEATURES
-			18, /* long displacement facility */
-#endif
-#ifdef CONFIG_HAVE_MARCH_Z9_109_FEATURES
-			7,  /* stfle */
-			17, /* message security assist */
-			21, /* extended-immediate facility */
-			25, /* store clock fast */
-#endif
-#ifdef CONFIG_HAVE_MARCH_Z10_FEATURES
-			27, /* mvcos */
-			32, /* compare and swap and store */
-			33, /* compare and swap and store 2 */
-			34, /* general extension facility */
-			35, /* execute extensions */
-#endif
-#ifdef CONFIG_HAVE_MARCH_Z196_FEATURES
-			45, /* fast-BCR, etc. */
-#endif
-#ifdef CONFIG_HAVE_MARCH_ZEC12_FEATURES
-			49, /* misc-instruction-extensions */
-			52, /* interlocked facility 2 */
-#endif
-#ifdef CONFIG_HAVE_MARCH_Z13_FEATURES
-			53, /* load-and-zero-rightmost-byte, etc. */
-#endif
-			-1 /* END */
-		}
-	},
-	{
-		.name = "FACILITIES_KVM",
-		.bits = (int[]){
-			0,  /* N3 instructions */
-			1,  /* z/Arch mode installed */
-			2,  /* z/Arch mode active */
-			3,  /* DAT-enhancement */
-			4,  /* idte segment table */
-			5,  /* idte region table */
-			6,  /* ASN-and-LX reuse */
-			7,  /* stfle */
-			8,  /* enhanced-DAT 1 */
-			9,  /* sense-running-status */
-			10, /* conditional sske */
-			13, /* ipte-range */
-			14, /* nonquiescing key-setting */
-			73, /* transactional execution */
-			75, /* access-exception-fetch/store indication */
-			76, /* msa extension 3 */
-			77, /* msa extension 4 */
-			78, /* enhanced-DAT 2 */
-			-1  /* END */
-		}
-	},
-};
diff --git a/arch/s390/tools/gen_facilities.c b/arch/s390/tools/gen_facilities.c
index fe4e6c9..8cc53b1 100644
--- a/arch/s390/tools/gen_facilities.c
+++ b/arch/s390/tools/gen_facilities.c
@@ -7,13 +7,83 @@ 
  *
  */
 
-#define S390_GEN_FACILITIES_C
-
 #include <strings.h>
 #include <string.h>
 #include <stdlib.h>
 #include <stdio.h>
-#include <asm/facilities_src.h>
+
+struct facility_def {
+	char *name;
+	int *bits;
+};
+
+static struct facility_def facility_defs[] = {
+	{
+		/*
+		 * FACILITIES_ALS contains the list of facilities that are
+		 * required to run a kernel that is compiled e.g. with
+		 * -march=<machine>.
+		 */
+		.name = "FACILITIES_ALS",
+		.bits = (int[]){
+#ifdef CONFIG_HAVE_MARCH_Z900_FEATURES
+			0,  /* N3 instructions */
+			1,  /* z/Arch mode installed */
+#endif
+#ifdef CONFIG_HAVE_MARCH_Z990_FEATURES
+			18, /* long displacement facility */
+#endif
+#ifdef CONFIG_HAVE_MARCH_Z9_109_FEATURES
+			7,  /* stfle */
+			17, /* message security assist */
+			21, /* extended-immediate facility */
+			25, /* store clock fast */
+#endif
+#ifdef CONFIG_HAVE_MARCH_Z10_FEATURES
+			27, /* mvcos */
+			32, /* compare and swap and store */
+			33, /* compare and swap and store 2 */
+			34, /* general extension facility */
+			35, /* execute extensions */
+#endif
+#ifdef CONFIG_HAVE_MARCH_Z196_FEATURES
+			45, /* fast-BCR, etc. */
+#endif
+#ifdef CONFIG_HAVE_MARCH_ZEC12_FEATURES
+			49, /* misc-instruction-extensions */
+			52, /* interlocked facility 2 */
+#endif
+#ifdef CONFIG_HAVE_MARCH_Z13_FEATURES
+			53, /* load-and-zero-rightmost-byte, etc. */
+#endif
+			-1 /* END */
+		}
+	},
+	{
+		.name = "FACILITIES_KVM",
+		.bits = (int[]){
+			0,  /* N3 instructions */
+			1,  /* z/Arch mode installed */
+			2,  /* z/Arch mode active */
+			3,  /* DAT-enhancement */
+			4,  /* idte segment table */
+			5,  /* idte region table */
+			6,  /* ASN-and-LX reuse */
+			7,  /* stfle */
+			8,  /* enhanced-DAT 1 */
+			9,  /* sense-running-status */
+			10, /* conditional sske */
+			13, /* ipte-range */
+			14, /* nonquiescing key-setting */
+			73, /* transactional execution */
+			75, /* access-exception-fetch/store indication */
+			76, /* msa extension 3 */
+			77, /* msa extension 4 */
+			78, /* enhanced-DAT 2 */
+			-1  /* END */
+		}
+	},
+};
 
 static void print_facility_list(struct facility_def *def)
 {