Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1515658 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/2] s390: squash facilities_src.h into gen_facilities.c Masahiro Yamada <yamada.masahiro@socionext.com> - 2016-11-06 04:50 +0100
    Re: [PATCH 2/2] s390: squash facilities_src.h into gen_facilities.c Heiko Carstens <heiko.carstens@de.ibm.com> - 2016-11-07 08:10 +0100
      Re: [PATCH 2/2] s390: squash facilities_src.h into gen_facilities.c Martin Schwidefsky <schwidefsky@de.ibm.com> - 2016-11-07 11:00 +0100
        Re: [PATCH 2/2] s390: squash facilities_src.h into gen_facilities.c Paul Bolle <pebolle@tiscali.nl> - 2016-11-07 14:20 +0100
          Re: [PATCH 2/2] s390: squash facilities_src.h into gen_facilities.c Heiko Carstens <heiko.carstens@de.ibm.com> - 2016-11-07 14:40 +0100
            Re: [PATCH 2/2] s390: squash facilities_src.h into gen_facilities.c Paul Bolle <pebolle@tiscali.nl> - 2016-11-08 10:20 +0100

#1515658 — [PATCH 2/2] s390: squash facilities_src.h into gen_facilities.c

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

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)
 {
-- 
1.9.1

[toc] | [next] | [standalone]

#1515875

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?

[toc] | [prev] | [next] | [standalone]

#1515951

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.

[toc] | [prev] | [next] | [standalone]

#1516154

On Mon, 2016-11-07 at 10:50 +0100, Martin Schwidefsky wrote:
> 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>

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

Thanks,


Paul Bolle

diff --git a/arch/s390/Makefile b/arch/s390/Makefile
index 54e00526b8df..a0ee0a1ee677 100644
--- a/arch/s390/Makefile
+++ b/arch/s390/Makefile
@@ -108,7 +108,6 @@ drivers-y	+= drivers/s390/
 drivers-$(CONFIG_OPROFILE)	+= arch/s390/oprofile/
 
 boot		:= arch/s390/boot
-tools		:= arch/s390/tools
 
 all: image bzImage
 
@@ -127,10 +126,6 @@ vdso_install:
 
 archclean:
 	$(Q)$(MAKE) $(clean)=$(boot)
-	$(Q)$(MAKE) $(clean)=$(tools)
-
-archprepare:
-	$(Q)$(MAKE) $(build)=$(tools) include/generated/facilities.h
 
 # Don't use tabs in echo arguments
 define archhelp
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 */ \
+	_AC(IS_BUILTIN(CONFIG_HAVE_MARCH_Z900_FEATURES), UL) << 1 |	/* z/Arch mode installed */ \
+	_AC(IS_BUILTIN(CONFIG_HAVE_MARCH_Z9_109_FEATURES), UL) << 7 |	/* stfle */ \
+	_AC(IS_BUILTIN(CONFIG_HAVE_MARCH_Z9_109_FEATURES), UL) << 17 |	/* message security assist */ \
+	_AC(IS_BUILTIN(CONFIG_HAVE_MARCH_Z990_FEATURES), UL) << 18 |	/* long displacement facility */ \
+	_AC(IS_BUILTIN(CONFIG_HAVE_MARCH_Z9_109_FEATURES), UL) << 21 |	/* extended-immediate facility */ \
+	_AC(IS_BUILTIN(CONFIG_HAVE_MARCH_Z9_109_FEATURES), UL) << 25 |	/* store clock fast */ \
+	_AC(IS_BUILTIN(CONFIG_HAVE_MARCH_Z10_FEATURES), UL) << 27 |	/* mvcos */ \
+	_AC(IS_BUILTIN(CONFIG_HAVE_MARCH_Z10_FEATURES), UL) << 32 |	/* compare and swap and store */ \
+	_AC(IS_BUILTIN(CONFIG_HAVE_MARCH_Z10_FEATURES), UL) << 33 |	/* compare and swap and store 2 */ \
+	_AC(IS_BUILTIN(CONFIG_HAVE_MARCH_Z10_FEATURES), UL) << 34 |	/* general extension facility */ \
+	_AC(IS_BUILTIN(CONFIG_HAVE_MARCH_Z10_FEATURES), UL) << 35 |	/* execute extensions */ \
+	_AC(IS_BUILTIN(CONFIG_HAVE_MARCH_Z196_FEATURES), UL) << 45 |	/* fast-BCR, etc. */ \
+	_AC(IS_BUILTIN(CONFIG_HAVE_MARCH_ZEC12_FEATURES), UL) << 49 |	/* misc-instruction-extensions */ \
+	_AC(IS_BUILTIN(CONFIG_HAVE_MARCH_ZEC12_FEATURES), UL) << 52 |	/* interlocked facility 2 */ \
+	_AC(IS_BUILTIN(CONFIG_HAVE_MARCH_Z13_FEATURES), UL) << 53	/* load-and-zero-rightmost-byte, etc. */
+
+#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 */ \
+	_BITUL(12) |	/* msa extension 3 */ \
+	_BITUL(13) |	/* msa extension 4 */ \
+	_BITUL(14)	/* enhanced-DAT 2 */
+
+#endif
diff --git a/arch/s390/include/asm/facility.h b/arch/s390/include/asm/facility.h
index 09b406db7529..aed6b5454662 100644
--- a/arch/s390/include/asm/facility.h
+++ b/arch/s390/include/asm/facility.h
@@ -7,7 +7,7 @@
 #ifndef __ASM_FACILITY_H
 #define __ASM_FACILITY_H
 
-#include <generated/facilities.h>
+#include <asm/facilities.h>
 
 #ifndef __ASSEMBLY__
 
diff --git a/arch/s390/tools/.gitignore b/arch/s390/tools/.gitignore
deleted file mode 100644
index 72a4b2cf1365..000000000000
diff --git a/arch/s390/tools/Makefile b/arch/s390/tools/Makefile
deleted file mode 100644
index 6d9814c9df2b..000000000000
diff --git a/arch/s390/tools/gen_facilities.c b/arch/s390/tools/gen_facilities.c
deleted file mode 100644
index fe4e6c910dd7..000000000000

[toc] | [prev] | [next] | [standalone]

#1516195

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.

[toc] | [prev] | [next] | [standalone]

#1516988

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web