Re: [OpenBIOS] [PATCH 2/2] add Adler-32 checksum capability

[Mark Cave-Ayland]

On 09/04/15 02:35, Cormac O'Brien wrote:
> From: Cormac O'Brien
>
> This patch adds Adler-32 checksum functionality to the Forth interpreter as
> required by the Mac OS 9 boot script. This code might belong somewhere else,
> do let me know.
>
> libopenbios/bootinfo_load.c | 49 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
>
> diff --git a/libopenbios/bootinfo_load.c b/libopenbios/bootinfo_load.c
> index fcb23ea..d0ccd89 100644
> --- a/libopenbios/bootinfo_load.c
> +++ b/libopenbios/bootinfo_load.c
> @@ -30,6 +30,54 @@
>      do { } while (0)
>  #endif
>
> +/* adler32 -- Adapted from Mark Adler's zlib implementation */

Can we add a stack diagram?
> +#define DO1(buf,i)  {s1 += buf[i]; s2 += s1;}
> +#define DO2(buf,i)  DO1(buf,i); DO1(buf,i+1);
> +#define DO4(buf,i)  DO2(buf,i); DO2(buf,i+2);
> +#define DO8(buf,i)  DO4(buf,i); DO4(buf,i+4);
> +#define DO16(buf)   DO8(buf,0); DO8(buf,8);
>
> +void
> +adler32( void ) {
> unsigned int len = POP();
> char *buf = POP();
> unsigned long adler = POP();
> if (buf == NULL) {
>    RET(-1);
> }
> long base = 65521L;
> int nmax = 5552;
> unsigned long s1 = adler & 0xffff;
> unsigned long s2 = (adler >> 16) & 0xffff;
> int k;
> while (len > 0) {
>    k = (len < nmax ? len : nmax);
>    len -= k;
>    while (k >= 16) {
>        DO16(buf);
>        buf += 16;
>        k -= 16;
>    }
>    if (k != 0) {
>        do {
>            s1 += *buf++;
>            s2 += s1;
>        } while (--k);
>    }
>    s1 %= base;
>    s2 %= base;
> }
> RET(s2 << 16 | s1);
>
> +}
>
> static char *
>  get_device( const char *path )
>  {
> @@ -255,6 +303,7 @@ bootinfo_init_program(void)
>    /* If the payload is bootinfo then we execute it immediately */
>    if (scriptvalid) {
>    	DPRINTF("bootscript: %s\n", bootscript);
> bind_func("adler32", adler32);
> feval(bootscript);
> }
> else

A couple of minor points here: firstly, can we switch to using the
standard uint32_t/uint64_t instead of long? The reason for this is that
if we decided to later move the function to the core then the sizes of
int/long will change between 32-bit and 64-bit which may cause some
unpredictable results.

Secondly the function should be moved somewhere outside of
bootinfo_load.c - I'd go for arch/ppc/qemu/init.c, similar to ffilll()
which was another Mac-only OpenFirmware word.

ATB,

Mark.