The patch cleans up the file lib/decompress_unxz.c by removing all memory
helper functions, e.g., memmove. By doing so, any architecture's preboot
environment supporting the XZ decompression needs to define its own copy of
any of the missing memory helper functions.
The patch makes sure that all 4 architectures' preboot currently supporting
the XZ decompressor, arm, s390, sh and x86, build without error when using
the XZ compression.
Adding a prototype for the memcmp function required by the XZ decompressor to
workaround compiler's implicit type error. Also removing both the memmove and
memcpy defines workaround, to disable lib/decompress_unxz.c from duplicating
both functions.
Adding the missing memcmp function, required by the XZ decompressor, to the
s390 preboot environment.
Adding both the missing memmove and memcmp functions, required by the XZ
decompressor, to the sh preboot environment.
Adding the missing memmove function, required by XZ decompressor, to the x86
preboot environment.
Signed-off-by: T. Makphaibulchoke <[email protected]>
---
arch/arm/boot/compressed/decompress.c | 3 +-
arch/s390/boot/compressed/misc.c | 14 +++++
arch/sh/boot/compressed/misc.c | 32 ++++++++++++
arch/x86/boot/compressed/string.c | 18 +++++++
lib/decompress_unxz.c | 85 +++++++--------------------------
lib/xz/xz_private.h | 7 ++-
6 files changed, 87 insertions(+), 72 deletions(-)
diff --git a/arch/arm/boot/compressed/decompress.c b/arch/arm/boot/compressed/decompress.c
index f41b38c..80a2219 100644
--- a/arch/arm/boot/compressed/decompress.c
+++ b/arch/arm/boot/compressed/decompress.c
@@ -9,6 +9,7 @@
extern unsigned long free_mem_ptr;
extern unsigned long free_mem_end_ptr;
extern void error(char *);
+extern int memcmp(const void *, const void *, size_t);
#define STATIC static
#define STATIC_RW_DATA /* non-static please */
@@ -45,8 +46,6 @@ extern void error(char *);
#endif
#ifdef CONFIG_KERNEL_XZ
-#define memmove memmove
-#define memcpy memcpy
#include "../../../../lib/decompress_unxz.c"
#endif
diff --git a/arch/s390/boot/compressed/misc.c b/arch/s390/boot/compressed/misc.c
index 465eca7..b966af3 100644
--- a/arch/s390/boot/compressed/misc.c
+++ b/arch/s390/boot/compressed/misc.c
@@ -87,6 +87,20 @@ void *memcpy(void *__dest, __const void *__src, size_t __n)
return __builtin_memcpy(__dest, __src, __n);
}
+int memcmp(const void *cs, const void *ct, size_t count)
+{
+ const unsigned char *su1, *su2;
+ int res = 0;
+
+ for (su1 = cs, su2 = ct; 0 < count; ++su1, ++su2, count--) {
+ res = *su1 - *su2;
+ if (res != 0)
+ break;
+ }
+
+ return res;
+}
+
void *memmove(void *__dest, __const void *__src, size_t __n)
{
char *d;
diff --git a/arch/sh/boot/compressed/misc.c b/arch/sh/boot/compressed/misc.c
index 95470a4..09e740d 100644
--- a/arch/sh/boot/compressed/misc.c
+++ b/arch/sh/boot/compressed/misc.c
@@ -94,6 +94,38 @@ void* memcpy(void* __dest, __const void* __src,
return __dest;
}
+void *memmove(void *dest, const void *src, size_t size)
+{
+ uint8_t *d = dest;
+ const uint8_t *s = src;
+ size_t i;
+
+ if (d < s) {
+ for (i = 0; i < size; ++i)
+ d[i] = s[i];
+ } else if (d > s) {
+ i = size;
+ while (i-- > 0)
+ d[i] = s[i];
+ }
+
+ return dest;
+}
+
+int memcmp(const void *cs, const void *ct, size_t count)
+{
+ const unsigned char *su1, *su2;
+ int res = 0;
+
+ for (su1 = cs, su2 = ct; 0 < count; ++su1, ++su2, count--) {
+ res = *su1 - *su2;
+ if (res != 0)
+ break;
+ }
+
+ return res;
+}
+
static void error(char *x)
{
puts("\n\n");
diff --git a/arch/x86/boot/compressed/string.c b/arch/x86/boot/compressed/string.c
index ffb9c5c..2f4fdfa 100644
--- a/arch/x86/boot/compressed/string.c
+++ b/arch/x86/boot/compressed/string.c
@@ -8,4 +8,22 @@ int memcmp(const void *s1, const void *s2, size_t len)
return diff;
}
+void *memmove(void *dest, const void *src, size_t size)
+{
+ uint8_t *d = dest;
+ const uint8_t *s = src;
+ size_t i;
+
+ if (d < s) {
+ for (i = 0; i < size; ++i)
+ d[i] = s[i];
+ } else if (d > s) {
+ i = size;
+ while (i-- > 0)
+ d[i] = s[i];
+ }
+
+ return dest;
+}
+
#include "../string.c"
diff --git a/lib/decompress_unxz.c b/lib/decompress_unxz.c
index 9f34eb5..bd09c2f 100644
--- a/lib/decompress_unxz.c
+++ b/lib/decompress_unxz.c
@@ -160,75 +160,24 @@
#define vfree(ptr) do { if (ptr != NULL) free(ptr); } while (0)
/*
- * FIXME: Not all basic memory functions are provided in architecture-specific
- * files (yet). We define our own versions here for now, but this should be
- * only a temporary solution.
+ * To support XZ-decompressed file in preboot environment, the following
+ * functions, memcmp, memset, memcpy and memmove, needed to be defined in
+ * architecture-specific preboot environment.
+ *
+ * Not all architecture-specific preboot environment currently support all of
+ * the above functions.
+ *
+ * If your architecture does not support any of the above functions, simply
+ * add the missing function(s) to the architecture-specific preboot string.c
+ * or misc.c file, for example arch/x86/boot/compressed/string.c for the x86
+ * architecture.
+ *
+ * memcmp is not used much and any remotely sane implementation is fast enough.
+ * memmove speed matters in multi-call mode. Any missing memory helper
+ * functions, including a decent implementation of an in-place memmove function
+ * could be found in lib/string.c
*
- * memeq and memzero are not used much and any remotely sane implementation
- * is fast enough. memcpy/memmove speed matters in multi-call mode, but
- * the kernel image is decompressed in single-call mode, in which only
- * memcpy speed can matter and only if there is a lot of uncompressible data
- * (LZMA2 stores uncompressible chunks in uncompressed form). Thus, the
- * functions below should just be kept small; it's probably not worth
- * optimizing for speed.
- */
-
-#ifndef memeq
-static bool memeq(const void *a, const void *b, size_t size)
-{
- const uint8_t *x = a;
- const uint8_t *y = b;
- size_t i;
-
- for (i = 0; i < size; ++i)
- if (x[i] != y[i])
- return false;
-
- return true;
-}
-#endif
-
-#ifndef memzero
-static void memzero(void *buf, size_t size)
-{
- uint8_t *b = buf;
- uint8_t *e = b + size;
-
- while (b != e)
- *b++ = '\0';
-}
-#endif
-
-#ifndef memmove
-/* Not static to avoid a conflict with the prototype in the Linux headers. */
-void *memmove(void *dest, const void *src, size_t size)
-{
- uint8_t *d = dest;
- const uint8_t *s = src;
- size_t i;
-
- if (d < s) {
- for (i = 0; i < size; ++i)
- d[i] = s[i];
- } else if (d > s) {
- i = size;
- while (i-- > 0)
- d[i] = s[i];
- }
-
- return dest;
-}
-#endif
-
-/*
- * Since we need memmove anyway, would use it as memcpy too.
- * Commented out for now to avoid breaking things.
*/
-/*
-#ifndef memcpy
-# define memcpy memmove
-#endif
-*/
#include "xz/xz_crc32.c"
#include "xz/xz_dec_stream.c"
diff --git a/lib/xz/xz_private.h b/lib/xz/xz_private.h
index 482b90f..a997dca 100644
--- a/lib/xz/xz_private.h
+++ b/lib/xz/xz_private.h
@@ -37,9 +37,12 @@
# ifdef CONFIG_XZ_DEC_SPARC
# define XZ_DEC_SPARC
# endif
-# define memeq(a, b, size) (memcmp(a, b, size) == 0)
-# define memzero(buf, size) memset(buf, 0, size)
# endif
+ /* Make all environments, including preboot, use memcmp for memeq */
+# define memeq(a, b, size) (memcmp(a, b, size) == 0)
+ /* To suppress redefine warning in some architecture's preboot */
+# undef memzero
+# define memzero(buf, size) memset(buf, 0, size)
# define get_le32(p) le32_to_cpup((const uint32_t *)(p))
#else
/*
--
1.7.1
On Thu, 2012-05-24 at 10:03 -0600, T Makphaibulchoke wrote:
> The patch cleans up the file lib/decompress_unxz.c by removing all memory
> helper functions, e.g., memmove. By doing so, any architecture's preboot
> environment supporting the XZ decompression needs to define its own copy of
> any of the missing memory helper functions.
Perhaps a silly question, but why not use
the __builtin variants?
On 05/24/2012 10:26 AM, Joe Perches wrote:
> On Thu, 2012-05-24 at 10:03 -0600, T Makphaibulchoke wrote:
>> The patch cleans up the file lib/decompress_unxz.c by removing all memory
>> helper functions, e.g., memmove. By doing so, any architecture's preboot
>> environment supporting the XZ decompression needs to define its own copy of
>> any of the missing memory helper functions.
>
> Perhaps a silly question, but why not use
> the __builtin variants?
For a lot of cases the __builtin variants just generate a call to the
expected out-of-line function, so you need it anyway.
-hpa
--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.
On Thu, 2012-05-24 at 10:29 -0700, H. Peter Anvin wrote:
> On 05/24/2012 10:26 AM, Joe Perches wrote:
> > On Thu, 2012-05-24 at 10:03 -0600, T Makphaibulchoke wrote:
> >> The patch cleans up the file lib/decompress_unxz.c by removing all memory
> >> helper functions, e.g., memmove. By doing so, any architecture's preboot
> >> environment supporting the XZ decompression needs to define its own copy of
> >> any of the missing memory helper functions.
> >
> > Perhaps a silly question, but why not use
> > the __builtin variants?
>
> For a lot of cases the __builtin variants just generate a call to the
> expected out-of-line function, so you need it anyway.
While I'm not completely knowledgeable about gcc,
aren't all the __builtin_mem<foo> functions always
available in gcc 3+
http://gcc.gnu.org/onlinedocs/gcc-3.0.4/gcc_5.html#SEC114
It does say:
Many of these functions are only optimized in certain cases; if not
optimized in a particular case, a call to the library function will be
emitted.
On 05/24/2012 10:40 AM, Joe Perches wrote:
>
> Many of these functions are only optimized in certain cases; if not
> optimized in a particular case, a call to the library function will be
> emitted.
>
Right, what part of that don't you understand?
-hpa
On Thu, 2012-05-24 at 10:41 -0700, H. Peter Anvin wrote:
> On 05/24/2012 10:40 AM, Joe Perches wrote:
> > Many of these functions are only optimized in certain cases; if not
> > optimized in a particular case, a call to the library function will be
> > emitted.
> Right, what part of that don't you understand?
:)
Whether or not the many applies to __builtin_mem<foo>.
I believe not.
cheers,
On 05/24/2012 10:50 AM, Joe Perches wrote:
> On Thu, 2012-05-24 at 10:41 -0700, H. Peter Anvin wrote:
>> On 05/24/2012 10:40 AM, Joe Perches wrote:
>>> Many of these functions are only optimized in certain cases; if not
>>> optimized in a particular case, a call to the library function will be
>>> emitted.
>> Right, what part of that don't you understand?
>
> :)
>
> Whether or not the many applies to __builtin_mem<foo>.
> I believe not.
>
There are definitely cases where gcc can call these out of line. I
already told you that.
-hpa
On 2012-05-24 T Makphaibulchoke wrote:
> The patch cleans up the file lib/decompress_unxz.c by removing all
> memory helper functions, e.g., memmove. By doing so, any
> architecture's preboot environment supporting the XZ decompression
> needs to define its own copy of any of the missing memory helper
> functions.
Is it best to copy these functions to each arch, or would it be better
to have a shared file from which these functions could be pulled for
multiple archs? I wasn't sure when I wrote decompress_unxz.c, which is
why I put the extra functions there as a temporary solution.
> Adding both the missing memmove and memcmp functions, required by the
> XZ decompressor, to the sh preboot environment.
>
> Adding the missing memmove function, required by XZ decompressor, to
> the x86 preboot environment.
These already have memcpy. It can save a few bytes if one reused
memmove as memcpy when using XZ compression. I got a difference of 48
bytes on x86_64.
Adding memmove to string.c on x86 means that memmove is included in the
kernel image even when memmove isn't needed. With gzip compression I
got 128 bytes bigger image on x86_64 after adding the unneeded memmove
to string.c.
I don't know if those size increases matter in practice.
> + * To support XZ-decompressed file in preboot environment, the
s/XZ-decompressed/XZ-compressed/ :-)
--
Lasse Collin | IRC: Larhzu @ IRCnet & Freenode
On 05/25/2012 02:34 AM, Lasse Collin wrote:
> On 2012-05-24 T Makphaibulchoke wrote:
>> The patch cleans up the file lib/decompress_unxz.c by removing all
>> memory helper functions, e.g., memmove. By doing so, any
>> architecture's preboot environment supporting the XZ decompression
>> needs to define its own copy of any of the missing memory helper
>> functions.
>
> Is it best to copy these functions to each arch, or would it be better
> to have a shared file from which these functions could be pulled for
> multiple archs? I wasn't sure when I wrote decompress_unxz.c, which is
> why I put the extra functions there as a temporary solution.
>
Yes, I agree that having a shared file would be the best solution.
Unfortunately with the current preboot architecture and infra structure,
it would not be a trivial task. I believe defining these functions for
each arch would give a better code modularity compared to including them
in the decompressor file.
>> Adding both the missing memmove and memcmp functions, required by the
>> XZ decompressor, to the sh preboot environment.
>>
>> Adding the missing memmove function, required by XZ decompressor, to
>> the x86 preboot environment.
>
> These already have memcpy. It can save a few bytes if one reused
> memmove as memcpy when using XZ compression. I got a difference of 48
> bytes on x86_64.
>
We could do that. But according to the comment in the original
implementation, there seems to be a concern regarding its performance
speed. If you believe all archs' memcpy would give comparable
performance to the memmove. then I could do that.
> Adding memmove to string.c on x86 means that memmove is included in the
> kernel image even when memmove isn't needed. With gzip compression I
> got 128 bytes bigger image on x86_64 after adding the unneeded memmove
> to string.c.
>
> I don't know if those size increases matter in practice.
>
>> + * To support XZ-decompressed file in preboot environment, the
>
> s/XZ-decompressed/XZ-compressed/ :-)
>
For x86, I think I could move memmove to the misc.c file and put an
ifdef XZ_PREBOOT around it. This way it should not impact other
decompressor. I could also do this for s390 and sh. But if we decide
to replace memmove with memcpy this would be necessart.
Thank you very for your comment. Please let me know what you think.
I'll redo and send out a new patch. Thanks in advance.
Thanks,
Mak.
> We could do that. But according to the comment in the original
> implementation, there seems to be a concern regarding its performance
> speed. If you believe all archs' memcpy would give comparable
> performance to the memmove. then I could do that.
>
Also how about the case for overlapping buffer, especially when the
destination is at a higher address? I do not believe memcpy guarantee
to work.
Thanks,
Mak.
On 05/25/2012 10:05 AM, Thavatchai Makphaibulcboke wrote:
>> We could do that. But according to the comment in the original
>> implementation, there seems to be a concern regarding its performance
>> speed. If you believe all archs' memcpy would give comparable
>> performance to the memmove. then I could do that.
>>
>
> Also how about the case for overlapping buffer, especially when the
> destination is at a higher address? I do not believe memcpy guarantee
> to work.
memcpy() is not guaranteed to work for any overlap; that is what
memmove() is for.
-hpa
--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.
On 2012-05-25 Thavatchai Makphaibulcboke wrote:
> Yes, I agree that having a shared file would be the best solution.
> Unfortunately with the current preboot architecture and infra
> structure, it would not be a trivial task.
I agree.
> I believe defining these functions for each arch would give a better
> code modularity compared to including them in the decompressor file.
I guess so. decompress_unxz.c certainly isn't the right place for those
functions. The arch-specific files also allow arch-specific
optimizations if someone finds them useful.
> > These already have memcpy. It can save a few bytes if one reused
> > memmove as memcpy when using XZ compression. I got a difference of
> > 48 bytes on x86_64.
>
> We could do that. But according to the comment in the original
> implementation, there seems to be a concern regarding its performance
> speed. If you believe all archs' memcpy would give comparable
> performance to the memmove. then I could do that.
In a generic case, you can replace memcpy with memmove but not vice
versa. memmove is generally slower than memcpy because memmove has to
support overlapping buffers.
I guess the current comment in decompress_unxz.c could be clearer. In
short, memmove and memcpy speeds don't matter much *for kernel
decompression* which is done in the single-call mode of the XZ
decompressor on archs that currently support XZ. memcpy speed could
matter if the kernel image contained a large amount of incompressible
data, but even if it did, it shouldn't matter much.
> > Adding memmove to string.c on x86 means that memmove is included in
> > the kernel image even when memmove isn't needed. With gzip
> > compression I got 128 bytes bigger image on x86_64 after adding the
> > unneeded memmove to string.c.
> >
> > I don't know if those size increases matter in practice.
>
> For x86, I think I could move memmove to the misc.c file and put an
> ifdef XZ_PREBOOT around it. This way it should not impact other
> decompressor. I could also do this for s390 and sh. But if we decide
> to replace memmove with memcpy this would be necessart.
Or you can use #ifdef CONFIG_KERNEL_XZ in string.c. XZ_PREBOOT is kind
of internal define to the XZ code so it's not good to rely on it.
If you add a static memmove function to misc.c, compiler can omit it
when memmove isn't used. Since misc.h pulls <linux/string.h> via
<linux/elf.h> and so there's a prototype of extern memmove already, one
needs to call the static function e.g. my_memmove and #define memmove
my_memmove. This way you don't need to #ifdef it to any particular
decompressors.
If memmove is used to implement memcpy, you probably cannot avoid
decompressor-specific #ifdefs unless you use memmove for all
decompressors. The question is how clean code you want vs. how much you
care about saving 30-150 bytes in bzImage size.
--
Lasse Collin | IRC: Larhzu @ IRCnet & Freenode
On 10:03 Thu 24 May , T Makphaibulchoke wrote:
> The patch cleans up the file lib/decompress_unxz.c by removing all memory
> helper functions, e.g., memmove. By doing so, any architecture's preboot
> environment supporting the XZ decompression needs to define its own copy of
> any of the missing memory helper functions.
>
> The patch makes sure that all 4 architectures' preboot currently supporting
> the XZ decompressor, arm, s390, sh and x86, build without error when using
> the XZ compression.
>
> Adding a prototype for the memcmp function required by the XZ decompressor to
> workaround compiler's implicit type error. Also removing both the memmove and
> memcpy defines workaround, to disable lib/decompress_unxz.c from duplicating
> both functions.
>
> Adding the missing memcmp function, required by the XZ decompressor, to the
> s390 preboot environment.
>
> Adding both the missing memmove and memcmp functions, required by the XZ
> decompressor, to the sh preboot environment.
>
> Adding the missing memmove function, required by XZ decompressor, to the x86
> preboot environment.
>
> Signed-off-by: T. Makphaibulchoke <[email protected]>
> ---
> arch/arm/boot/compressed/decompress.c | 3 +-
> arch/s390/boot/compressed/misc.c | 14 +++++
> arch/sh/boot/compressed/misc.c | 32 ++++++++++++
> arch/x86/boot/compressed/string.c | 18 +++++++a
can we do not duplicate those functions?
Best Regards,
J.
On 05/28/2012 01:03 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> can we do not duplicate those functions?
>
> Best Regards,
> J.
Thanks J for the comment. Unfortunately, there is no easy way to share
files among different architectures' preboot environment. We have the
choices either to have each architecture define its own copies or the
decompressor defines them. I believe the former is preferable. As
Lasse also pointed out, this way any specific architecture could provide
an architectural dependent optimized version, if it chooses to.
Thanks,
Mak.
On Thu, 2012-05-31 at 20:48 -0600, Thavatchai Makphaibulcboke wrote:
> On 05/28/2012 01:03 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>
> > can we do not duplicate those functions?
>
> Thanks J for the comment. Unfortunately, there is no easy way to share
> files among different architectures' preboot environment. We have the
> choices either to have each architecture define its own copies or the
> decompressor defines them. I believe the former is preferable. As
> Lasse also pointed out, this way any specific architecture could provide
> an architectural dependent optimized version, if it chooses to.
Maybe provide __weak default functions that
can be overridden by any arch that could
or wants to improve on the defaults.
On 05/31/2012 07:48 PM, Thavatchai Makphaibulcboke wrote:
> On 05/28/2012 01:03 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>
>> can we do not duplicate those functions?
>>
>> Best Regards,
>> J.
>
> Thanks J for the comment. Unfortunately, there is no easy way to share
> files among different architectures' preboot environment. We have the
> choices either to have each architecture define its own copies or the
> decompressor defines them. I believe the former is preferable. As
> Lasse also pointed out, this way any specific architecture could provide
> an architectural dependent optimized version, if it chooses to.
>
We could always make a source code library, like we already in effect
have for the compression functions themselves.
-hpa
On 05/31/2012 09:38 PM, H. Peter Anvin wrote:
> We could always make a source code library, like we already in effect
> have for the compression functions themselves.
>
> -hpa
>
Thanks for the suggestion and comment. I guess we could add a new
directory with a new file under arch, such as arch/lib/mem.c and use the
source include similar to the compression.
Thanks,
Mak.
On 06/01/2012 11:37 AM, Thavatchai Makphaibulcboke wrote:
> On 05/31/2012 09:38 PM, H. Peter Anvin wrote:
>> We could always make a source code library, like we already in effect
>> have for the compression functions themselves.
>>
>> -hpa
>>
>
> Thanks for the suggestion and comment. I guess we could add a new
> directory with a new file under arch, such as arch/lib/mem.c and use the
> source include similar to the compression.
>
Well, the decompression stuff is under lib/ ... maybe lib/boot/ or
something like that...
-hpa
--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.
On 06/01/2012 12:46 PM, H. Peter Anvin wrote:
>
> Well, the decompression stuff is under lib/ ... maybe lib/boot/ or
> something like that...
>
> -hpa
>
>
Thanks again for the comment. I thought of putting it under arch to
separate it from the main line kernel to avoid any confusion. But if
you think it is OK to put under lib/boot, that sounds good to me.
Thanks,
Mak.