All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH 0/1] RISCV_EFI_BOOT_PROTOCOL support in U-boot
@ 2022-01-26 11:06 Sunil V L
  2022-01-26 11:06 ` [RFC PATCH 1/1] efi_loader: Enable RISCV_EFI_BOOT_PROTOCOL support Sunil V L
  0 siblings, 1 reply; 6+ messages in thread
From: Sunil V L @ 2022-01-26 11:06 UTC (permalink / raw)
  To: Heinrich Schuchardt
  Cc: u-boot, Ard Biesheuvel, Anup Patel, Atish Patra, Abner Chang,
	Jessica Clarke, Sunil V L

This patch adds the support in u-boot for new RISCV_EFI_BOOT_PROTOCOL for RISC-V
UEFI platforms. This protocol is required to communicate the boot hart ID to the
bootloader/kernel which need to follow the EFI calling conventions.

The latest draft spec of this new protocol is available at
https://github.com/riscv-non-isa/riscv-uefi/releases/download/1.0-rc2/RISCV_UEFI_PROTOCOL-spec.pdf

This u-boot patch can be found in:
riscv_boot_protocol_rfc_v1 branch at https://github.com/vlsunil/u-boot.git

This patch is tested in qemu. To fully test the feature, we need
Linux changes which consume this protocol.
The linux patch can be found in:
riscv_boot_protocol_rfc_v1 branch at https://github.com/vlsunil/linux.git

Sunil V L (1):
  efi_loader: Enable RISCV_EFI_BOOT_PROTOCOL support

 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

-- 
2.25.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [RFC PATCH 1/1] efi_loader: Enable RISCV_EFI_BOOT_PROTOCOL support
  2022-01-26 11:06 [RFC PATCH 0/1] RISCV_EFI_BOOT_PROTOCOL support in U-boot Sunil V L
@ 2022-01-26 11:06 ` Sunil V L
  2022-01-26 14:01   ` Jessica Clarke
  2022-01-27  8:44   ` Heinrich Schuchardt
  0 siblings, 2 replies; 6+ messages in thread
From: Sunil V L @ 2022-01-26 11:06 UTC (permalink / raw)
  To: Heinrich Schuchardt
  Cc: u-boot, Ard Biesheuvel, Anup Patel, Atish Patra, Abner Chang,
	Jessica Clarke, Sunil V L

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
+
+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;
+
+/**
+ * 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");
+	if ((this == NULL) || (boot_hartid == NULL))
+		return EFI_INVALID_PARAMETER;
+
+	*boot_hartid = hartid;
+
+	return 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
+	 */
+	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)
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [RFC PATCH 1/1] efi_loader: Enable RISCV_EFI_BOOT_PROTOCOL support
  2022-01-26 11:06 ` [RFC PATCH 1/1] efi_loader: Enable RISCV_EFI_BOOT_PROTOCOL support Sunil V L
@ 2022-01-26 14:01   ` Jessica Clarke
  2022-01-27  6:35     ` Heinrich Schuchardt
  2022-01-27  8:44   ` Heinrich Schuchardt
  1 sibling, 1 reply; 6+ messages in thread
From: Jessica Clarke @ 2022-01-26 14:01 UTC (permalink / raw)
  To: Sunil V L
  Cc: Heinrich Schuchardt, u-boot, Ard Biesheuvel, Anup Patel,
	Atish Patra, Abner Chang

On 26 Jan 2022, at 11:06, Sunil V L <sunilvl@ventanamicro.com> 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
> +
> +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

Why would you ever turn it off for a RISC-V EFI system?

> +	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;
> +
> +/**
> + * 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");
> +	if ((this == NULL) || (boot_hartid == NULL))
> +		return EFI_INVALID_PARAMETER;
> +
> +	*boot_hartid = hartid;
> +
> +	return 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
> +	 */
> +	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;
> +	}
> +

I don’t think this will link if EFI_RISCV_BOOT_PROTOCOL is disabled. At
least not at -O0, at -O1 and above it’ll probably optimise away the
dead code and thus not reference the undefined symbol.

Jess

> 	/* Secure boot */
> 	ret = efi_init_secure_boot();
> 	if (ret != EFI_SUCCESS)
> -- 
> 2.25.1
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC PATCH 1/1] efi_loader: Enable RISCV_EFI_BOOT_PROTOCOL support
  2022-01-26 14:01   ` Jessica Clarke
@ 2022-01-27  6:35     ` Heinrich Schuchardt
  0 siblings, 0 replies; 6+ messages in thread
From: Heinrich Schuchardt @ 2022-01-27  6:35 UTC (permalink / raw)
  To: Jessica Clarke, Sunil V L
  Cc: u-boot, Ard Biesheuvel, Anup Patel, Atish Patra, Abner Chang

Am 26. Januar 2022 15:01:56 MEZ schrieb Jessica Clarke <jrtc27@jrtc27.com>:
>On 26 Jan 2022, at 11:06, Sunil V L <sunilvl@ventanamicro.com> 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
>> +
>> +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
>
>Why would you ever turn it off for a RISC-V EFI system?

We don't need the protocol on ARM. So we want a configuration symbol for it.

The feature is experimental as there is no ratified specification requiring it. So it is ok to be able to disable it. Anyway it defaults to yes on RISC-V.

We still put the boot-hartid into the device-tree as elder kernels need it and future kernels may support this due to the legacy entry point. To minimize the code size on constrained devices it may be necessary to disable the protocol.

>
>> +	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;
>> +
>> +/**
>> + * 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");
>> +	if ((this == NULL) || (boot_hartid == NULL))
>> +		return EFI_INVALID_PARAMETER;
>> +
>> +	*boot_hartid = hartid;
>> +
>> +	return 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
>> +	 */
>> +	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;
>> +	}
>> +
>
>I don’t think this will link if EFI_RISCV_BOOT_PROTOCOL is disabled. At
>least not at -O0, at -O1 and above it’ll probably optimise away the
>dead code and thus not reference the undefined symbol.

U-Boot is built with -O2 (or -Os). The project wants to avoid #ifdef. See the warning in scripts/checkpatch.pl. With this coding style we catch more coding issues in our CI without increasing the binary size.

Best regards

Heinrich

>
>Jess
>
>> 	/* Secure boot */
>> 	ret = efi_init_secure_boot();
>> 	if (ret != EFI_SUCCESS)
>> -- 
>> 2.25.1
>> 
>


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC PATCH 1/1] efi_loader: Enable RISCV_EFI_BOOT_PROTOCOL support
  2022-01-26 11:06 ` [RFC PATCH 1/1] efi_loader: Enable RISCV_EFI_BOOT_PROTOCOL support Sunil V L
  2022-01-26 14:01   ` Jessica Clarke
@ 2022-01-27  8:44   ` Heinrich Schuchardt
  2022-01-28  5:08     ` Sunil V L
  1 sibling, 1 reply; 6+ messages in thread
From: Heinrich Schuchardt @ 2022-01-27  8:44 UTC (permalink / raw)
  To: Sunil V L
  Cc: u-boot, Ard Biesheuvel, Anup Patel, Atish Patra, Abner Chang,
	Jessica Clarke

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.

> +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.

> +
> +/**
> + * 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)

> +		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.

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)


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC PATCH 1/1] efi_loader: Enable RISCV_EFI_BOOT_PROTOCOL support
  2022-01-27  8:44   ` Heinrich Schuchardt
@ 2022-01-28  5:08     ` Sunil V L
  0 siblings, 0 replies; 6+ messages in thread
From: Sunil V L @ 2022-01-28  5:08 UTC (permalink / raw)
  To: Heinrich Schuchardt
  Cc: u-boot, Ard Biesheuvel, Anup Patel, Atish Patra, Abner Chang,
	Jessica Clarke

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)
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-01-28  5:08 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-26 11:06 [RFC PATCH 0/1] RISCV_EFI_BOOT_PROTOCOL support in U-boot Sunil V L
2022-01-26 11:06 ` [RFC PATCH 1/1] efi_loader: Enable RISCV_EFI_BOOT_PROTOCOL support Sunil V L
2022-01-26 14:01   ` Jessica Clarke
2022-01-27  6:35     ` Heinrich Schuchardt
2022-01-27  8:44   ` Heinrich Schuchardt
2022-01-28  5:08     ` Sunil V L

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.