Page MenuHomeFreeBSD

ossl: Add support for powerpc64/powerpc64le
ClosedPublic

Authored by sanastasio_raptorengineering.com on Sep 12 2023, 8:17 PM.
Tags
None
Referenced Files
F106189881: D41837.diff
Thu, Dec 26, 9:16 PM
Unknown Object (File)
Mon, Dec 9, 6:59 AM
Unknown Object (File)
Sun, Dec 1, 9:25 AM
Unknown Object (File)
Nov 2 2024, 6:41 AM
Unknown Object (File)
Nov 2 2024, 6:41 AM
Unknown Object (File)
Nov 2 2024, 6:41 AM
Unknown Object (File)
Oct 7 2024, 2:30 PM
Unknown Object (File)
Oct 7 2024, 2:30 PM

Details

Summary

Add support for building ossl(4) on powerpc64* by implementing ossl_cpuid and
other support functions for powerpc. The required assembly files for ppc were
already present in-tree.

Test Plan

The changes were tested using the in-tree tools/tools/crypto/cryptocheck.c tool on both powerpc64 and powerpc64le on a POWER9 system.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

lwhsu added inline comments.
sys/crypto/openssl/ossl_ppc.c
8

Are you copying/modifying a file that already has a Foundation's copyright? If no, you can just have your own copyright header.

sys/crypto/openssl/ossl_ppc.c
8

No, this file (other than the trivial wrapper functions from OpenSSL noted below) was written from scratch. Would it be acceptable to just replace the "Copyright (c) 2023 The FreeBSD Foundation)" text with my own copyright and retain the rest of the comment/license text?

sys/crypto/openssl/ossl_ppc.c
8

Yes, and that's how it supposes to be done. :-) As you are the original author, you should own the copyright, and release it under the license the project accepts.

The current form is when you're doing this work under the sponsorship of the Foundation, then the copyright owner would be the Foundation, and note that you write it in the comment.

31

The new code doesn't need $FreeBSD$ anymore.

sys/crypto/openssl/ossl_ppc.c
8

Yes, and that's how it supposes to be done. :-) As you are the original author, you should own the copyright, and release it under the license the project accepts.

The current form is when you're doing this work under the sponsorship of the Foundation, then the copyright owner would be the Foundation, and note that you write it in the comment.

Thanks for clarifying that -- I'll correct this.

31

Got it, will drop.

  • Correct license header on newly-added ossl_ppc.c

Hi all,

Just a quick ping to see if anybody has the time to review this patch -- it would be greatly appreciated.

sys/crypto/openssl/ossl_ppc.c
57

Are you sure it supports MFTB? I thought the mftb instruction was deprecated long ago.

PowerISA 3.1 reference states on page 1094 "New programs should use mfspr instead of mftb to access the Time Base."

Or does this bit do something else?

sys/crypto/openssl/ossl_ppc.c
57

Good point. I originally referenced the logic in openssl-1.1.1v's ppccap.c which seems to prefer MFTB whenever it is available (i.e. it doesn't cause a SIGILL during the feature probe) as well as FreeBSD's mftb() function[1] which always uses the mftb instruction on ppc64 and mfspr on ppc32.

On closer inspection though, I don't think any of the code present in the ossl driver here actually checks or uses this flag in any way. If you'd like me to drop the detection and setting of this flag it'd be safe to do so -- care would just need to be taken if any code that depended on it was imported from upstream openssl.

[1] https://github.com/freebsd/freebsd-src/blob/main/sys/powerpc/include/cpufunc.h#L137

Sorry to ping again, but now that the fix for the kernel_fpu facilities has been merged could I request further review of this patch?

Do you need to update sys/modules/Makefile to enable ossl on powepc64*?

sys/crypto/openssl/ossl_aes.c
42

Hmm, arch(7) lists __powerpc__ as the normal macro to use rather than __PPC__, also presumably this should be __powerpc64__ since this review doesn't include 32-bit PPC support?

In D41837#978749, @jhb wrote:

Do you need to update sys/modules/Makefile to enable ossl on powepc64*?

Strangely enough I did not need to change sys/modules/Makefile to get the module built and running on powerpc64{,le}, though it seems like for completeness' sake this ought to be done anyways. Will do.

sys/crypto/openssl/ossl_aes.c
42

Good observation, will update to powerpc64.

  • Use powerpc64 compiler conditional
  • Update sys/modules/Makefile
  • Rebase
jhb added inline comments.
sys/powerpc/conf/GENERIC64
202

Should you add ossl to GENERIC64LE as well?

This revision is now accepted and ready to land.Mar 4 2024, 11:01 PM
This revision now requires review to proceed.Mar 5 2024, 7:04 PM
sys/powerpc/conf/GENERIC64
202

Good catch, this was an oversight in my most recent version of the diff. Fixed.

This revision is now accepted and ready to land.Mar 7 2024, 2:33 AM

Hi all,

As far as I can tell, this change still hasn't been committed despite receiving approval. If someone with the appropriate privileges could do so I'd greatly appreciate it.

Thanks,
Shawn

Hi all,

As far as I can tell, this change still hasn't been committed despite receiving approval. If someone with the appropriate privileges could do so I'd greatly appreciate it.

Thanks,
Shawn

Sorry, this slipped through the cracks. Will push it shortly.