mbox series

[0/5] Support for 64bit hartid on RV64 platforms

Message ID 20220525151106.2176147-1-sunilvl@ventanamicro.com
Headers show
Series Support for 64bit hartid on RV64 platforms | expand

Message

Sunil V L May 25, 2022, 3:11 p.m. UTC
The hartid can be a 64bit value on RV64 platforms. This series updates
the code so that 64bit hartid can be supported on RV64 platforms.

Sunil V L (5):
  riscv: cpu_ops_sbi: Support for 64bit hartid
  riscv: cpu_ops_spinwait: Support for 64bit hartid
  riscv: smp: Support for 64bit hartid
  riscv: cpu: Support for 64bit hartid
  riscv/efi_stub: Support for 64bit boot-hartid

 arch/riscv/include/asm/processor.h        |  4 ++--
 arch/riscv/include/asm/smp.h              |  4 ++--
 arch/riscv/kernel/cpu.c                   | 26 +++++++++++++----------
 arch/riscv/kernel/cpu_ops_sbi.c           |  4 ++--
 arch/riscv/kernel/cpu_ops_spinwait.c      |  2 +-
 arch/riscv/kernel/cpufeature.c            |  6 ++++--
 arch/riscv/kernel/smp.c                   |  4 ++--
 arch/riscv/kernel/smpboot.c               |  9 ++++----
 drivers/clocksource/timer-riscv.c         | 15 +++++++------
 drivers/firmware/efi/libstub/riscv-stub.c | 12 ++++++++---
 drivers/irqchip/irq-riscv-intc.c          |  7 +++---
 drivers/irqchip/irq-sifive-plic.c         |  7 +++---
 12 files changed, 58 insertions(+), 42 deletions(-)

Comments

Heinrich Schuchardt May 25, 2022, 3:17 p.m. UTC | #1
On 5/25/22 17:11, Sunil V L wrote:
> The hartid can be a 64bit value on RV64 platforms. This patch modifies
> the hartid variable type to unsigned long so that it can hold 64bit
> value on RV64 platforms.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>

Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>

> ---
>   arch/riscv/kernel/cpu_ops_sbi.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/riscv/kernel/cpu_ops_sbi.c b/arch/riscv/kernel/cpu_ops_sbi.c
> index 4f5a6f84e2a4..efa0f0816634 100644
> --- a/arch/riscv/kernel/cpu_ops_sbi.c
> +++ b/arch/riscv/kernel/cpu_ops_sbi.c
> @@ -65,7 +65,7 @@ static int sbi_hsm_hart_get_status(unsigned long hartid)
>   static int sbi_cpu_start(unsigned int cpuid, struct task_struct *tidle)
>   {
>   	unsigned long boot_addr = __pa_symbol(secondary_start_sbi);
> -	int hartid = cpuid_to_hartid_map(cpuid);
> +	unsigned long hartid = cpuid_to_hartid_map(cpuid);
>   	unsigned long hsm_data;
>   	struct sbi_hart_boot_data *bdata = &per_cpu(boot_data, cpuid);
>   
> @@ -107,7 +107,7 @@ static void sbi_cpu_stop(void)
>   static int sbi_cpu_is_stopped(unsigned int cpuid)
>   {
>   	int rc;
> -	int hartid = cpuid_to_hartid_map(cpuid);
> +	unsigned long hartid = cpuid_to_hartid_map(cpuid);
>   
>   	rc = sbi_hsm_hart_get_status(hartid);
>
Ard Biesheuvel May 25, 2022, 3:48 p.m. UTC | #2
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.


> +       else
>                 return -EINVAL;
>
> -       hartid = fdt32_to_cpu(*prop);
>         return 0;
>  }
>
> --
> 2.25.1
>
Heinrich Schuchardt May 25, 2022, 3:58 p.m. UTC | #3
On 5/25/22 17:11, Sunil V L wrote:
> The hartid can be a 64bit value on RV64 platforms. This patch
> modifies the hartid parameter in riscv_hartid_to_cpuid() as
> unsigned long so that it can hold 64bit value on RV64 platforms.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>

Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>

> ---
>   arch/riscv/include/asm/smp.h | 4 ++--
>   arch/riscv/kernel/smp.c      | 4 ++--
>   2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/smp.h b/arch/riscv/include/asm/smp.h
> index 23170c933d73..d3443be7eedc 100644
> --- a/arch/riscv/include/asm/smp.h
> +++ b/arch/riscv/include/asm/smp.h
> @@ -42,7 +42,7 @@ void arch_send_call_function_ipi_mask(struct cpumask *mask);
>   /* Hook for the generic smp_call_function_single() routine. */
>   void arch_send_call_function_single_ipi(int cpu);
>   
> -int riscv_hartid_to_cpuid(int hartid);
> +int riscv_hartid_to_cpuid(unsigned long hartid);
>   
>   /* Set custom IPI operations */
>   void riscv_set_ipi_ops(const struct riscv_ipi_ops *ops);
> @@ -70,7 +70,7 @@ static inline void show_ipi_stats(struct seq_file *p, int prec)
>   {
>   }
>   
> -static inline int riscv_hartid_to_cpuid(int hartid)
> +static inline int riscv_hartid_to_cpuid(unsigned long hartid)
>   {
>   	if (hartid == boot_cpu_hartid)
>   		return 0;
> diff --git a/arch/riscv/kernel/smp.c b/arch/riscv/kernel/smp.c
> index b5d30ea92292..018e7dc45df6 100644
> --- a/arch/riscv/kernel/smp.c
> +++ b/arch/riscv/kernel/smp.c
> @@ -47,7 +47,7 @@ static struct {
>   	unsigned long bits ____cacheline_aligned;
>   } ipi_data[NR_CPUS] __cacheline_aligned;
>   
> -int riscv_hartid_to_cpuid(int hartid)
> +int riscv_hartid_to_cpuid(unsigned long hartid)
>   {
>   	int i;
>   
> @@ -55,7 +55,7 @@ int riscv_hartid_to_cpuid(int hartid)
>   		if (cpuid_to_hartid_map(i) == hartid)
>   			return i;
>   
> -	pr_err("Couldn't find cpu id for hartid [%d]\n", hartid);
> +	pr_err("Couldn't find cpu id for hartid [%lu]\n", hartid);
>   	return -ENOENT;
>   }
>
Heinrich Schuchardt May 25, 2022, 4:09 p.m. UTC | #4
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.

Best regards

Heinrich

> 
> 
>> +       else
>>                  return -EINVAL;
>>
>> -       hartid = fdt32_to_cpu(*prop);
>>          return 0;
>>   }
>>
>> --
>> 2.25.1
>>
Atish Patra May 25, 2022, 11:11 p.m. UTC | #5
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
> >>
>
Jessica Clarke May 25, 2022, 11:36 p.m. UTC | #6
On 26 May 2022, at 00:11, Atish Patra <atishp@atishpatra.org> wrote:
> 
> 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.

Why?

Jess

> 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
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Atish Patra May 25, 2022, 11:49 p.m. UTC | #7
On Wed, May 25, 2022 at 4:36 PM Jessica Clarke <jrtc27@jrtc27.com> wrote:
>
> On 26 May 2022, at 00:11, Atish Patra <atishp@atishpatra.org> wrote:
> >
> > 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.
>
> Why?
>
To support prctl call with PR_SET_UNALIGN

> Jess
>
> > 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
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
>
Jessica Clarke May 26, 2022, 12:06 a.m. UTC | #8
On 26 May 2022, at 00:49, Atish Patra <atishp@atishpatra.org> wrote:
> 
> On Wed, May 25, 2022 at 4:36 PM Jessica Clarke <jrtc27@jrtc27.com> wrote:
>> 
>> On 26 May 2022, at 00:11, Atish Patra <atishp@atishpatra.org> wrote:
>>> 
>>> 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.
>> 
>> Why?
>> 
> To support prctl call with PR_SET_UNALIGN

Is that needed? It’s almost entirely unused as far as I can tell, with
all but one use turning unaligned fixups *on*, and the other use being
IA-64-specific. What is the actual use case other than seeing a thing
that exists on some architectures and wanting to have it do something
on RISC-V?

Jess
Sunil V L May 26, 2022, 10:13 a.m. UTC | #9
On Wed, May 25, 2022 at 06:09:05PM +0200, Heinrich Schuchardt 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.

Thank you very much for the feedback. Have updated as per your
suggestion and sent V2.

Thanks
Sunil

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