Re: [RFC PATCH 1/1] efi_loader: Enable RISCV_EFI_BOOT_PROTOCOL support

[Sunil V L <sunilvl@ventanamicro.com>]

Hi Heinrich,
On Thu, Jan 27, 2022 at 09:44:57AM +0100, Heinrich Schuchardt wrote:
> On 1/26/22 12:06, Sunil V L wrote:
> > This adds support for new RISCV_EFI_BOOT_PROTOCOL to
> > communicate the boot hart ID to bootloader/kernel on RISC-V
> > UEFI platforms.
> > 
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > ---
> >   include/efi_api.h          |  4 +++
> >   include/efi_loader.h       |  2 ++
> >   include/efi_riscv.h        | 16 +++++++++
> >   lib/efi_loader/Kconfig     |  7 ++++
> >   lib/efi_loader/Makefile    |  1 +
> >   lib/efi_loader/efi_riscv.c | 69 ++++++++++++++++++++++++++++++++++++++
> >   lib/efi_loader/efi_setup.c |  6 ++++
> >   7 files changed, 105 insertions(+)
> >   create mode 100644 include/efi_riscv.h
> >   create mode 100644 lib/efi_loader/efi_riscv.c
> > 
> > diff --git a/include/efi_api.h b/include/efi_api.h
> > index 8d5d835bd0..f123d0557c 100644
> > --- a/include/efi_api.h
> > +++ b/include/efi_api.h
> > @@ -438,6 +438,10 @@ struct efi_runtime_services {
> >   	EFI_GUID(0x607f766c, 0x7455, 0x42be, 0x93, \
> >   		 0x0b, 0xe4, 0xd7, 0x6d, 0xb2, 0x72, 0x0f)
> > 
> > +#define RISCV_EFI_BOOT_PROTOCOL_GUID \
> > +	EFI_GUID(0xccd15fec, 0x6f73, 0x4eec, 0x83, \
> > +		 0x95, 0x3e, 0x69, 0xe4, 0xb9, 0x40, 0xbf)
> > +
> >   /**
> >    * struct efi_configuration_table - EFI Configuration Table
> >    *
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index 701efcd2b6..1fa75b40fe 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -527,6 +527,8 @@ efi_status_t efi_disk_register(void);
> >   efi_status_t efi_rng_register(void);
> >   /* Called by efi_init_obj_list() to install EFI_TCG2_PROTOCOL */
> >   efi_status_t efi_tcg2_register(void);
> > +/* Called by efi_init_obj_list() to install RISCV_EFI_BOOT_PROTOCOL */
> > +efi_status_t efi_riscv_register(void);
> >   /* Called by efi_init_obj_list() to do initial measurement */
> >   efi_status_t efi_tcg2_do_initial_measurement(void);
> >   /* measure the pe-coff image, extend PCR and add Event Log */
> > diff --git a/include/efi_riscv.h b/include/efi_riscv.h
> > new file mode 100644
> > index 0000000000..6beb2637f6
> > --- /dev/null
> > +++ b/include/efi_riscv.h
> > @@ -0,0 +1,16 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * RISCV_EFI_BOOT_PROTOCOL
> > + *
> > + * Copyright (c) 2022 Ventana Micro Systems Inc
> > + */
> > +
> > +#include <efi_api.h>
> > +
> > +#define RISCV_EFI_BOOT_PROTOCOL_REVISION 0x00010000
> > +
> 
> Please, add Sphinx style comments to describe the structure.

Sure.

> 
> > +struct riscv_efi_boot_protocol {
> > +	u64 revision;
> > +	efi_status_t (EFIAPI *get_boot_hartid) (struct riscv_efi_boot_protocol *this,
> > +						efi_uintn_t *boot_hartid);
> > +};
> > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > index 24f9a2bb75..77ba6a7ea1 100644
> > --- a/lib/efi_loader/Kconfig
> > +++ b/lib/efi_loader/Kconfig
> > @@ -369,4 +369,11 @@ config EFI_ESRT
> >   	help
> >   	  Enabling this option creates the ESRT UEFI system table.
> > 
> > +config EFI_RISCV_BOOT_PROTOCOL
> > +	bool "RISCV_EFI_BOOT_PROTOCOL support"
> > +	default y
> > +	depends on RISCV
> > +	help
> > +	  Provide a RISCV_EFI_BOOT_PROTOCOL implementation on RISC-V platform.
> > +
> >   endif
> > diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> > index fd344cea29..b2c664d108 100644
> > --- a/lib/efi_loader/Makefile
> > +++ b/lib/efi_loader/Makefile
> > @@ -62,6 +62,7 @@ obj-$(CONFIG_GENERATE_ACPI_TABLE) += efi_acpi.o
> >   obj-$(CONFIG_GENERATE_SMBIOS_TABLE) += efi_smbios.o
> >   obj-$(CONFIG_EFI_RNG_PROTOCOL) += efi_rng.o
> >   obj-$(CONFIG_EFI_TCG2_PROTOCOL) += efi_tcg2.o
> > +obj-$(CONFIG_EFI_RISCV_BOOT_PROTOCOL) += efi_riscv.o
> >   obj-$(CONFIG_EFI_LOAD_FILE2_INITRD) += efi_load_initrd.o
> >   obj-$(CONFIG_EFI_SIGNATURE_SUPPORT) += efi_signature.o
> > 
> > diff --git a/lib/efi_loader/efi_riscv.c b/lib/efi_loader/efi_riscv.c
> > new file mode 100644
> > index 0000000000..91b8d2b927
> > --- /dev/null
> > +++ b/lib/efi_loader/efi_riscv.c
> > @@ -0,0 +1,69 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Defines APIs that allow an OS to interact with UEFI firmware to query
> > + * information about the boot hart ID.
> > + *
> > + * Copyright (c) 2022, Ventana Micro Systems Inc
> > + */
> > +
> > +#define LOG_CATEGORY LOGC_EFI
> > +#include <common.h>
> > +#include <efi_loader.h>
> > +#include <efi_variable.h>
> > +#include <log.h>
> > +#include <asm/global_data.h>
> > +#include <efi_riscv.h>
> > +
> > +DECLARE_GLOBAL_DATA_PTR;
> > +
> > +static const efi_guid_t efi_guid_riscv_boot_protocol = RISCV_EFI_BOOT_PROTOCOL_GUID;
> > +static efi_uintn_t hartid;
> 
> I think we can do without this static variable. gd->arch.boot_hart does
> not change.

Yes. I missed EFI_ENTRY. Without EFI_ENTRY(), this didn't work. Will fix
this in RFC V2 patch.

> 
> > +
> > +/**
> > + * efi_riscv_get_boot_hartid() - return boot hart ID
> > + *
> > + * @this:		RISCV_EFI_BOOT_PROTOCOL instance
> > + * @boot_hartid		caller allocated memory to return boot hart id
> > +
> > + * Return:	status code
> > + */
> > +static efi_status_t EFIAPI
> > +efi_riscv_get_boot_hartid(struct riscv_efi_boot_protocol *this,
> > +					efi_uintn_t *boot_hartid)
> > +{
> > +	log_err(" efi_riscv_get_boot_hartid ENTER\n");
> 
> This seems to be left from debugging.
> 
> Please, use EFI_ENTRY in all UEFI API interface implementations.
> 
> EFI_ENTRY("");
> 
> This will show the function name when enabling debug messages.
> 
> > +	if ((this == NULL) || (boot_hartid == NULL))
> 
> We try to avoid == NULL. We should check against the actual protocol.
> 
> if (this != riscv_efi_boot_prot || !boot_hartid)
> 
Thanks!. Will fix these issues.

> > +		return EFI_INVALID_PARAMETER;
> > +
> > +	*boot_hartid = hartid;
> > +
> > +	return EFI_SUCCESS;
> 
> return EFI_EXIT(EFI_SUCCESS).
> 
> > +}
> > +
> > +static const struct riscv_efi_boot_protocol riscv_efi_boot_prot = {
> > +	.revision = RISCV_EFI_BOOT_PROTOCOL_REVISION,
> > +	.get_boot_hartid = efi_riscv_get_boot_hartid
> > +};
> > +
> > +/**
> > + * efi_riscv_register() - register RISCV_EFI_BOOT_PROTOCOL
> > + *
> > + *
> > + * Return:	status code
> > + */
> > +efi_status_t efi_riscv_register(void)
> > +{
> > +	efi_status_t ret = EFI_SUCCESS;
> > +
> > +	/* save boot hart id since gd is not accessible after launching
> > +	 * the kernel
> 
> gd is accessible until EXIT_BOOT_SERVICES(). After EXIT_BOOT_SERVICES
> the protocol cannot be called.
> 
> set_gd(efi_gd) called by EFI_ENTRY() takes care of restoring the gp
> register if the kernel should mess it up, which an UEFI program should
> not do.

Thanks a lot for your comments. Will address these issues and send V2
version.

Thanks!
Sunil
> 
> Best regards
> 
> Heinrich
> 
> > +	 */
> > +	hartid = gd->arch.boot_hart;
> > +
> > +	ret = efi_add_protocol(efi_root, &efi_guid_riscv_boot_protocol,
> > +			       (void *)&riscv_efi_boot_prot);
> > +	if (ret != EFI_SUCCESS) {
> > +		log_err("Cannot install RISCV_EFI_BOOT_PROTOCOL\n");
> > +	}
> > +	return ret;
> > +}
> > diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> > index 49172e3579..380adc15c8 100644
> > --- a/lib/efi_loader/efi_setup.c
> > +++ b/lib/efi_loader/efi_setup.c
> > @@ -247,6 +247,12 @@ efi_status_t efi_init_obj_list(void)
> >   			goto out;
> >   	}
> > 
> > +	if (IS_ENABLED(CONFIG_EFI_RISCV_BOOT_PROTOCOL)) {
> > +		ret = efi_riscv_register();
> > +		if (ret != EFI_SUCCESS)
> > +			goto out;
> > +	}
> > +
> >   	/* Secure boot */
> >   	ret = efi_init_secure_boot();
> >   	if (ret != EFI_SUCCESS)
>