Received-SPF: pass (google.com: domain of linux-kernel+bounces-95371-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240307083154.346542-1-haibo1.xu@intel.com>
In-Reply-To: <20240307083154.346542-1-haibo1.xu@intel.com>
From: Ard Biesheuvel <ardb@kernel.org>
Date: Thu, 7 Mar 2024 12:00:38 +0100
Message-ID: <CAMj1kXGF_PNLYsLugQnm=gW08XuLEbd_C0-4YYzkH1aYRGSaYg@mail.gmail.com>
Subject: Re: [PATCH] riscv: dmi: Add SMBIOS/DMI support
To: Haibo Xu <haibo1.xu@intel.com>
Cc: linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org, 
	linux-efi@vger.kernel.org, paul.walmsley@sifive.com, palmer@dabbelt.com, 
	aou@eecs.berkeley.edu, sunilvl@ventanamicro.com, xiaobo55x@gmail.com
Content-Type: text/plain; charset="UTF-8"

Hello Haibo,

Some notes below.

On Thu, 7 Mar 2024 at 09:18, Haibo Xu <haibo1.xu@intel.com> wrote:
>
> Enable the dmi driver for riscv which would allow access the
> SMBIOS info through some userspace file(/sys/firmware/dmi/*).
>
> The change was based on that of arm64 and has been verified
> by dmidecode tool.
>
> Signed-off-by: Haibo Xu <haibo1.xu@intel.com>
> ---
>  arch/riscv/Kconfig                   | 11 +++++++++++
>  arch/riscv/include/asm/dmi.h         | 29 ++++++++++++++++++++++++++++
>  drivers/firmware/efi/riscv-runtime.c | 13 +++++++++++++
>  3 files changed, 53 insertions(+)
>  create mode 100644 arch/riscv/include/asm/dmi.h
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 0bfcfec67ed5..a123a3e7e5f3 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -918,6 +918,17 @@ config EFI
>           allow the kernel to be booted as an EFI application. This
>           is only useful on systems that have UEFI firmware.
>
> +config DMI
> +       bool "Enable support for SMBIOS (DMI) tables"
> +       depends on EFI
> +       default y
> +       help
> +         This enables SMBIOS/DMI feature for systems.
> +
> +         This option is only useful on systems that have UEFI firmware.
> +         However, even with this option, the resultant kernel should
> +         continue to boot on existing non-UEFI platforms.
> +
>  config CC_HAVE_STACKPROTECTOR_TLS
>         def_bool $(cc-option,-mstack-protector-guard=tls -mstack-protector-guard-reg=tp -mstack-protector-guard-offset=0)
>
> diff --git a/arch/riscv/include/asm/dmi.h b/arch/riscv/include/asm/dmi.h
> new file mode 100644
> index 000000000000..a861043f02dc
> --- /dev/null
> +++ b/arch/riscv/include/asm/dmi.h
> @@ -0,0 +1,29 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2024 Intel Corporation
> + *
> + * based on arch/arm64/include/asm/dmi.h
> + *
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License.  See the file "COPYING" in the main directory of this archive
> + * for more details.
> + */
> +
> +#ifndef __ASM_DMI_H
> +#define __ASM_DMI_H
> +
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +
> +/*
> + * According to section 2.3.6 of the UEFI spec, the firmware should not
> + * request a virtual mapping for configuration tables such as SMBIOS.
> + * This means we have to map them before use.
> + */

You can drop this comment, it is not really accurate.

'Requesting a virtual mapping' means the memory is mapped by the OS
into the EFI page tables before calling a runtime service, so that the
firmware (which runs under the OS's memory translation regime) can
access the contents.

SMBIOS tables are informational and for consumption by the OS only,
not by the runtime service implementations themselves, and so they can
be omitted from the EFI runtime page tables.


> +#define dmi_early_remap(x, l)          ioremap_prot(x, l, _PAGE_KERNEL)
> +#define dmi_early_unmap(x, l)          iounmap(x)
> +#define dmi_remap(x, l)                        ioremap_prot(x, l, _PAGE_KERNEL)
> +#define dmi_unmap(x)                   iounmap(x)

Why not use memremap() here? That will reuse the linear map if it
happens to already cover the region.

> +#define dmi_alloc(l)                   kzalloc(l, GFP_KERNEL)
> +
> +#endif
> diff --git a/drivers/firmware/efi/riscv-runtime.c b/drivers/firmware/efi/riscv-runtime.c
> index 09525fb5c240..c3bfb9e77e02 100644
> --- a/drivers/firmware/efi/riscv-runtime.c
> +++ b/drivers/firmware/efi/riscv-runtime.c
> @@ -152,3 +152,16 @@ void arch_efi_call_virt_teardown(void)
>  {
>         efi_virtmap_unload();
>  }
> +
> +static int __init riscv_dmi_init(void)
> +{
> +       /*
> +        * On riscv, DMI depends on UEFI, and dmi_setup() needs to
> +        * be called early because dmi_id_init(), which is an arch_initcall
> +        * itself, depends on dmi_scan_machine() having been called already.
> +        */
> +       dmi_setup();
> +
> +       return 0;
> +}
> +core_initcall(riscv_dmi_init);
> --
> 2.34.1
>
>