Received-SPF: pass (google.com: domain of linux-kernel+bounces-159951-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date: Fri, 26 Apr 2024 14:48:39 +0300
Message-Id: <D0U14GGWT2AW.XRFW7MB1QZFV@kernel.org>
Cc: <linux-kernel@vger.kernel.org>, "Ilias Apalodimas"
 <ilias.apalodimas@linaro.org>, "Lennart Poettering"
 <lennart@poettering.net>
Subject: Re: [PATCH] efi: expose TPM event log to userspace via sysfs
From: "Jarkko Sakkinen" <jarkko@kernel.org>
To: "Mikko Rapeli" <mikko.rapeli@linaro.org>, "Ard Biesheuvel"
 <ardb@kernel.org>, <linux-efi@vger.kernel.org>
References: <20240422112711.362779-1-mikko.rapeli@linaro.org>
In-Reply-To: <20240422112711.362779-1-mikko.rapeli@linaro.org>

On Mon Apr 22, 2024 at 2:27 PM EEST, Mikko Rapeli wrote:
>  Documentation/ABI/testing/sysfs-firmware-efi | 12 ++++++++++++
>  drivers/firmware/efi/efi.c                   | 13 +++++++++++++
>  2 files changed, 25 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-firmware-efi b/Documentation=
/ABI/testing/sysfs-firmware-efi
> index 5e4d0b27cdfe..caaff27cc73e 100644
> --- a/Documentation/ABI/testing/sysfs-firmware-efi
> +++ b/Documentation/ABI/testing/sysfs-firmware-efi
> @@ -36,3 +36,15 @@ Description:	Displays the content of the Runtime Confi=
guration Interface
>  		Table version 2 on Dell EMC PowerEdge systems in binary format
>  Users:		It is used by Dell EMC OpenManage Server Administrator tool to
>  		populate BIOS setup page.
> +
> +What:		/sys/firmware/efi/tpm_log

*Conditional* suggestion: s/tpm_log/tpm_event_log/

I.e. if we want this better to use the most popular name.

> +Date:		April 2024
> +Contact:	Mikko Rapeli <mikko.rapeli@linaro.org>
> +Description:	If EFI firmware supports TPM device and measurements were d=
one
> +		then a TPM event log has very likely been generated and provided
> +		to the kernel. This serves as indicator for userspace to load
> +		TPM drivers and to start related service early in the boot sequence,
> +		e.g. initramfs, where full bus probes and device scans are not yet
> +		done.
> +Users:		systemd will use this interface to support TPM drivers as module=
s also
> +		for early initramfs
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 4fcda50acfa4..94773e8b8806 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -162,6 +162,13 @@ static ssize_t fw_platform_size_show(struct kobject =
*kobj,
>  	return sprintf(buf, "%d\n", efi_enabled(EFI_64BIT) ? 64 : 32);
>  }
> =20
> +static ssize_t tpm_log_show(struct kobject *kobj,
> +				     struct kobj_attribute *attr, char *buf)
> +{
> +	return sprintf(buf, "TPMEventLog=3D0x%lx", efi.tpm_log);

It is also asymmetric with the log message as we can see here. Why you
want to put that prefix and not just print the number? Why hex and not
just %lu?


> +}
> +static struct kobj_attribute efi_attr_tpm_log =3D __ATTR_RO_MODE(tpm_log=
, 0400);
> +
>  extern __weak struct kobj_attribute efi_attr_fw_vendor;
>  extern __weak struct kobj_attribute efi_attr_runtime;
>  extern __weak struct kobj_attribute efi_attr_config_table;
> @@ -459,6 +466,12 @@ static int __init efisubsys_init(void)
>  		platform_device_register_simple("efi_secret", 0, NULL, 0);
>  #endif
> =20
> +	if (efi.tpm_log !=3D EFI_INVALID_TABLE_ADDR) {
> +		error =3D sysfs_create_file(efi_kobj, &efi_attr_tpm_log.attr);
> +		if (error)
> +			pr_err("sysfs create file failed with error %d.\n", error);
> +	}

s/err/warn/

Why "sys create file" and not "sys_create_file"?

> +
>  	return 0;
> =20
>  err_remove_group:

BR, Jarkko