| Message ID | 20220525151106.2176147-6-sunilvl@ventanamicro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support for 64bit hartid on RV64 platforms | expand |
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 >
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 >>
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 > >> >
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
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 >
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
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 > > > >
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); + else return -EINVAL; - hartid = fdt32_to_cpu(*prop); return 0; }
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(-)