| Message ID | 20220628125938.694256-1-sudeep.holla@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: efi: Simplify arch_efi_call_virt() macro by using typeof() | expand |
On Tue, 28 Jun 2022 at 14:59, Sudeep Holla <sudeep.holla@arm.com> wrote: > > Currently, the arch_efi_call_virt() assumes all users of it will have > defined a type 'efi_##f##_t' to make use of it. It is unnecessarily > forcing the users to create a new typedef when __efi_rt_asm_wrapper() > actually expects void pointer. > > Simplify the arch_efi_call_virt() macro by using typeof(p->f) which must > be a pointer as required by __efi_rt_asm_wrapper() and eliminate the > explicit need for efi_##f##_t type for every user of this macro. > > This change is done to align with implementations on other similar > architectures. > > Cc: Ard Biesheuvel <ardb@kernel.org> > Cc: Russell King <linux@armlinux.org.uk> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- > arch/arm/include/asm/efi.h | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > Hi, > > Reference for this change [1] and in particular[2] > > Regards, > Sudeep > > [1] https://lore.kernel.org/r/20220628125346.693304-1-sudeep.holla@arm.com > [2] https://lore.kernel.org/r/20220628125346.693304-3-sudeep.holla@arm.com/ > > diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h > index 27218eabbf9a..d4a405c9b4b6 100644 > --- a/arch/arm/include/asm/efi.h > +++ b/arch/arm/include/asm/efi.h > @@ -26,8 +26,7 @@ int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md); > > #define arch_efi_call_virt(p, f, args...) \ > ({ \ > - efi_##f##_t *__f; \ > - __f = p->f; \ > + typeof(p->f) __f = p->f; \ > __f(args); \ > }) > I think this could simply be #define arch_efi_call_virt(p, f, args...) ((p)->f(args)) no?
On Tue, Jun 28, 2022 at 03:16:26PM +0200, Ard Biesheuvel wrote: > On Tue, 28 Jun 2022 at 14:59, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > Currently, the arch_efi_call_virt() assumes all users of it will have > > defined a type 'efi_##f##_t' to make use of it. It is unnecessarily > > forcing the users to create a new typedef when __efi_rt_asm_wrapper() > > actually expects void pointer. > > > > Simplify the arch_efi_call_virt() macro by using typeof(p->f) which must > > be a pointer as required by __efi_rt_asm_wrapper() and eliminate the > > explicit need for efi_##f##_t type for every user of this macro. > > > > This change is done to align with implementations on other similar > > architectures. > > > > Cc: Ard Biesheuvel <ardb@kernel.org> > > Cc: Russell King <linux@armlinux.org.uk> > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > > --- > > arch/arm/include/asm/efi.h | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > Hi, > > > > Reference for this change [1] and in particular[2] > > > > Regards, > > Sudeep > > > > [1] https://lore.kernel.org/r/20220628125346.693304-1-sudeep.holla@arm.com > > [2] https://lore.kernel.org/r/20220628125346.693304-3-sudeep.holla@arm.com/ > > > > diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h > > index 27218eabbf9a..d4a405c9b4b6 100644 > > --- a/arch/arm/include/asm/efi.h > > +++ b/arch/arm/include/asm/efi.h > > @@ -26,8 +26,7 @@ int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md); > > > > #define arch_efi_call_virt(p, f, args...) \ > > ({ \ > > - efi_##f##_t *__f; \ > > - __f = p->f; \ > > + typeof(p->f) __f = p->f; \ > > __f(args); \ > > }) > > > > I think this could simply be > > #define arch_efi_call_virt(p, f, args...) ((p)->f(args)) > > no? Yes, I came to similar conclusion just after sending this out as I started to look if we can have one generic definition for arm/arm64/riscv/loongarch. I am yet to figure out how asm/efi.h and linux/efi.h are included so that we can have generic definition in linux/efi.h and x86 can undefine that and redefine its own version. Does that make sense ? -- Regards, Sudeep
On Tue, 28 Jun 2022 at 15:47, Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Tue, Jun 28, 2022 at 03:16:26PM +0200, Ard Biesheuvel wrote: > > On Tue, 28 Jun 2022 at 14:59, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > > > Currently, the arch_efi_call_virt() assumes all users of it will have > > > defined a type 'efi_##f##_t' to make use of it. It is unnecessarily > > > forcing the users to create a new typedef when __efi_rt_asm_wrapper() > > > actually expects void pointer. > > > > > > Simplify the arch_efi_call_virt() macro by using typeof(p->f) which must > > > be a pointer as required by __efi_rt_asm_wrapper() and eliminate the > > > explicit need for efi_##f##_t type for every user of this macro. > > > > > > This change is done to align with implementations on other similar > > > architectures. > > > > > > Cc: Ard Biesheuvel <ardb@kernel.org> > > > Cc: Russell King <linux@armlinux.org.uk> > > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > > > --- > > > arch/arm/include/asm/efi.h | 3 +-- > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > Hi, > > > > > > Reference for this change [1] and in particular[2] > > > > > > Regards, > > > Sudeep > > > > > > [1] https://lore.kernel.org/r/20220628125346.693304-1-sudeep.holla@arm.com > > > [2] https://lore.kernel.org/r/20220628125346.693304-3-sudeep.holla@arm.com/ > > > > > > diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h > > > index 27218eabbf9a..d4a405c9b4b6 100644 > > > --- a/arch/arm/include/asm/efi.h > > > +++ b/arch/arm/include/asm/efi.h > > > @@ -26,8 +26,7 @@ int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md); > > > > > > #define arch_efi_call_virt(p, f, args...) \ > > > ({ \ > > > - efi_##f##_t *__f; \ > > > - __f = p->f; \ > > > + typeof(p->f) __f = p->f; \ > > > __f(args); \ > > > }) > > > > > > > I think this could simply be > > > > #define arch_efi_call_virt(p, f, args...) ((p)->f(args)) > > > > no? > > Yes, I came to similar conclusion just after sending this out as I started to > look if we can have one generic definition for arm/arm64/riscv/loongarch. > Not really - arm64 has the asm wrapper, and loongarch is only halfway merged so I'm not sure yet if this is the final form. > I am yet to figure out how asm/efi.h and linux/efi.h are included so that > we can have generic definition in linux/efi.h and x86 can undefine that > and redefine its own version. > > Does that make sense ? > I appreciate the effort, but for now, let's just fix the ones we need to fix (and the ARM one too while we're at it). PRM can only be enabled on x86 and arm64 anyway.
On Tue, Jun 28, 2022 at 03:57:38PM +0200, Ard Biesheuvel wrote: > On Tue, 28 Jun 2022 at 15:47, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > On Tue, Jun 28, 2022 at 03:16:26PM +0200, Ard Biesheuvel wrote: > > > On Tue, 28 Jun 2022 at 14:59, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > > > > > Currently, the arch_efi_call_virt() assumes all users of it will have > > > > defined a type 'efi_##f##_t' to make use of it. It is unnecessarily > > > > forcing the users to create a new typedef when __efi_rt_asm_wrapper() > > > > actually expects void pointer. > > > > > > > > Simplify the arch_efi_call_virt() macro by using typeof(p->f) which must > > > > be a pointer as required by __efi_rt_asm_wrapper() and eliminate the > > > > explicit need for efi_##f##_t type for every user of this macro. > > > > > > > > This change is done to align with implementations on other similar > > > > architectures. > > > > > > > > Cc: Ard Biesheuvel <ardb@kernel.org> > > > > Cc: Russell King <linux@armlinux.org.uk> > > > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > > > > --- > > > > arch/arm/include/asm/efi.h | 3 +-- > > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > > > Hi, > > > > > > > > Reference for this change [1] and in particular[2] > > > > > > > > Regards, > > > > Sudeep > > > > > > > > [1] https://lore.kernel.org/r/20220628125346.693304-1-sudeep.holla@arm.com > > > > [2] https://lore.kernel.org/r/20220628125346.693304-3-sudeep.holla@arm.com/ > > > > > > > > diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h > > > > index 27218eabbf9a..d4a405c9b4b6 100644 > > > > --- a/arch/arm/include/asm/efi.h > > > > +++ b/arch/arm/include/asm/efi.h > > > > @@ -26,8 +26,7 @@ int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md); > > > > > > > > #define arch_efi_call_virt(p, f, args...) \ > > > > ({ \ > > > > - efi_##f##_t *__f; \ > > > > - __f = p->f; \ > > > > + typeof(p->f) __f = p->f; \ > > > > __f(args); \ > > > > }) > > > > > > > > > > I think this could simply be > > > > > > #define arch_efi_call_virt(p, f, args...) ((p)->f(args)) > > > > > > no? > > > > Yes, I came to similar conclusion just after sending this out as I started to > > look if we can have one generic definition for arm/arm64/riscv/loongarch. > > > > Not really - arm64 has the asm wrapper, and loongarch is only halfway > merged so I'm not sure yet if this is the final form. > Aargh! arm64 was typo, indeed arm64 has wrapper. I meant to refer other 3 archs. > > I am yet to figure out how asm/efi.h and linux/efi.h are included so that > > we can have generic definition in linux/efi.h and x86 can undefine that > > and redefine its own version. > > > > Does that make sense ? > > > > I appreciate the effort, but for now, let's just fix the ones we need > to fix (and the ARM one too while we're at it). PRM can only be > enabled on x86 and arm64 anyway. True. OK then I will just update ARM version and leave loongarch as is. -- Regards, Sudeep
On Tue, 28 Jun 2022 at 16:09, Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Tue, Jun 28, 2022 at 03:57:38PM +0200, Ard Biesheuvel wrote: > > On Tue, 28 Jun 2022 at 15:47, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > ... > > > I am yet to figure out how asm/efi.h and linux/efi.h are included so that > > > we can have generic definition in linux/efi.h and x86 can undefine that > > > and redefine its own version. > > > > > > Does that make sense ? > > > > > > > I appreciate the effort, but for now, let's just fix the ones we need > > to fix (and the ARM one too while we're at it). PRM can only be > > enabled on x86 and arm64 anyway. > > True. OK then I will just update ARM version and leave loongarch as is. > Actually, this was rather straight-forward so I folded this change into your ARM patch.
On Tue, Jun 28, 2022 at 07:58:38PM +0200, Ard Biesheuvel wrote: > On Tue, 28 Jun 2022 at 16:09, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > On Tue, Jun 28, 2022 at 03:57:38PM +0200, Ard Biesheuvel wrote: > > > On Tue, 28 Jun 2022 at 15:47, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > > ... > > > > > I am yet to figure out how asm/efi.h and linux/efi.h are included so that > > > > we can have generic definition in linux/efi.h and x86 can undefine that > > > > and redefine its own version. > > > > > > > > Does that make sense ? > > > > > > > > > > I appreciate the effort, but for now, let's just fix the ones we need > > > to fix (and the ARM one too while we're at it). PRM can only be > > > enabled on x86 and arm64 anyway. > > > > True. OK then I will just update ARM version and leave loongarch as is. > > > > Actually, this was rather straight-forward so I folded this change > into your ARM patch. I see you have the generic version for all archs except arm64 and x86 as we discussed earlier. Since you have even included the arm64 changes, the PRMT enablement patches need to routed via your tree now as it depends on the change you have in your -next. Are you OK with that if Rafael agrees ? I can ask him on the other thread. No further changes are needed. Let me know.
On Wed, 29 Jun 2022 at 10:57, Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Tue, Jun 28, 2022 at 07:58:38PM +0200, Ard Biesheuvel wrote: > > On Tue, 28 Jun 2022 at 16:09, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > > > On Tue, Jun 28, 2022 at 03:57:38PM +0200, Ard Biesheuvel wrote: > > > > On Tue, 28 Jun 2022 at 15:47, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > > > > ... > > > > > > > I am yet to figure out how asm/efi.h and linux/efi.h are included so that > > > > > we can have generic definition in linux/efi.h and x86 can undefine that > > > > > and redefine its own version. > > > > > > > > > > Does that make sense ? > > > > > > > > > > > > > I appreciate the effort, but for now, let's just fix the ones we need > > > > to fix (and the ARM one too while we're at it). PRM can only be > > > > enabled on x86 and arm64 anyway. > > > > > > True. OK then I will just update ARM version and leave loongarch as is. > > > > > > > Actually, this was rather straight-forward so I folded this change > > into your ARM patch. > > I see you have the generic version for all archs except arm64 and x86 as > we discussed earlier. Since you have even included the arm64 changes, the > PRMT enablement patches need to routed via your tree now as it depends on > the change you have in your -next. > > Are you OK with that if Rafael agrees ? I can ask him on the other thread. > No further changes are needed. Let me know. > Yes, that is fine. Or I can put that patch on a stable branch by itself.
On Wed, Jun 29, 2022 at 10:58:29AM +0200, Ard Biesheuvel wrote: > On Wed, 29 Jun 2022 at 10:57, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > On Tue, Jun 28, 2022 at 07:58:38PM +0200, Ard Biesheuvel wrote: > > > On Tue, 28 Jun 2022 at 16:09, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > > > > > On Tue, Jun 28, 2022 at 03:57:38PM +0200, Ard Biesheuvel wrote: > > > > > On Tue, 28 Jun 2022 at 15:47, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > > > > > > ... > > > > > > > > > I am yet to figure out how asm/efi.h and linux/efi.h are included so that > > > > > > we can have generic definition in linux/efi.h and x86 can undefine that > > > > > > and redefine its own version. > > > > > > > > > > > > Does that make sense ? > > > > > > > > > > > > > > > > I appreciate the effort, but for now, let's just fix the ones we need > > > > > to fix (and the ARM one too while we're at it). PRM can only be > > > > > enabled on x86 and arm64 anyway. > > > > > > > > True. OK then I will just update ARM version and leave loongarch as is. > > > > > > > > > > Actually, this was rather straight-forward so I folded this change > > > into your ARM patch. > > > > I see you have the generic version for all archs except arm64 and x86 as > > we discussed earlier. Since you have even included the arm64 changes, the > > PRMT enablement patches need to routed via your tree now as it depends on > > the change you have in your -next. > > > > Are you OK with that if Rafael agrees ? I can ask him on the other thread. > > No further changes are needed. Let me know. > > > > Yes, that is fine. Or I can put that patch on a stable branch by itself. Thanks I will check with Rafael now.
diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h index 27218eabbf9a..d4a405c9b4b6 100644 --- a/arch/arm/include/asm/efi.h +++ b/arch/arm/include/asm/efi.h @@ -26,8 +26,7 @@ int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md); #define arch_efi_call_virt(p, f, args...) \ ({ \ - efi_##f##_t *__f; \ - __f = p->f; \ + typeof(p->f) __f = p->f; \ __f(args); \ })
Currently, the arch_efi_call_virt() assumes all users of it will have defined a type 'efi_##f##_t' to make use of it. It is unnecessarily forcing the users to create a new typedef when __efi_rt_asm_wrapper() actually expects void pointer. Simplify the arch_efi_call_virt() macro by using typeof(p->f) which must be a pointer as required by __efi_rt_asm_wrapper() and eliminate the explicit need for efi_##f##_t type for every user of this macro. This change is done to align with implementations on other similar architectures. Cc: Ard Biesheuvel <ardb@kernel.org> Cc: Russell King <linux@armlinux.org.uk> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> --- arch/arm/include/asm/efi.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) Hi, Reference for this change [1] and in particular[2] Regards, Sudeep [1] https://lore.kernel.org/r/20220628125346.693304-1-sudeep.holla@arm.com [2] https://lore.kernel.org/r/20220628125346.693304-3-sudeep.holla@arm.com/