Re: [PATCH v3 2/4] asm-generic: Provide a fncpy() implementation
On Mon, Jun 19, 2017 at 06:43:48PM +0100, Russell King - ARM Linux wrote: > On Mon, Jun 19, 2017 at 06:18:18PM +0300, Yury Norov wrote: > > One else thing I forgot to ask - now you have the generic > > implementation for fncpy(), so do you really need to save arm > > version of it? > > This was covered in the review of v1, which took the ARM version > and incorrectly used it as an asm-generic implementation. > > I explicitly asked Florian _not_ to copy the ARM fncpy() version > to asm-generic because it has (surprise surprise) ARM specific > behaviours that do not belong in a cross-architecture generic > version. > > Namely, the ARM specific behaviour that bit 0 of a code address is > used to signal whether the code should be executed as ARM code or > as Thumb code. > > This behaviour has no meaning on other architectures (eg, x86) > where code addresses are not 32-bit aligned. > > So, suggesting that the ARM fncpy() should be used as an asm-generic > version is completely absurd, and just because we have an asm-generic > version also does not mean ARM should use it. > > Florian's approach to providing an asm-generic version, leaving the > ARM specific version is entirely correct and appropriate. > > So, in answer to your question, yes, we need _both_ an ARM specific > version and an asm-generic version, where the ARM specific version is > different from the asm-generic version. Purely because it needs > architecture specific details. Hi Russell, Florian, Thanks for clarifications. Thumb bit is a good reason to save arm version, and I completely agree with you in this. Sorry that missed it in the v1 discussion. > I explicitly asked Florian _not_ to copy the ARM fncpy() version > to asm-generic because it has (surprise surprise) ARM specific > behaviours that do not belong in a cross-architecture generic > version. But it seems that v3 does exactly that - copies arm with very small changes. :) Maybe there are good reasons to have arm version exactly how it looks now, but in general case, for me, some things that it does are not needed. I mean checking the alignment of the source and the type of destination. And after some headscratching I became even more convinced that for the general case it would be much preferable to write the fncpy() as regular function in .c file, not a macro, at least to have the corresponding symbol in binary and let the assembler code to call it, which is very probable. Yury
Re: [PATCH v3 2/4] asm-generic: Provide a fncpy() implementation
On Mon, Jun 19, 2017 at 01:58:53PM -0700, Florian Fainelli wrote:
> On 06/18/2017 04:51 PM, Yury Norov wrote:
> > Hi Florian,
> >
> > Some questions and thoughts inline.
> >
> > Yury
> >
> > On Fri, Jun 16, 2017 at 05:07:42PM -0700, Florian Fainelli wrote:
> >> Define a generic fncpy() implementation largely based on the ARM version
> >> that requires an 8 bytes alignment for the destination address where to
> >> copy this function as well as the function's own address.
> >>
> >> Signed-off-by: Florian Fainelli
> >> ---
> >> include/asm-generic/fncpy.h | 93
> >> +
> >> 1 file changed, 93 insertions(+)
> >> create mode 100644 include/asm-generic/fncpy.h
> >>
> >> diff --git a/include/asm-generic/fncpy.h b/include/asm-generic/fncpy.h
> >> new file mode 100644
> >> index ..ec03b83b8535
> >> --- /dev/null
> >> +++ b/include/asm-generic/fncpy.h
> >> @@ -0,0 +1,93 @@
> >> +/*
> >> + * include/asm-generic/fncpy.h - helper macros for function body copying
> >> + *
> >> + * Copyright (C) 2011 Linaro Limited
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify
> >> + * it under the terms of the GNU General Public License version 2 as
> >> + * published by the Free Software Foundation.
> >> + *
> >> + * This program is distributed in the hope that it will be useful,
> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> >> + * GNU General Public License for more details.
> >> + *
> >> + * You should have received a copy of the GNU General Public License
> >> + * along with this program; if not, write to the Free Software
> >> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> >> + */
> >> +
> >> +/*
> >> + * These macros are intended for use when there is a need to copy a
> >> low-level
> >> + * function body into special memory.
> >> + *
> >> + * For example, when reconfiguring the SDRAM controller, the code doing
> >> the
> >> + * reconfiguration may need to run from SRAM.
> >> + *
> >> + * NOTE: that the copied function body must be entirely self-contained and
> >> + * position-independent in order for this to work properly.
> >> + *
> >> + * NOTE: in order for embedded literals and data to get referenced
> >> correctly,
> >> + * the alignment of functions must be preserved when copying. To ensure
> >> this,
> >> + * the source and destination addresses for fncpy() must be aligned to a
> >> + * multiple of 8 bytes: you will be get a BUG() if this condition is not
> >> met.
> >> + * You will typically need a ".align 3" directive in the assembler where
> >> the
> >> + * function to be copied is defined, and ensure that your allocator for
> >> the
> >> + * destination buffer returns 8-byte-aligned pointers.
> >> + *
> >> + * Typical usage example:
> >> + *
> >> + * extern int f(args);
> >> + * extern uint32_t size_of_f;
> >> + * int (*copied_f)(args);
> >> + * void *sram_buffer;
> >> + *
> >> + * copied_f = fncpy(sram_buffer, &f, size_of_f);
> >> + *
> >> + * ... later, call the function: ...
> >> + *
> >> + * copied_f(args);
> >> + *
> >> + * The size of the function to be copied can't be determined from C:
> >> + * this must be determined by other means, such as adding assmbler
> >> directives
> >> + * in the file where f is defined.
> >> + */
> >> +
> >> +#ifndef __ASM_FNCPY_H
> >> +#define __ASM_FNCPY_H
> >> +
> >> +#include
> >> +#include
> >> +
> >> +#include
> >> +#include
> >> +
> >> +/*
> >> + * Minimum alignment requirement for the source and destination addresses
> >> + * for function copying.
> >> + */
> >> +#define FNCPY_ALIGN 8
> >
> > From now this is not arm-only, and it's possible that some architectures
> > might want to redefine it in their arch/xxx/include/asm/fncpy.h files.
> > So it will be easier for them if you'll wrap FNCPY_ALIGN here with #ifdef
> > guards.
> >
> > By the way, compiler already has an information on the proper alignment.
> > Maybe it's better to use it as the default value here instead of the
> > hardcoded value?
> >
> > #ifndef FNCPY_ALIGN
> > #define FNCPY_ALIGN ({void foo(); __alignof__(&foo);})
> > #endif
> >
> >> +
> >> +#define fncpy(dest_buf, funcp, size) ({
> >> \
> >
> > Do you really need to check types inside the macro? If not, you can
> > declare it as function, which is better in general, with the memcpy-like
> > prototype.
> >
> >> + uintptr_t __funcp_address; \
> >> + typeof(funcp) __result; \
> >> + \
> >> + asm("" : "=r" (__funcp_address) : "0" (funcp)); \
> >> + \
> >> + /* \
> >> + * Ensure alignment of sourc
Re: [PATCH v3 2/4] asm-generic: Provide a fncpy() implementation
On 06/18/2017 04:51 PM, Yury Norov wrote:
> Hi Florian,
>
> Some questions and thoughts inline.
>
> Yury
>
> On Fri, Jun 16, 2017 at 05:07:42PM -0700, Florian Fainelli wrote:
>> Define a generic fncpy() implementation largely based on the ARM version
>> that requires an 8 bytes alignment for the destination address where to
>> copy this function as well as the function's own address.
>>
>> Signed-off-by: Florian Fainelli
>> ---
>> include/asm-generic/fncpy.h | 93
>> +
>> 1 file changed, 93 insertions(+)
>> create mode 100644 include/asm-generic/fncpy.h
>>
>> diff --git a/include/asm-generic/fncpy.h b/include/asm-generic/fncpy.h
>> new file mode 100644
>> index ..ec03b83b8535
>> --- /dev/null
>> +++ b/include/asm-generic/fncpy.h
>> @@ -0,0 +1,93 @@
>> +/*
>> + * include/asm-generic/fncpy.h - helper macros for function body copying
>> + *
>> + * Copyright (C) 2011 Linaro Limited
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
>> + */
>> +
>> +/*
>> + * These macros are intended for use when there is a need to copy a
>> low-level
>> + * function body into special memory.
>> + *
>> + * For example, when reconfiguring the SDRAM controller, the code doing the
>> + * reconfiguration may need to run from SRAM.
>> + *
>> + * NOTE: that the copied function body must be entirely self-contained and
>> + * position-independent in order for this to work properly.
>> + *
>> + * NOTE: in order for embedded literals and data to get referenced
>> correctly,
>> + * the alignment of functions must be preserved when copying. To ensure
>> this,
>> + * the source and destination addresses for fncpy() must be aligned to a
>> + * multiple of 8 bytes: you will be get a BUG() if this condition is not
>> met.
>> + * You will typically need a ".align 3" directive in the assembler where the
>> + * function to be copied is defined, and ensure that your allocator for the
>> + * destination buffer returns 8-byte-aligned pointers.
>> + *
>> + * Typical usage example:
>> + *
>> + * extern int f(args);
>> + * extern uint32_t size_of_f;
>> + * int (*copied_f)(args);
>> + * void *sram_buffer;
>> + *
>> + * copied_f = fncpy(sram_buffer, &f, size_of_f);
>> + *
>> + * ... later, call the function: ...
>> + *
>> + * copied_f(args);
>> + *
>> + * The size of the function to be copied can't be determined from C:
>> + * this must be determined by other means, such as adding assmbler
>> directives
>> + * in the file where f is defined.
>> + */
>> +
>> +#ifndef __ASM_FNCPY_H
>> +#define __ASM_FNCPY_H
>> +
>> +#include
>> +#include
>> +
>> +#include
>> +#include
>> +
>> +/*
>> + * Minimum alignment requirement for the source and destination addresses
>> + * for function copying.
>> + */
>> +#define FNCPY_ALIGN 8
>
> From now this is not arm-only, and it's possible that some architectures
> might want to redefine it in their arch/xxx/include/asm/fncpy.h files.
> So it will be easier for them if you'll wrap FNCPY_ALIGN here with #ifdef
> guards.
>
> By the way, compiler already has an information on the proper alignment.
> Maybe it's better to use it as the default value here instead of the
> hardcoded value?
>
> #ifndef FNCPY_ALIGN
> #define FNCPY_ALIGN ({void foo(); __alignof__(&foo);})
> #endif
>
>> +
>> +#define fncpy(dest_buf, funcp, size) ({
>> \
>
> Do you really need to check types inside the macro? If not, you can
> declare it as function, which is better in general, with the memcpy-like
> prototype.
>
>> +uintptr_t __funcp_address; \
>> +typeof(funcp) __result; \
>> +\
>> +asm("" : "=r" (__funcp_address) : "0" (funcp)); \
>> +\
>> +/* \
>> + * Ensure alignment of source and destination addresses.\
>> + */ \
>> +BUG_ON((uintptr_t)(dest_buf) & (FNCPY_ALIGN - 1) || \
>
> People don't like new BUG_ONs. Maybe it's better to use BUILD_BUG_ON()
> at compile time and WARN_ON() at runtime?
If you have a BUI
Re: [PATCH v3 2/4] asm-generic: Provide a fncpy() implementation
On Mon, Jun 19, 2017 at 06:18:18PM +0300, Yury Norov wrote: > One else thing I forgot to ask - now you have the generic > implementation for fncpy(), so do you really need to save arm > version of it? This was covered in the review of v1, which took the ARM version and incorrectly used it as an asm-generic implementation. I explicitly asked Florian _not_ to copy the ARM fncpy() version to asm-generic because it has (surprise surprise) ARM specific behaviours that do not belong in a cross-architecture generic version. Namely, the ARM specific behaviour that bit 0 of a code address is used to signal whether the code should be executed as ARM code or as Thumb code. This behaviour has no meaning on other architectures (eg, x86) where code addresses are not 32-bit aligned. So, suggesting that the ARM fncpy() should be used as an asm-generic version is completely absurd, and just because we have an asm-generic version also does not mean ARM should use it. Florian's approach to providing an asm-generic version, leaving the ARM specific version is entirely correct and appropriate. So, in answer to your question, yes, we need _both_ an ARM specific version and an asm-generic version, where the ARM specific version is different from the asm-generic version. Purely because it needs architecture specific details. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Re: [PATCH v3 2/4] asm-generic: Provide a fncpy() implementation
On 06/19/2017 08:18 AM, Yury Norov wrote: > On Mon, Jun 19, 2017 at 02:51:08AM +0300, Yury Norov wrote: >> Hi Florian, >> >> Some questions and thoughts inline. >> >> Yury >> >> On Fri, Jun 16, 2017 at 05:07:42PM -0700, Florian Fainelli wrote: >>> Define a generic fncpy() implementation largely based on the ARM version >>> that requires an 8 bytes alignment for the destination address where to >>> copy this function as well as the function's own address. >>> >>> Signed-off-by: Florian Fainelli >>> --- >>> include/asm-generic/fncpy.h | 93 >>> + >>> 1 file changed, 93 insertions(+) >>> create mode 100644 include/asm-generic/fncpy.h > > One else thing I forgot to ask - now you have the generic > implementation for fncpy(), so do you really need to save arm > version of it? Yes, it needs to deal with the Thumb bit, and there is no point in making the generic implementation extremely flexible to support that special case. Thanks -- Florian
Re: [PATCH v3 2/4] asm-generic: Provide a fncpy() implementation
On Mon, Jun 19, 2017 at 02:51:08AM +0300, Yury Norov wrote: > Hi Florian, > > Some questions and thoughts inline. > > Yury > > On Fri, Jun 16, 2017 at 05:07:42PM -0700, Florian Fainelli wrote: > > Define a generic fncpy() implementation largely based on the ARM version > > that requires an 8 bytes alignment for the destination address where to > > copy this function as well as the function's own address. > > > > Signed-off-by: Florian Fainelli > > --- > > include/asm-generic/fncpy.h | 93 > > + > > 1 file changed, 93 insertions(+) > > create mode 100644 include/asm-generic/fncpy.h One else thing I forgot to ask - now you have the generic implementation for fncpy(), so do you really need to save arm version of it? Yury
Re: [PATCH v3 2/4] asm-generic: Provide a fncpy() implementation
> > +/*
> > + * Minimum alignment requirement for the source and destination addresses
> > + * for function copying.
> > + */
> > +#define FNCPY_ALIGN 8
>
> >From now this is not arm-only, and it's possible that some architectures
> might want to redefine it in their arch/xxx/include/asm/fncpy.h files.
> So it will be easier for them if you'll wrap FNCPY_ALIGN here with #ifdef
> guards.
>
> By the way, compiler already has an information on the proper alignment.
> Maybe it's better to use it as the default value here instead of the
> hardcoded value?
>
> #ifndef FNCPY_ALIGN
> #define FNCPY_ALIGN ({void foo(); __alignof__(&foo);})
> #endif
Ah sorry, at first it should be like this:
#define FNCPY_ALIGN ({void foo(); __alignof__(foo);})
And at second, the correct version returns 1 always.
Even if I pass falign-functions=4096 to gcc, and I see that functions
are aligned accordingly in elf file, the macro returns 1 anyway. So if
it doesn't work, the hardcoded '8' is the only option.
Yury
Re: [PATCH v3 2/4] asm-generic: Provide a fncpy() implementation
Hi Florian,
Some questions and thoughts inline.
Yury
On Fri, Jun 16, 2017 at 05:07:42PM -0700, Florian Fainelli wrote:
> Define a generic fncpy() implementation largely based on the ARM version
> that requires an 8 bytes alignment for the destination address where to
> copy this function as well as the function's own address.
>
> Signed-off-by: Florian Fainelli
> ---
> include/asm-generic/fncpy.h | 93
> +
> 1 file changed, 93 insertions(+)
> create mode 100644 include/asm-generic/fncpy.h
>
> diff --git a/include/asm-generic/fncpy.h b/include/asm-generic/fncpy.h
> new file mode 100644
> index ..ec03b83b8535
> --- /dev/null
> +++ b/include/asm-generic/fncpy.h
> @@ -0,0 +1,93 @@
> +/*
> + * include/asm-generic/fncpy.h - helper macros for function body copying
> + *
> + * Copyright (C) 2011 Linaro Limited
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + */
> +
> +/*
> + * These macros are intended for use when there is a need to copy a low-level
> + * function body into special memory.
> + *
> + * For example, when reconfiguring the SDRAM controller, the code doing the
> + * reconfiguration may need to run from SRAM.
> + *
> + * NOTE: that the copied function body must be entirely self-contained and
> + * position-independent in order for this to work properly.
> + *
> + * NOTE: in order for embedded literals and data to get referenced correctly,
> + * the alignment of functions must be preserved when copying. To ensure
> this,
> + * the source and destination addresses for fncpy() must be aligned to a
> + * multiple of 8 bytes: you will be get a BUG() if this condition is not met.
> + * You will typically need a ".align 3" directive in the assembler where the
> + * function to be copied is defined, and ensure that your allocator for the
> + * destination buffer returns 8-byte-aligned pointers.
> + *
> + * Typical usage example:
> + *
> + * extern int f(args);
> + * extern uint32_t size_of_f;
> + * int (*copied_f)(args);
> + * void *sram_buffer;
> + *
> + * copied_f = fncpy(sram_buffer, &f, size_of_f);
> + *
> + * ... later, call the function: ...
> + *
> + * copied_f(args);
> + *
> + * The size of the function to be copied can't be determined from C:
> + * this must be determined by other means, such as adding assmbler directives
> + * in the file where f is defined.
> + */
> +
> +#ifndef __ASM_FNCPY_H
> +#define __ASM_FNCPY_H
> +
> +#include
> +#include
> +
> +#include
> +#include
> +
> +/*
> + * Minimum alignment requirement for the source and destination addresses
> + * for function copying.
> + */
> +#define FNCPY_ALIGN 8
>From now this is not arm-only, and it's possible that some architectures
might want to redefine it in their arch/xxx/include/asm/fncpy.h files.
So it will be easier for them if you'll wrap FNCPY_ALIGN here with #ifdef
guards.
By the way, compiler already has an information on the proper alignment.
Maybe it's better to use it as the default value here instead of the
hardcoded value?
#ifndef FNCPY_ALIGN
#define FNCPY_ALIGN ({void foo(); __alignof__(&foo);})
#endif
> +
> +#define fncpy(dest_buf, funcp, size) ({
> \
Do you really need to check types inside the macro? If not, you can
declare it as function, which is better in general, with the memcpy-like
prototype.
> + uintptr_t __funcp_address; \
> + typeof(funcp) __result; \
> + \
> + asm("" : "=r" (__funcp_address) : "0" (funcp)); \
> + \
> + /* \
> + * Ensure alignment of source and destination addresses.\
> + */ \
> + BUG_ON((uintptr_t)(dest_buf) & (FNCPY_ALIGN - 1) || \
People don't like new BUG_ONs. Maybe it's better to use BUILD_BUG_ON()
at compile time and WARN_ON() at runtime?
> + (__funcp_address & (FNCPY_ALIGN - 1))); \
There is IS_ALIGNED() macro for things like this.
And I frankly don't understand the 2nd check. One can imagine the
situation when someone want
[PATCH v3 2/4] asm-generic: Provide a fncpy() implementation
Define a generic fncpy() implementation largely based on the ARM version
that requires an 8 bytes alignment for the destination address where to
copy this function as well as the function's own address.
Signed-off-by: Florian Fainelli
---
include/asm-generic/fncpy.h | 93 +
1 file changed, 93 insertions(+)
create mode 100644 include/asm-generic/fncpy.h
diff --git a/include/asm-generic/fncpy.h b/include/asm-generic/fncpy.h
new file mode 100644
index ..ec03b83b8535
--- /dev/null
+++ b/include/asm-generic/fncpy.h
@@ -0,0 +1,93 @@
+/*
+ * include/asm-generic/fncpy.h - helper macros for function body copying
+ *
+ * Copyright (C) 2011 Linaro Limited
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+/*
+ * These macros are intended for use when there is a need to copy a low-level
+ * function body into special memory.
+ *
+ * For example, when reconfiguring the SDRAM controller, the code doing the
+ * reconfiguration may need to run from SRAM.
+ *
+ * NOTE: that the copied function body must be entirely self-contained and
+ * position-independent in order for this to work properly.
+ *
+ * NOTE: in order for embedded literals and data to get referenced correctly,
+ * the alignment of functions must be preserved when copying. To ensure this,
+ * the source and destination addresses for fncpy() must be aligned to a
+ * multiple of 8 bytes: you will be get a BUG() if this condition is not met.
+ * You will typically need a ".align 3" directive in the assembler where the
+ * function to be copied is defined, and ensure that your allocator for the
+ * destination buffer returns 8-byte-aligned pointers.
+ *
+ * Typical usage example:
+ *
+ * extern int f(args);
+ * extern uint32_t size_of_f;
+ * int (*copied_f)(args);
+ * void *sram_buffer;
+ *
+ * copied_f = fncpy(sram_buffer, &f, size_of_f);
+ *
+ * ... later, call the function: ...
+ *
+ * copied_f(args);
+ *
+ * The size of the function to be copied can't be determined from C:
+ * this must be determined by other means, such as adding assmbler directives
+ * in the file where f is defined.
+ */
+
+#ifndef __ASM_FNCPY_H
+#define __ASM_FNCPY_H
+
+#include
+#include
+
+#include
+#include
+
+/*
+ * Minimum alignment requirement for the source and destination addresses
+ * for function copying.
+ */
+#define FNCPY_ALIGN 8
+
+#define fncpy(dest_buf, funcp, size) ({
\
+ uintptr_t __funcp_address; \
+ typeof(funcp) __result; \
+ \
+ asm("" : "=r" (__funcp_address) : "0" (funcp)); \
+ \
+ /* \
+* Ensure alignment of source and destination addresses.\
+*/ \
+ BUG_ON((uintptr_t)(dest_buf) & (FNCPY_ALIGN - 1) || \
+ (__funcp_address & (FNCPY_ALIGN - 1))); \
+ \
+ memcpy(dest_buf, (void const *)__funcp_address, size); \
+ flush_icache_range((unsigned long)(dest_buf), \
+ (unsigned long)(dest_buf) + (size));\
+ \
+ asm("" : "=r" (__result)\
+ : "0" ((uintptr_t)(dest_buf))); \
+ \
+ __result; \
+})
+
+#endif /* !__ASM_FNCPY_H */
--
2.9.3
