qemu-trivial
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] mark <zlib.h> with for-crc32 in a consistent manner


From: Mark Cave-Ayland
Subject: Re: [PATCH] mark <zlib.h> with for-crc32 in a consistent manner
Date: Wed, 28 Aug 2024 09:43:20 +0100
User-agent: Mozilla Thunderbird

On 27/08/2024 18:51, Michael Tokarev wrote:

27.08.2024 15:09, Mark Cave-Ayland wrote:
On 27/08/2024 11:02, Michael Tokarev wrote:

in many cases, <zlib.h> is only included for crc32 function,
and in some of them, there's a comment saying that, but in
a different way.  In one place (hw/net/rtl8139.c), there was
another #include added between the comment and <zlib.h> include.

Make all such comments to be on the same line as #include, make
it consistent, and also add a few missing comments, including
hw/nvram/mac_nvram.c which uses adler32 instead.
...
  //#define DEBUG_STELLARIS_ENET 1

For the hw/net devices there are separate net_crc32() and net_crc32_le() functions from net/net.c which are intended for (most) network devices where the "standard" polynomials are used.

In many hw/net files I touched in this patch, *both*
plain crc32() and qemu's net_crc32() are used.

For now I just marked the #include, nothing more, we
can finish the refactoring later if needs to be.

Speaking of crc32 from zlib, I don't really see a point
in re-implementing it in this context (it is re-implemented
in net/net.c:net_crc32(), with comment "XXX: optimize*).
Implementation from zlib is quite a good one. Not the best
possible but definitely not the worst and is better than
net_crc32().

(See also https://create.stephan-brumme.com/crc32/)

Right, I was just wondering that since you were already changing the zlib.h includes if you were interested to update the net devices and switch net_crc32() to use the zlib implementation whilst you were working in that area ;)

What we definitely *can* optimize is the two cases in
tcg (arm and loong iirc) - they have hardware isns for
crc32, but these operate in fixed 4 or 8-bytes integers,
and there, implementing a function in qemu would be
nice - not much code but significant speedup due to
fixed size of the argument.

I don't see any isse with using crc32 from zlib, since
zlib is used for other things anyway and is mandatory
dependency.  In case of qemu-user binaries, even static
link, it is tiny (since only crc32 stuff is linked to),
but there it would be more interesting to have in-qemu
implementation for static-size isns.

Agreed, I can certainly see the need for an implementation in the QEMU core. In which case the net_crc32_*() functions will then be simple wrappers onto those implementations...


ATB,

Mark.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]