Received-SPF: pass (google.com: domain of linux-kernel+bounces-96592-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240307083154.346542-1-haibo1.xu@intel.com> <CAMj1kXGF_PNLYsLugQnm=gW08XuLEbd_C0-4YYzkH1aYRGSaYg@mail.gmail.com>
In-Reply-To: <CAMj1kXGF_PNLYsLugQnm=gW08XuLEbd_C0-4YYzkH1aYRGSaYg@mail.gmail.com>
From: Haibo Xu <xiaobo55x@gmail.com>
Date: Fri, 8 Mar 2024 15:38:17 +0800
Message-ID: <CAJve8ok1E3viWwQS3+mwt02i07BfA5wAnmhOpTALpEEuuGkpEw@mail.gmail.com>
Subject: Re: [PATCH] riscv: dmi: Add SMBIOS/DMI support
To: Ard Biesheuvel <ardb@kernel.org>
Cc: Haibo Xu <haibo1.xu@intel.com>, 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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Mar 7, 2024 at 7:00=E2=80=AFPM Ard Biesheuvel <ardb@kernel.org> wro=
te:
>
> 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=3Dtls -mstack-prot=
ector-guard-reg=3Dtp -mstack-protector-guard-offset=3D0)
> >
> > 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 arch=
ive
> > + * 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 no=
t
> > + * 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.
>

Sure. Thanks for elaborating on it and it's very helpful!

> > +#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, _PAG=
E_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.
>

Yes, memremap() is better here. Will update it in v2.
Thank you for the review!

> > +#define dmi_alloc(l)                   kzalloc(l, GFP_KERNEL)
> > +
> > +#endif
> > diff --git a/drivers/firmware/efi/riscv-runtime.c b/drivers/firmware/ef=
i/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_init=
call
> > +        * itself, depends on dmi_scan_machine() having been called alr=
eady.
> > +        */
> > +       dmi_setup();
> > +
> > +       return 0;
> > +}
> > +core_initcall(riscv_dmi_init);
> > --
> > 2.34.1
> >
> >