fwts: Enable optional efi_runtime kernel module build

PATCH Superseded Sumit Garg 4 participants 6 messages Nov 2, 2018 – Nov 5, 2018

Share & tools

Sumit Garg

author

2018-11-02 12:36 UTC #0 lore raw

PATCH 1/1 fwts: Enable optional efi_runtime kernel module build

Firmware test suite does provides efi_runtime kernel module required to run UEFI tests. So optionally enable this module build.

Signed-off-by Sumit Garg <[email protected]>

---
 package/fwts/Config.in | 8 ++++++++
 package/fwts/fwts.mk   | 6 ++++++
 2 files changed, 14 insertions(+)

package/fwts/Config.in +8 -0

index 959d871..3ddb989 100644
@@ -28,3 +28,11 @@ comment "fwts needs a glibc toolchain w/ wchar, threads"
 	depends on BR2_USE_MMU
 	depends on !BR2_USE_WCHAR || !BR2_TOOLCHAIN_HAS_THREADS || \
 		!BR2_TOOLCHAIN_USES_GLIBC
+
+config BR2_PACKAGE_FWTS_EFI_RUNTIME_MODULE
+	bool "efi_runtime_module"
+	depends on BR2_PACKAGE_FWTS
+	depends on BR2_LINUX_KERNEL
+	help
+	  Firmware Test Suite (FWTS) also provides EFI runtime kernel
+	  module required to run UEFI tests.

package/fwts/fwts.mk +6 -0

index 15f0afc..840190e 100644
@@ -14,3 +14,9 @@ FWTS_DEPENDENCIES = host-bison host-flex host-pkgconf json-c libglib2 libbsd \
 	$(if $(BR2_PACKAGE_DTC),dtc)
 
 $(eval $(autotools-package))
+
+ifdef BR2_PACKAGE_FWTS_EFI_RUNTIME_MODULE
+FWTS_DEPENDENCIES += linux
+FWTS_MODULE_SUBDIRS = efi_runtime
+$(eval $(kernel-module))
+endif

Signature · 1 line

2.7.4

Erico Nunes

2018-11-02 13:57 UTC #1 lore raw

PATCH 1/1 fwts: Enable optional efi_runtime kernel module build

Quoted 3 lines · Sumit Garg

Firmware test suite does provides efi_runtime kernel module required to run UEFI tests. So optionally enable this module build.

Thanks for working on this. I ended up never adding support for it in the package because efi_runtime can also be enabled in the kernel config (config EFI_TEST) rather than being provided by fwts. But I think it's not bad to also have the support here for the source shipped with the given fwts version.

I have some suggestions below.

Quoted 12 lines

Signed-off-by Sumit Garg <[email protected]>
---
 package/fwts/Config.in | 8 ++++++++
 package/fwts/fwts.mk   | 6 ++++++
 2 files changed, 14 insertions(+)
package/fwts/Config.in +0 -0
index 959d871..3ddb989 100644

package/fwts/Config.in comment "fwts needs a glibc toolchain w/ wchar, threads" · line 33

Quoted 6 lines

package/fwts/Config.in +3 -0

@@ -28,3 +28,11 @@ comment "fwts needs a glibc toolchain w/ wchar, threads"
        depends on BR2_USE_MMU
        depends on !BR2_USE_WCHAR || !BR2_TOOLCHAIN_HAS_THREADS || \
                !BR2_TOOLCHAIN_USES_GLIBC
+
+config BR2_PACKAGE_FWTS_EFI_RUNTIME_MODULE
+       bool "efi_runtime_module"

For a slightly better looking menu, I'd suggest dropping the second underscore and making it "efi_runtime module".

package/fwts/Config.in comment "fwts needs a glibc toolchain w/ wchar, threads" · line 34

Quoted 1 line

package/fwts/Config.in +1 -0
+       depends on BR2_PACKAGE_FWTS

Looking at most packages, I think a more clear way to show this option only then fwts is selected, is to wrap it inside a 'if BR2_PACKAGE_FWTS' instead of a 'depends on' for this case. Not sure if it changes anything in practice but it seems more appropriate than a 'depends on' to me.

package/fwts/Config.in comment "fwts needs a glibc toolchain w/ wchar, threads" · line 38

Quoted 4 lines

package/fwts/Config.in +4 -0

+       depends on BR2_LINUX_KERNEL
+       help
+         Firmware Test Suite (FWTS) also provides EFI runtime kernel
+         module required to run UEFI tests.

As it is, the option looks confusing in the menu as it doesn't look like a fwts suboption. It looks like this:
[*] fwts
[ ] efi_runtime_module

I don't fully understand the Kconfig inner workings, but if BR2_PACKAGE_FWTS_EFI_RUNTIME_MODULE is moved to right after BR2_PACKAGE_FWTS (before the comment) then it becomes automatically indented and to me much more easier to understand that it is a fwts suboption:
[*] fwts
[ ] efi_runtime_module

package/fwts/fwts.mk FWTS_DEPENDENCIES = host-bison host-flex host-pkgconf json-c libglib2 libbsd \ · line 19

Quoted 11 lines

package/fwts/fwts.mk +3 -0

index 15f0afc..840190e 100644
@@ -14,3 +14,9 @@ FWTS_DEPENDENCIES = host-bison host-flex host-pkgconf json-c libglib2 libbsd \
        $(if $(BR2_PACKAGE_DTC),dtc)
 
 $(eval $(autotools-package))
+
+ifdef BR2_PACKAGE_FWTS_EFI_RUNTIME_MODULE
+FWTS_DEPENDENCIES += linux

You don't need to add linux as an explicit dependency if you are using $(kernel-module).

package/fwts/fwts.mk FWTS_DEPENDENCIES = host-bison host-flex host-pkgconf json-c libglib2 libbsd \ · line 22

Quoted 3 lines

package/fwts/fwts.mk +3 -0

+FWTS_MODULE_SUBDIRS = efi_runtime
+$(eval $(kernel-module))
+endif

This doesn't work for me, it doesn't trigger linux as a dependency and fails to build from a clean build. The entire block including the $(eval $(kernel-module)) needs to happen before $(eval $(autotools-package)) so that it works as intended.

Erico

Yann E. MORIN

2018-11-02 14:19 UTC #2 lore raw

PATCH 1/1 fwts: Enable optional efi_runtime kernel module build

Erico, Sumit, All,

On 2018-11-02 14:57 +0100, Erico Nunes spake thusly:

Quoted 6 lines · Sumit Garg

package/fwts/Config.in comment "fwts needs a glibc toolchain w/ wchar, threads" · line 33
Quoted 2 lines
package/fwts/Config.in +2 -0
+config BR2_PACKAGE_FWTS_EFI_RUNTIME_MODULE
+       bool "efi_runtime_module"
For a slightly better looking menu, I'd suggest dropping the second underscore and making it "efi_runtime module".

ACK.

Quoted 7 lines

Quoted 1 line
+ depends on BR2_PACKAGE_FWTS
Looking at most packages, I think a more clear way to show this option only then fwts is selected, is to wrap it inside a 'if BR2_PACKAGE_FWTS' instead of a 'depends on' for this case. Not sure if it changes anything in practice but it seems more appropriate than a 'depends on' to me.

It depends (pun intended). While I do prefer an if-block as you suggest, some package do use 'depends on', especially when there is only one or two sub-options. So, historically, both exist, but using an the if-block makes it more consistent.

Quoted 10 lines

package/fwts/Config.in comment "fwts needs a glibc toolchain w/ wchar, threads" · line 38
Quoted 4 lines
package/fwts/Config.in +4 -0
+       depends on BR2_LINUX_KERNEL
+       help
+         Firmware Test Suite (FWTS) also provides EFI runtime kernel
+         module required to run UEFI tests.
As it is, the option looks confusing in the menu as it doesn't look like a fwts suboption. It looks like this:
[*] fwts
[ ] efi_runtime_module

Quoted 7 lines

I don't fully understand the Kconfig inner workings, but if BR2_PACKAGE_FWTS_EFI_RUNTIME_MODULE is moved to right after BR2_PACKAGE_FWTS (before the comment) then it becomes automatically indented and to me much more easier to understand that it is a fwts suboption:
[*] fwts
[ ] efi_runtime_module

Exactly: options are only indented when they are right below the option they depend on. This is an idiosyncracy of kconfig.

And using 'depends on' or an if-block is basically just equivalent.

Quoted 15 lines

package/fwts/fwts.mk FWTS_DEPENDENCIES = host-bison host-flex host-pkgconf json-c libglib2 libbsd \ · line 19
Quoted 11 lines
package/fwts/fwts.mk +3 -0
index 15f0afc..840190e 100644
@@ -14,3 +14,9 @@ FWTS_DEPENDENCIES = host-bison host-flex host-pkgconf json-c libglib2 libbsd \
        $(if $(BR2_PACKAGE_DTC),dtc)
 
 $(eval $(autotools-package))
+
+ifdef BR2_PACKAGE_FWTS_EFI_RUNTIME_MODULE
+FWTS_DEPENDENCIES += linux
You don't need to add linux as an explicit dependency if you are using $(kernel-module).

Quoted 8 lines

package/fwts/fwts.mk FWTS_DEPENDENCIES = host-bison host-flex host-pkgconf json-c libglib2 libbsd \ · line 22
Quoted 3 lines
package/fwts/fwts.mk +3 -0
+FWTS_MODULE_SUBDIRS = efi_runtime
+$(eval $(kernel-module))
+endif
This doesn't work for me, it doesn't trigger linux as a dependency and fails to build from a clean build. The entire block including the $(eval $(kernel-module)) needs to happen before $(eval $(autotools-package)) so that it works as intended.

Exactly, see:
https://buildroot.org/downloads/manual/manual.html#_infrastructure_for_packages_building_kernel_modules

Unlike other package infrastructures, [kernel-module] is not stand-alone, and requires any of the other *-package macros be called after it.

Regards,
Yann E. MORIN.

Signature · 6 lines

.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

Sumit Garg

author

2018-11-05 04:15 UTC #3 lore raw

PATCH 1/1 fwts: Enable optional efi_runtime kernel module build

Quoted 4 lines · Yann E. MORIN

Erico, Sumit, All,
On 2018-11-02 14:57 +0100, Erico Nunes spake thusly:

Quoted 9 lines

Quoted 6 lines · Sumit Garg
package/fwts/Config.in comment "fwts needs a glibc toolchain w/ wchar, threads" · line 33
Quoted 2 lines
package/fwts/Config.in +2 -0
+config BR2_PACKAGE_FWTS_EFI_RUNTIME_MODULE
+       bool "efi_runtime_module"
For a slightly better looking menu, I'd suggest dropping the second underscore and making it "efi_runtime module".
ACK.

Ok.

Quoted 8 lines

Quoted 7 lines
Quoted 1 line
+ depends on BR2_PACKAGE_FWTS
Looking at most packages, I think a more clear way to show this option only then fwts is selected, is to wrap it inside a 'if BR2_PACKAGE_FWTS' instead of a 'depends on' for this case. Not sure if it changes anything in practice but it seems more appropriate than a 'depends on' to me.

Quoted 5 lines

It depends (pun intended). While I do prefer an if-block as you suggest, some package do use 'depends on', especially when there is only one or two sub-options. So, historically, both exist, but using an the if-block makes it more consistent.

Ok will use if block instead.

Quoted 18 lines

Quoted 10 lines
package/fwts/Config.in comment "fwts needs a glibc toolchain w/ wchar, threads" · line 38
Quoted 4 lines
package/fwts/Config.in +4 -0
+       depends on BR2_LINUX_KERNEL
+       help
+         Firmware Test Suite (FWTS) also provides EFI runtime kernel
+         module required to run UEFI tests.
As it is, the option looks confusing in the menu as it doesn't look like a fwts suboption. It looks like this:
[*] fwts
[ ] efi_runtime_module
Quoted 7 lines
I don't fully understand the Kconfig inner workings, but if BR2_PACKAGE_FWTS_EFI_RUNTIME_MODULE is moved to right after BR2_PACKAGE_FWTS (before the comment) then it becomes automatically indented and to me much more easier to understand that it is a fwts suboption:
[*] fwts
[ ] efi_runtime_module

Quoted 3 lines

Exactly: options are only indented when they are right below the option they depend on. This is an idiosyncracy of kconfig.

Ok will move this option right below "fwts".

Quoted 17 lines

And using 'depends on' or an if-block is basically just equivalent.
Quoted 15 lines
package/fwts/fwts.mk FWTS_DEPENDENCIES = host-bison host-flex host-pkgconf json-c libglib2 libbsd \ · line 19
Quoted 11 lines
package/fwts/fwts.mk +3 -0
index 15f0afc..840190e 100644
@@ -14,3 +14,9 @@ FWTS_DEPENDENCIES = host-bison host-flex host-pkgconf json-c libglib2 libbsd \
        $(if $(BR2_PACKAGE_DTC),dtc)
 
 $(eval $(autotools-package))
+
+ifdef BR2_PACKAGE_FWTS_EFI_RUNTIME_MODULE
+FWTS_DEPENDENCIES += linux
You don't need to add linux as an explicit dependency if you are using $(kernel-module).

Ok will remove linux as a dependency.

Quoted 9 lines

Quoted 8 lines
package/fwts/fwts.mk FWTS_DEPENDENCIES = host-bison host-flex host-pkgconf json-c libglib2 libbsd \ · line 22
Quoted 3 lines
package/fwts/fwts.mk +3 -0
+FWTS_MODULE_SUBDIRS = efi_runtime
+$(eval $(kernel-module))
+endif
This doesn't work for me, it doesn't trigger linux as a dependency and fails to build from a clean build. The entire block including the $(eval $(kernel-module)) needs to happen before $(eval $(autotools-package)) so that it works as intended.

Ok.

Quoted 7 lines

Exactly, see:
https://buildroot.org/downloads/manual/manual.html#_infrastructure_for_packages_building_kernel_modules
Unlike other package infrastructures, [kernel-module] is not stand-alone, and requires any of the other *-package macros be called after it.

Thanks for the explanation.

Regards,
Sumit

Quoted 10 lines

Regards,
Yann E. MORIN.
--
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

Thomas Petazzoni

2018-11-02 18:18 UTC #4 lore raw

PATCH 1/1 fwts: Enable optional efi_runtime kernel module build

In reply to Sumit Garg

Hello,

Quoted 15 lines · Sumit Garg

Firmware test suite does provides efi_runtime kernel module required to run UEFI tests. So optionally enable this module build.
Signed-off-by Sumit Garg <[email protected]>
---
 package/fwts/Config.in | 8 ++++++++
 package/fwts/fwts.mk   | 6 ++++++
 2 files changed, 14 insertions(+)
package/fwts/Config.in +0 -0
index 959d871..3ddb989 100644
@@ -28,3 +28,11 @@ comment "fwts needs a glibc toolchain w/ wchar, threads"
 	depends on BR2_USE_MMU

package/fwts/Config.in comment "fwts needs a glibc toolchain w/ wchar, threads" · line 38

Quoted 10 lines

package/fwts/Config.in +8 -0

 	depends on !BR2_USE_WCHAR || !BR2_TOOLCHAIN_HAS_THREADS || \
 		!BR2_TOOLCHAIN_USES_GLIBC
+
+config BR2_PACKAGE_FWTS_EFI_RUNTIME_MODULE
+	bool "efi_runtime_module"
+	depends on BR2_PACKAGE_FWTS
+	depends on BR2_LINUX_KERNEL
+	help
+	  Firmware Test Suite (FWTS) also provides EFI runtime kernel
+	  module required to run UEFI tests.

In addition to the other comments being made: when there is a "depends on BR2_LINUX_KERNEL", we typically need add something like this:

comment "efi-runtime module needs a Linux kernel to be built"
depends on !BR2_LINUX_KERNEL

Best regards,

Thomas

Signature · 3 lines

Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Sumit Garg

author

2018-11-05 04:16 UTC #5 lore raw

PATCH 1/1 fwts: Enable optional efi_runtime kernel module build

Quoted 33 lines · Thomas Petazzoni

Hello,
Quoted 15 lines · Sumit Garg
Firmware test suite does provides efi_runtime kernel module required to run UEFI tests. So optionally enable this module build.
Signed-off-by Sumit Garg <[email protected]>
---
 package/fwts/Config.in | 8 ++++++++
 package/fwts/fwts.mk   | 6 ++++++
 2 files changed, 14 insertions(+)
package/fwts/Config.in +0 -0
index 959d871..3ddb989 100644
@@ -28,3 +28,11 @@ comment "fwts needs a glibc toolchain w/ wchar, threads"
      depends on BR2_USE_MMU
package/fwts/Config.in comment "fwts needs a glibc toolchain w/ wchar, threads" · line 38
Quoted 10 lines
package/fwts/Config.in +8 -0
      depends on !BR2_USE_WCHAR || !BR2_TOOLCHAIN_HAS_THREADS || \
              !BR2_TOOLCHAIN_USES_GLIBC
+
+config BR2_PACKAGE_FWTS_EFI_RUNTIME_MODULE
+     bool "efi_runtime_module"
+     depends on BR2_PACKAGE_FWTS
+     depends on BR2_LINUX_KERNEL
+     help
+       Firmware Test Suite (FWTS) also provides EFI runtime kernel
+       module required to run UEFI tests.
In addition to the other comments being made: when there is a "depends on BR2_LINUX_KERNEL", we typically need add something like this:

Quoted 3 lines

comment "efi-runtime module needs a Linux kernel to be built"
depends on !BR2_LINUX_KERNEL

Sure will add this comment.

Regards,
Sumit

Quoted 7 lines

Best regards,
Thomas
--
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com