Fix alignment issues on MIPS

[br]

Summary

MIPS required the pointers to be aligned

Test Plan

5520 GEOM_ELI tests passed successfully

[emaste]

I think we should commit this, with appropriate #ifdef guards. Maybe __mips_n64?

[imp]

sys/geom/eli/g_eli.h

251 ↗

(On Diff #20385)

This scares me. Seems like you'd be changing the size of the metadata on the disk if you did this.

sys/geom/eli/g_eli_integrity.c

448 ↗

(On Diff #20385)

sizeof(uintptr_t) maybe?

456 ↗

(On Diff #20385)

what's wrong with the roundup macro?

[emaste]

sys/geom/eli/g_eli.h

251 ↗

(On Diff #20385)

This is why I said above in #ifdef guards, and wondering about an appropriate macro to use. Because of this geli does not work today on mips64, so we can either end up with a new on-disk geli format for mips64 or have geli explicitly not supported on mips64.

[imp]

sys/geom/eli/g_eli.h

251 ↗

(On Diff #20385)

So md5 requires that we hash into an aligned buffer?

Do we have the same issue with ARM?

[br]

add mips64 ifdefs

[br]

In D7905#172795, @emaste wrote:

I think we should commit this, with appropriate #ifdef guards. Maybe __mips_n64?

probably __mips_n64 || __mips_o64

[br]

include Makefile change to allow module build

[emaste]

Longer term, I think we should see if we can bump G_ELI_VERSION and apply this change on all architectures, in addition to switching away from md5.

[br]

sys/geom/eli/g_eli.h

251 ↗

(On Diff #20385)

Yes md5 requires aligned md_hash. Not sure about ARM

[emaste]

sys/geom/eli/g_eli.h

251 ↗

(On Diff #20385)

Alternately we could memcpy the md_hash in.

[br]

yes, you right.
it looks like per-byte copy works. I'll try run all the tests

[emaste]

sys/geom/eli/g_eli.h

359 ↗

(On Diff #21590)

FYI I would expect that alignment of at least 4 is mandated by all of our ABIs.

[emaste]

sbin/geom/class/eli/geom_eli.c

670 ↗

(On Diff #21590)

I'm slightly surprised we'd end up with a misaligned sector[], even if it's not explicitly guaranteed. However, if we do need to explicitly specify alignment there should be no problem (for this stack variable) doing it for all architectures.

[imp]

sbin/geom/class/eli/geom_eli.c

670 ↗

(On Diff #21590)

We should do it for all architectures. There's no harm, and it may help.

sys/geom/eli/g_eli.h

359 ↗

(On Diff #21590)

Why not make this an array of uint32_t?

sys/geom/eli/g_eli_integrity.c

448 ↗

(On Diff #21590)

We can do this for all archs. There's no harm.

456 ↗

(On Diff #20385)

Ditto.

But honestly, the kernel malloc should returned a properly aligned buffer. Have you verified that it actually doesn't?

[emaste]

sbin/geom/class/eli/geom_eli.c

670 ↗

(On Diff #21590)

Probably something like __aligned(__alignof(struct g_eli_metadata))

[br]

update

[br]

sbin/geom/class/eli/geom_eli.c

670 ↗

(On Diff #21590)

__alignof(struct g_eli_metadata) is 1, but we need 4

[br]

sys/geom/eli/g_eli_integrity.c

456 ↗

(On Diff #20385)

p is adjusted pointer:

if (bp->bio_cmd == BIO_READ) {
    data = bp->bio_driver2;
    p = auth + sc->sc_alen * nsec;
} else {
    data = malloc(size, M_ELI, M_WAITOK);
    p = data + encr_secsize * nsec;
}
p = (char *)roundup((uintptr_t)p, sizeof(uintptr_t));

Sometimes p comes not aligned after this.