mbox series

[RFC,0/1] RISCV_EFI_BOOT_PROTOCOL support in linux

Message ID 20220126110615.33371-1-sunilvl@ventanamicro.com
Headers show
Series RISCV_EFI_BOOT_PROTOCOL support in linux | expand

Message

Sunil V L Jan. 26, 2022, 11:06 a.m. UTC 
This patch adds support for getting the boot hart ID using new
RISCV_EFI_BOOT_PROTOCOL in linux efi stub. While there is an existing solution
of passing the boot hart ID through Device Tree, it doesn't work for ACPI. Hence
an EFI protocol protocol is recommended which works for both DT and ACPI based
platforms.

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 linux ptach can be found in:
riscv_boot_protocol_rfc_v1 branch at https://github.com/vlsunil/linux.git

This is tested in qemu with both u-boot and edk2 firmware changes. To test this
patch with u-boot, we need u-boot to support this new protocol which can be
found in:
riscv_boot_protocol_rfc_v1 branch at https://github.com/vlsunil/u-boot.git

Sunil V L (1):
  riscv/efi_stub: Add support for RISCV_EFI_BOOT_PROTOCOL

 drivers/firmware/efi/libstub/efistub.h    | 15 ++++++++++++
 drivers/firmware/efi/libstub/riscv-stub.c | 28 ++++++++++++++++++++---
 include/linux/efi.h                       |  1 +
 3 files changed, 41 insertions(+), 3 deletions(-)

Comments

Sunil V L Jan. 26, 2022, 11:45 a.m. UTC | #1 
On Wed, Jan 26, 2022 at 12:13:42PM +0100, Ard Biesheuvel wrote:
> Hello Sunil,
> 
> On Wed, 26 Jan 2022 at 12:06, Sunil V L <sunilvl@ventanamicro.com> wrote:
> >
> > This patch adds the support for getting the boot hart ID in
> > Linux EFI stub using RISCV_EFI_BOOT_PROTOCOL.
> >
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > ---
> >  drivers/firmware/efi/libstub/efistub.h    | 15 ++++++++++++
> >  drivers/firmware/efi/libstub/riscv-stub.c | 28 ++++++++++++++++++++---
> >  include/linux/efi.h                       |  1 +
> >  3 files changed, 41 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> > index edb77b0621ea..0428f8816942 100644
> > --- a/drivers/firmware/efi/libstub/efistub.h
> > +++ b/drivers/firmware/efi/libstub/efistub.h
> > @@ -720,6 +720,21 @@ union efi_tcg2_protocol {
> >         } mixed_mode;
> >  };
> >
> > +typedef union riscv_efi_boot_protocol riscv_efi_boot_protocol_t;
> > +
> > +union riscv_efi_boot_protocol {
> > +       struct {
> > +               u64 revision;
> > +               efi_status_t (__efiapi *get_boot_hartid)(
> > +                                                        riscv_efi_boot_protocol_t *,
> > +                                                        size_t *);
> > +       };
> > +       struct {
> > +               u32 revision;
> > +               u32 get_boot_hartid;
> > +       } mixed_mode;
> > +};
> > +
> 
> You don't the mixed mode member here - this is for X64 kernels on IA32
> firmware only.

Ah OK. Thanks for the feedback, Ard. Will remove it in next version.

Thanks
Sunil
> 
> >  typedef union efi_load_file_protocol efi_load_file_protocol_t;
> >  typedef union efi_load_file_protocol efi_load_file2_protocol_t;
> >
> > diff --git a/drivers/firmware/efi/libstub/riscv-stub.c b/drivers/firmware/efi/libstub/riscv-stub.c
> > index 380e4e251399..c7add4eb5453 100644
> > --- a/drivers/firmware/efi/libstub/riscv-stub.c
> > +++ b/drivers/firmware/efi/libstub/riscv-stub.c
> > @@ -46,12 +46,34 @@ static u32 get_boot_hartid_from_fdt(void)
> >         return fdt32_to_cpu(*prop);
> >  }
> >
> > +static u32 get_boot_hartid_from_efi(void)
> > +{
> > +       efi_guid_t boot_protocol_guid = RISCV_EFI_BOOT_PROTOCOL_GUID;
> > +       efi_status_t status;
> > +       riscv_efi_boot_protocol_t *boot_protocol;
> > +       size_t boot_hart_id;
> > +
> > +       status = efi_bs_call(locate_protocol, &boot_protocol_guid, NULL,
> > +                            (void **)&boot_protocol);
> > +       if (status == EFI_SUCCESS) {
> > +               status = efi_call_proto(boot_protocol,
> > +                                       get_boot_hartid, &boot_hart_id);
> > +               if (status == EFI_SUCCESS) {
> > +                       return (u32)boot_hart_id;
> > +               }
> > +       }
> > +       return U32_MAX;
> > +}
> > +
> >  efi_status_t check_platform_features(void)
> >  {
> > -       hartid = get_boot_hartid_from_fdt();
> > +       hartid = get_boot_hartid_from_efi();
> >         if (hartid == U32_MAX) {
> > -               efi_err("/chosen/boot-hartid missing or invalid!\n");
> > -               return EFI_UNSUPPORTED;
> > +               hartid = get_boot_hartid_from_fdt();
> > +               if (hartid == U32_MAX) {
> > +                       efi_err("/chosen/boot-hartid missing or invalid!\n");
> > +                       return EFI_UNSUPPORTED;
> > +               }
> >         }
> >         return EFI_SUCCESS;
> >  }
> > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > index ccd4d3f91c98..9822c730207c 100644
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -380,6 +380,7 @@ void efi_native_runtime_setup(void);
> >  #define EFI_CONSOLE_OUT_DEVICE_GUID            EFI_GUID(0xd3b36f2c, 0xd551, 0x11d4,  0x9a, 0x46, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d)
> >  #define APPLE_PROPERTIES_PROTOCOL_GUID         EFI_GUID(0x91bd12fe, 0xf6c3, 0x44fb,  0xa5, 0xb7, 0x51, 0x22, 0xab, 0x30, 0x3a, 0xe0)
> >  #define EFI_TCG2_PROTOCOL_GUID                 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)
> >  #define EFI_LOAD_FILE_PROTOCOL_GUID            EFI_GUID(0x56ec3091, 0x954c, 0x11d2,  0x8e, 0x3f, 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b)
> >  #define EFI_LOAD_FILE2_PROTOCOL_GUID           EFI_GUID(0x4006c0c1, 0xfcb3, 0x403e,  0x99, 0x6d, 0x4a, 0x6c, 0x87, 0x24, 0xe0, 0x6d)
> >  #define EFI_RT_PROPERTIES_TABLE_GUID           EFI_GUID(0xeb66918a, 0x7eef, 0x402a,  0x84, 0x2e, 0x93, 0x1d, 0x21, 0xc3, 0x8a, 0xe9)
> > --
> > 2.25.1
> >
Heinrich Schuchardt Jan. 27, 2022, 7:47 a.m. UTC | #2 
Am 26. Januar 2022 12:06:15 MEZ schrieb Sunil V L <sunilvl@ventanamicro.com>:
>This patch adds the support for getting the boot hart ID in
>Linux EFI stub using RISCV_EFI_BOOT_PROTOCOL.

It would be helpful to add a link to the spec in the commit message and maybe a comment that this protocol is needed for the ACPI use case.

>
>Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
>---
> drivers/firmware/efi/libstub/efistub.h    | 15 ++++++++++++
> drivers/firmware/efi/libstub/riscv-stub.c | 28 ++++++++++++++++++++---
> include/linux/efi.h                       |  1 +
> 3 files changed, 41 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
>index edb77b0621ea..0428f8816942 100644
>--- a/drivers/firmware/efi/libstub/efistub.h
>+++ b/drivers/firmware/efi/libstub/efistub.h
>@@ -720,6 +720,21 @@ union efi_tcg2_protocol {
> 	} mixed_mode;
> };
> 
>+typedef union riscv_efi_boot_protocol riscv_efi_boot_protocol_t;
>+
>+union riscv_efi_boot_protocol {
>+	struct {
>+		u64 revision;
>+		efi_status_t (__efiapi *get_boot_hartid)(
>+							 riscv_efi_boot_protocol_t *,
>+							 size_t *);

I prefer to have parameter names for readability

According to the platform specification mhartid is MXLEN wide. UINTN (size_t) is SXLEN wide. 

Does this have any implications on how we define the protocol?

>+	};
>+	struct {
>+		u32 revision;
>+		u32 get_boot_hartid;>+	} mixed_mode;
>+};
>+
> typedef union efi_load_file_protocol efi_load_file_protocol_t;
> typedef union efi_load_file_protocol efi_load_file2_protocol_t;
> 
>diff --git a/drivers/firmware/efi/libstub/riscv-stub.c b/drivers/firmware/efi/libstub/riscv-stub.c
>index 380e4e251399..c7add4eb5453 100644
>--- a/drivers/firmware/efi/libstub/riscv-stub.c
>+++ b/drivers/firmware/efi/libstub/riscv-stub.c
>@@ -46,12 +46,34 @@ static u32 get_boot_hartid_from_fdt(void)
> 	return fdt32_to_cpu(*prop);
> }
> 
>+static u32 get_boot_hartid_from_efi(void)
>+{

The returned value must be UINTN /size_t like the protocol. This width must be carried to the legacy entry point of Linux.

>+	efi_guid_t boot_protocol_guid = RISCV_EFI_BOOT_PROTOCOL_GUID;
>+	efi_status_t status;
>+	riscv_efi_boot_protocol_t *boot_protocol;
>+	size_t boot_hart_id;
>+
>+	status = efi_bs_call(locate_protocol, &boot_protocol_guid, NULL,
>+			     (void **)&boot_protocol);
>+	if (status == EFI_SUCCESS) {
>+		status = efi_call_proto(boot_protocol,
>+					get_boot_hartid, &boot_hart_id);
>+		if (status == EFI_SUCCESS) {
>+			return (u32)boot_hart_id;
>+		}
>+	}
>+	return U32_MAX;

U32_MAX is a legal value for the hart id.

>+}
>+
> efi_status_t check_platform_features(void)
> {
>-	hartid = get_boot_hartid_from_fdt();
>+	hartid = get_boot_hartid_from_efi();
> 	if (hartid == U32_MAX) {
>-		efi_err("/chosen/boot-hartid missing or invalid!\n");
>-		return EFI_UNSUPPORTED;
>+		hartid = get_boot_hartid_from_fdt();
>+		if (hartid == U32_MAX) {

U32_MAX is a legal value for the hart id. Please, separate status and value.

Best regards

Heinrich

>+			efi_err("/chosen/boot-hartid missing or invalid!\n");
>+			return EFI_UNSUPPORTED;
>+		}
> 	}
> 	return EFI_SUCCESS;
> }
>diff --git a/include/linux/efi.h b/include/linux/efi.h
>index ccd4d3f91c98..9822c730207c 100644
>--- a/include/linux/efi.h
>+++ b/include/linux/efi.h
>@@ -380,6 +380,7 @@ void efi_native_runtime_setup(void);
> #define EFI_CONSOLE_OUT_DEVICE_GUID		EFI_GUID(0xd3b36f2c, 0xd551, 0x11d4,  0x9a, 0x46, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d)
> #define APPLE_PROPERTIES_PROTOCOL_GUID		EFI_GUID(0x91bd12fe, 0xf6c3, 0x44fb,  0xa5, 0xb7, 0x51, 0x22, 0xab, 0x30, 0x3a, 0xe0)
> #define EFI_TCG2_PROTOCOL_GUID			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)
> #define EFI_LOAD_FILE_PROTOCOL_GUID		EFI_GUID(0x56ec3091, 0x954c, 0x11d2,  0x8e, 0x3f, 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b)
> #define EFI_LOAD_FILE2_PROTOCOL_GUID		EFI_GUID(0x4006c0c1, 0xfcb3, 0x403e,  0x99, 0x6d, 0x4a, 0x6c, 0x87, 0x24, 0xe0, 0x6d)
> #define EFI_RT_PROPERTIES_TABLE_GUID		EFI_GUID(0xeb66918a, 0x7eef, 0x402a,  0x84, 0x2e, 0x93, 0x1d, 0x21, 0xc3, 0x8a, 0xe9)
Sunil V L Jan. 28, 2022, 5:16 a.m. UTC | #3 
On Thu, Jan 27, 2022 at 08:47:35AM +0100, Heinrich Schuchardt wrote:
> Am 26. Januar 2022 12:06:15 MEZ schrieb Sunil V L <sunilvl@ventanamicro.com>:
> >This patch adds the support for getting the boot hart ID in
> >Linux EFI stub using RISCV_EFI_BOOT_PROTOCOL.
> 
> It would be helpful to add a link to the spec in the commit message and maybe a comment that this protocol is needed for the ACPI use case.

Sure. Will add.

> 
> >
> >Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> >---
> > drivers/firmware/efi/libstub/efistub.h    | 15 ++++++++++++
> > drivers/firmware/efi/libstub/riscv-stub.c | 28 ++++++++++++++++++++---
> > include/linux/efi.h                       |  1 +
> > 3 files changed, 41 insertions(+), 3 deletions(-)
> >
> >diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> >index edb77b0621ea..0428f8816942 100644
> >--- a/drivers/firmware/efi/libstub/efistub.h
> >+++ b/drivers/firmware/efi/libstub/efistub.h
> >@@ -720,6 +720,21 @@ union efi_tcg2_protocol {
> > 	} mixed_mode;
> > };
> > 
> >+typedef union riscv_efi_boot_protocol riscv_efi_boot_protocol_t;
> >+
> >+union riscv_efi_boot_protocol {
> >+	struct {
> >+		u64 revision;
> >+		efi_status_t (__efiapi *get_boot_hartid)(
> >+							 riscv_efi_boot_protocol_t *,
> >+							 size_t *);
> 
> I prefer to have parameter names for readability

Sure. Will add.

> 
> According to the platform specification mhartid is MXLEN wide. UINTN (size_t) is SXLEN wide. 
> 
> Does this have any implications on how we define the protocol?

I don't think so. EFI and kernel will be at same privilige level. So, it
is not really an issue for this protocol. 

But when MXLEN > SXLEN (ex: 64 vs 32), then implementation need to
ensure hartid value is 32 bit only so that when it is passed from M-mode
firmware to S-mode, it gets correct value. But again this is not an
issue from this EFI protocol perspective.

> 
> >+	};
> >+	struct {
> >+		u32 revision;
> >+		u32 get_boot_hartid;>+	} mixed_mode;
> >+};
> >+
> > typedef union efi_load_file_protocol efi_load_file_protocol_t;
> > typedef union efi_load_file_protocol efi_load_file2_protocol_t;
> > 
> >diff --git a/drivers/firmware/efi/libstub/riscv-stub.c b/drivers/firmware/efi/libstub/riscv-stub.c
> >index 380e4e251399..c7add4eb5453 100644
> >--- a/drivers/firmware/efi/libstub/riscv-stub.c
> >+++ b/drivers/firmware/efi/libstub/riscv-stub.c
> >@@ -46,12 +46,34 @@ static u32 get_boot_hartid_from_fdt(void)
> > 	return fdt32_to_cpu(*prop);
> > }
> > 
> >+static u32 get_boot_hartid_from_efi(void)
> >+{
> 
> The returned value must be UINTN /size_t like the protocol. This width must be carried to the legacy entry point of Linux.
> 
> >+	efi_guid_t boot_protocol_guid = RISCV_EFI_BOOT_PROTOCOL_GUID;
> >+	efi_status_t status;
> >+	riscv_efi_boot_protocol_t *boot_protocol;
> >+	size_t boot_hart_id;
> >+
> >+	status = efi_bs_call(locate_protocol, &boot_protocol_guid, NULL,
> >+			     (void **)&boot_protocol);
> >+	if (status == EFI_SUCCESS) {
> >+		status = efi_call_proto(boot_protocol,
> >+					get_boot_hartid, &boot_hart_id);
> >+		if (status == EFI_SUCCESS) {
> >+			return (u32)boot_hart_id;
> >+		}
> >+	}
> >+	return U32_MAX;
> 
> U32_MAX is a legal value for the hart id.

Yeah. This is an existing issue in get_boot_hartid_from_fdt() for which
I have sent a fix patch. Once that gets accepted, I will fix
get_boot_hartid_from_efi() and send RFC V2 patch.

> 
> >+}
> >+
> > efi_status_t check_platform_features(void)
> > {
> >-	hartid = get_boot_hartid_from_fdt();
> >+	hartid = get_boot_hartid_from_efi();
> > 	if (hartid == U32_MAX) {
> >-		efi_err("/chosen/boot-hartid missing or invalid!\n");
> >-		return EFI_UNSUPPORTED;
> >+		hartid = get_boot_hartid_from_fdt();
> >+		if (hartid == U32_MAX) {
> 
> U32_MAX is a legal value for the hart id. Please, separate status and value.

Will fix it.

Thank you very much for the feedback.
Sunil

> 
> Best regards
> 
> Heinrich
> 
> >+			efi_err("/chosen/boot-hartid missing or invalid!\n");
> >+			return EFI_UNSUPPORTED;
> >+		}
> > 	}
> > 	return EFI_SUCCESS;
> > }
> >diff --git a/include/linux/efi.h b/include/linux/efi.h
> >index ccd4d3f91c98..9822c730207c 100644
> >--- a/include/linux/efi.h
> >+++ b/include/linux/efi.h
> >@@ -380,6 +380,7 @@ void efi_native_runtime_setup(void);
> > #define EFI_CONSOLE_OUT_DEVICE_GUID		EFI_GUID(0xd3b36f2c, 0xd551, 0x11d4,  0x9a, 0x46, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d)
> > #define APPLE_PROPERTIES_PROTOCOL_GUID		EFI_GUID(0x91bd12fe, 0xf6c3, 0x44fb,  0xa5, 0xb7, 0x51, 0x22, 0xab, 0x30, 0x3a, 0xe0)
> > #define EFI_TCG2_PROTOCOL_GUID			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)
> > #define EFI_LOAD_FILE_PROTOCOL_GUID		EFI_GUID(0x56ec3091, 0x954c, 0x11d2,  0x8e, 0x3f, 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b)
> > #define EFI_LOAD_FILE2_PROTOCOL_GUID		EFI_GUID(0x4006c0c1, 0xfcb3, 0x403e,  0x99, 0x6d, 0x4a, 0x6c, 0x87, 0x24, 0xe0, 0x6d)
> > #define EFI_RT_PROPERTIES_TABLE_GUID		EFI_GUID(0xeb66918a, 0x7eef, 0x402a,  0x84, 0x2e, 0x93, 0x1d, 0x21, 0xc3, 0x8a, 0xe9)
>