The order is SPDX then owner from style(9).

Can you move the macros before ENTRY? It makes it easier to find where the start of the function is if you don't have to skip over them.

Why are these commented out?

I have placed the macros after ENTRY as they “declare” local variables of this function. I can move the ENTRY macro around, but I believe it is cleared to have the declarations of local variables after the ENTRY macro, by analogy to how C functions are laid out.

They have been outlined and specialised in the two preceding blocks. I placed the commented out code there to make the correspondence clear; I should perhaps add a comment to make this clearer.

Could hide this down in lib/libc/md/Makefile.inc1 and avoid changing this line?

Why don't these other conditions influence the default of USE_ASM_SOURCES instead? Then that variable isn't a lie.

Can we put these in a directory instead like the other hash functions in lib/libmd/Makefile?

Why aren't these in files.arch? We don't use this pattern in sys/conf, flags come from config(8).

I doubt you want these in libsa, and have you verified they're actually important for performance?

This block of headers isn't style(9)-conformant; they need reordering and separating out into multiple blocks

It doesn't really make sense to keep a whole subdirectory and extra include Makefile around for this. The lib/libc/md directory is entirely replaced by this new file.

This file follows the sys/crypto conventions, not the lib/libmd conventions, so it's different from the others. It doesn't make sense to add extra subdirectories to make it follow neither convention properly.

I have tried to solve this with changing the config file, but it didn't work. I do welcome advice for how to do better.

I have. Otherwise the compiler does not unroll these loops, which prevents the compiler from properly scheduling the function. The old implementation avoided using a loop, instead opting for a macro, to cause a similar effect.

I've never done this before myself, and this is not tested - but could it look something like the following, in sys/conf/files.amd64:

md5c.c
	compile-with "${NORMAL_C} -DMD5_ASM"

And:

md5block_amd64.S       standard

And then performing this for each architecture where an assembly version is available?

Yes. This is the path. Funky flags like this are generally forbidden i. Makefile.foo.

If it were more than one file, we define a new macro based on NORMAL_C with the added flags.

So you tested with one compiler. Meh. The danger of over optimizing stand is that ot will cause integrity issues with some compuler, somewhere in the future, possibly leading to unbootable systems. Macros with the same effect are safer.

Have you verified the older, safer versions are too slow to the booting process? I think that was jrtc27's question.

Thanks, I will investigate this route.

It is unfortunate that it has taken over a year for someone to tell me the correct approach; this is not documented anywhere and nobody felt like helping me until I contacted Khorben directly.

I'm not sure what you mean by “safer.” None of this will be too slow for the boot process. I am submitting this change set because I regularly take md5 checksums of large files to check if they transferred correctly, so I do care about it being as fast as possible.

I can rewrite the code to use macros to force unrolling as before, but that'll achieve the same thing with code that is harder to read overall. I don't see any safety issue either way; the unroll pragma does not change the semantics of the code, it merely forces the compiler to unroll the loop.

Have you verified the older, safer versions are too slow to the booting process? I think that was jrtc27's question.

The old macro based code had the same effect (including on code size) as these unroll pragmas. I can remove them for slower, but smaller code than what we had before if that helps you.

I am not sure why we are having this conversation really; do you not see the value in getting rid of piles of macros and manually unrolled loops in favour of easy to understand loops with instructions to the compiler to unroll them?

You should have asked. This was burried deep in a complex review. It's a 5-10 minute exchange on irc. Whole the lack of docs is a fair jab, lots of examples are there and many others have done it.

I believe I have asked multiple times on IRC, and I think I have even tagged you personally. I did not get a response.

I don't recall seeing that, but in the fiture a quick note to arch@ would likely be a goid qay to escalate

So trying that, I came to the same issue I think I had last time I tried it. To make this work, we have in sys/conf/files:

crypto/md5/md5c.c               standard

and in sys/conf/files.amd64

crypto/md5/md5c.c               standard compile-with "${NORMAL_C} -DMD5_ASM"

but turns out the later setting doesn't overwrite the former! The generated Makefile just compiles with ${NORMAL_C}. If I remove the entry from sys/conf/files, the entry from sys/conf/files.amd64 is applied, but that defeats the purpose of overwriting the generic configuration with a platform-specific configuration.

What do I need to do to work around this issue?

PS: tried to reach you on IRC right now, but got no response.

Yes. You can't do that for any other files.. There's no 'override' concept like this in config(8).
Either you need to put it in each files.X with the special recipe (which is fragile) or you need to take it to the next level by putting

crypto/md5/md5c.c               standard compile-with "${MD5_C}"

in sys/conf/files and then add MD5_C to kern.post.mk

MD5_CFLAGS=
MD5_C=${NORMAL_C} ${MD5_CFLAGS}

and then in sys/conf/Makefile.amd64 have MD5_CFLAGS=-DMD5_ASM

But all of that hoop jumping suggests that a simpler:

#ifdef __amd64__
#define MD5_ASM
#endif

somewhere near the top of md5c.c would be the right way to encode this dependency.

We've had numerous cases where, due to the weird compile flags we need for other bits of the boot loader that something has broken when highly optimized code like this that does clever tricks is deployed. This were all due to compiler bugs, but we've had to do a lot of tweaks, especially on compiler upgrades, when we've used code like this in the past. It all came to a head a few years ago when ZFS had an arm-optimized routine for one of the non-optional crypto routines without havning a normal, C fallback. Due to the different ABIs / calling conventions, this broke EFI booting (even though we could compile) on arm64 for a time because registers were used inappropriately. It's eaten up enough of my time in the past that I'm super nervous about committing to possibly having code that might cost me more time in the future.

I'm OK with letting this in if you're OK coping with any weird breakage in the diversity of environments that we deploy the bootloader to. that might be caused by this.

So logically, all of these (and similar below) are to select the right, optimal set of arm instructions for the magnitude of this compile time constant, right? It might make sense to say so since that took some puzzling for me to arrive there. The other full macros have this sort of thing and it seems like it would be a good compromise to say that... or if I've puzzled it out wrong, maybe the right thing.

The whole k vs \k thing too is a bit confusing. Is there some logic to having the parameter named k while there's a different 'k' for the rest of the code, or is it a macro testing thing? It looks like a naked 'k' is register w9...

It might make sense to move these // lines up to above the .Lloop: label.

So these keys get repeated a lot... looks like maybe 5 times in various formats... Consider whether or not you can have just one copy (or maybe 2 since you unroll the first two round0 in the arm code.

Yes, correct. I'll add a comment and change the naming to be less ambiguous.