Re: [PATCH 5/5] riscv/efi_stub: Support for 64bit boot-hartid

[Atish Patra <atishp@atishpatra.org>]

On Wed, May 25, 2022 at 9:09 AM Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> On 5/25/22 17:48, Ard Biesheuvel wrote:
> > On Wed, 25 May 2022 at 17:11, Sunil V L <sunilvl@ventanamicro.com> wrote:
> >>
> >> The boot-hartid can be a 64bit value on RV64 platforms. Currently,
> >> the "boot-hartid" in DT is assumed to be 32bit only. This patch
> >> detects the size of the "boot-hartid" and uses 32bit or 64bit
> >> FDT reads appropriately.
> >>
> >> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> >> ---
> >>   drivers/firmware/efi/libstub/riscv-stub.c | 12 +++++++++---
> >>   1 file changed, 9 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/firmware/efi/libstub/riscv-stub.c b/drivers/firmware/efi/libstub/riscv-stub.c
> >> index 9e85e58d1f27..d748533f1329 100644
> >> --- a/drivers/firmware/efi/libstub/riscv-stub.c
> >> +++ b/drivers/firmware/efi/libstub/riscv-stub.c
> >> @@ -29,7 +29,7 @@ static int get_boot_hartid_from_fdt(void)
> >>   {
> >>          const void *fdt;
> >>          int chosen_node, len;
> >> -       const fdt32_t *prop;
> >> +       const void *prop;
> >>
> >>          fdt = get_efi_config_table(DEVICE_TREE_GUID);
> >>          if (!fdt)
> >> @@ -40,10 +40,16 @@ static int get_boot_hartid_from_fdt(void)
> >>                  return -EINVAL;
> >>
> >>          prop = fdt_getprop((void *)fdt, chosen_node, "boot-hartid", &len);
> >> -       if (!prop || len != sizeof(u32))
> >> +       if (!prop)
> >> +               return -EINVAL;
> >> +
> >> +       if (len == sizeof(u32))
> >> +               hartid = (unsigned long) fdt32_to_cpu(*(fdt32_t *)prop);
> >> +       else if (len == sizeof(u64))
> >> +               hartid = (unsigned long) fdt64_to_cpu(*(fdt64_t *)prop);
> >
> > Does RISC-V care about alignment? A 64-bit quantity is not guaranteed
> > to appear 64-bit aligned in the DT, and the cast violates C alignment
> > rules, so this should probably used get_unaligned_be64() or something
> > like that.
>
> When running in S-mode the SBI handles unaligned access but this has a
> performance penalty.
>
> We could use fdt64_to_cpu(__get_unaligned_t(fdt64_t, prop)) here.
>

It is better to avoid unaligned access in the kernel. There are some
plans to disable
misaligned load/store emulation in the firmware if user space requests
it via prctl.

We need another SBI extension to do that. The idea is to keep it
enabled by default in the firmware but
userspace should have an option to disable it via prctl. If we make
sure that the kernel doesn't invoke any
unaligned access, this feature can be implemented easily.

> Best regards
>
> Heinrich
>
> >
> >
> >> +       else
> >>                  return -EINVAL;
> >>
> >> -       hartid = fdt32_to_cpu(*prop);
> >>          return 0;
> >>   }
> >>
> >> --
> >> 2.25.1
> >>
>


-- 
Regards,
Atish