NVIDIA: Adding cuPQC as a backend for ML-KEM.

[stevenireeves]

This PR adds the support for the NVIDIA library cuPQC to be used as the backend for ML-KEM algorithms.

cuPQC requires the use of an NVIDIA GPU to perform the PQC algorithms.

• Does this PR change the input/output behaviour of a cryptographic algorithm (i.e., does it change known answer test values)? (If so, a version bump will be required from x.y.z to x.(y+1).0.)
• Does this PR change the list of algorithms available -- either adding, removing, or renaming? Does this PR otherwise change an API? (If so, PRs in fully supported downstream projects dependent on these, i.e., oqs-provider will also need to be ready for review and merge by the time this is merged.)

[stevenireeves]

@praveksharma @neil-lindquist for visibility.

[stevenireeves]

@praveksharma looks like a number of github actions are failing, can you help with this, or point to what needs to be changed?

[baentsch]

@stevenireeves For the code formatting errors, please review this. For the basic test error (and error, actually), please open the "twisty" in the CI error report e.g. here at the "Configure" step to see the config command executed: You can run locally and see the problem: The new config variable apparently isn't initialized in all cases:

[praveksharma]

@stevenireeves I've pushed 3 commits to https://github.com/open-quantum-safe/liboqs/tree/libOQSxcuPQC, which builds off of your branch, to get it to pass CI. I can't seem to push the changes directly to your fork.

Here's how to re-produce the changes:

1. Commit 1:

export LIBOQS_DIR=`pwd`
cd ./scripts/copy_from_upstream
python3 ./copy_from_upstream.py copy

2. Commit 2:

./tests/run_astyle.sh --no-dry-run

3. Commit 3: replace spaces with tabs in scripts/copy_from_upstream/src/kem/family/kem_scheme.c as in commit.

[praveksharma]

Thank you for making these changes @stevenireeves! Since I reviewed the private PR initially I'll let this one be reviewed by reviewers other than myself.

[dstebila]

I talked to @praveksharma just now to understand some of the approach here, and I understand why the cupqc metadata is patched into the PQ Crystals meta.yml file (since there isn't another meta.yml to patch it into); so that makes sense. But I think a few other pieces could be done differently. I don't think the *.cu files need to be added via the patch file; couldn't they just be added directly without also being added through the patch? And if so then could we give those directories a name that doesn't include pqcrystals -- e.g., just src/kem/ml_kem/cupqc-ml-kem-768?

[stevenireeves]

I talked to @praveksharma just now to understand some of the approach here, and I understand why the cupqc metadata is patched into the PQ Crystals meta.yml file (since there isn't another meta.yml to patch it into); so that makes sense. But I think a few other pieces could be done differently. I don't think the *.cu files need to be added via the patch file; couldn't they just be added directly without also being added through the patch? And if so then could we give those directories a name that doesn't include pqcrystals -- e.g., just src/kem/ml_kem/cupqc-ml-kem-768?

@dstebila with this organizational change will we need to mess with the copy_from_upstream stuff as well?

[praveksharma]

@dstebila with this organizational change will we need to mess with the copy_from_upstream stuff as well?

@stevenireeves I shall work on this and attemp to push the changes directly to your fork. I don't believe copy_from_upstream.py would need to be changed significantly.

[stevenireeves]

@dstebila with this organizational change will we need to mess with the copy_from_upstream stuff as well?

@stevenireeves I shall work on this and attemp to push the changes directly to your fork. I don't believe copy_from_upstream.py would need to be changed significantly.

I have the allow edits from maintainers option selected. So should be able to make those changes.

[praveksharma]

I don't think the *.cu files need to be added via the patch file; couldn't they just be added directly without also being added through the patch? And if so then could we give those directories a name that doesn't include pqcrystals -- e.g., just src/kem/ml_kem/cupqc-ml-kem-768?

@dstebila handling the naming shouldn't be an issue. The *.cu file must be sourced with every run of copy_from_upstream.py under delete mode, since there is no upstream the *.cu must be sourced from within the liboqs repo - a patch is the most straightforward way of doing this. Would a separate patch file for the *.cu file be adequate?

[baentsch]

I understand why the cupqc metadata is patched into the PQ Crystals meta.yml file (since there isn't another meta.yml to patch it into); so that makes sense

Hmm -- it doesn't to me: We have #2041 in the pipeline with the declared goal of removing the PQCrystals files. So this PR thus would need to be re-done after that landed, right? That doesn't seem sensible.

Wouldn't it be much more sensible to have this code be contained in an upstream of its own to pull it from? As far as I can see, this is effectively a different implementation with different license terms, characteristics etc. Such split also would make responsibilities clear: NVIDIA is to support the interfacing to its library (that may very well change over time unbeknownst to OQS) and OQS is responsible for the proper operation within/integration into the OQS APIs (that may change unbeknownst to NVIDIA). Or is the intention by NVIDIA to become committed, well committers and/or maintainers to OQS @stevenireeves ?

[stevenireeves]

Hmm -- it doesn't to me: We have #2041 in the pipeline with the declared goal of removing the PQCrystals files. So this PR thus would need to be re-done after that landed, right? That doesn't seem sensible.

Wouldn't it be much more sensible to have this code be contained in an upstream of its own to pull it from? As far as I can see, this is effectively a different implementation with different license terms, characteristics etc. Such split also would make responsibilities clear: NVIDIA is to support the interfacing to its library (that may very well change over time unbeknownst to OQS) and OQS is responsible for the proper operation within/integration into the OQS APIs (that may change unbeknownst to NVIDIA). Or is the intention by NVIDIA to become committed, well committers and/or maintainers to OQS @stevenireeves ?

Nvidia maintaining it's own fork of liboqs is not what we want to do. The intention of NVIDIA is to support libOQS on portions of liboqs that utilize cuPQC as it's backend. So if there are significant changes that require alteration of the source files related to cupqc in liboqs, we will help there (so long as we have the internal support). I believe that @praveksharma is in the process of removing the cupqc metadata from PQ Crystals meta.yaml, correct me if I'm wrong.

[dstebila]

I don't think the *.cu files need to be added via the patch file; couldn't they just be added directly without also being added through the patch? And if so then could we give those directories a name that doesn't include pqcrystals -- e.g., just src/kem/ml_kem/cupqc-ml-kem-768?

@dstebila handling the naming shouldn't be an issue. The *.cu file must be sourced with every run of copy_from_upstream.py under delete mode, since there is no upstream the *.cu must be sourced from within the liboqs repo - a patch is the most straightforward way of doing this. Would a separate patch file for the *.cu file be adequate?

Oh, I see now. I knew that we have some local implementations that --delete doesn't wipe out (e.g., FrodoKEM), but these algorithms aren't handled at all by copy_from_upstream. But I understand now that for algorithms that are handled by copy_from_upstream, there's no mixing of local (not deleted) and upstream (deleted) implementations.

So I see why the patch file is adding these.

[dstebila]

Wouldn't it be much more sensible to have this code be contained in an upstream of its own to pull it from? As far as I can see, this is effectively a different implementation with different license terms, characteristics etc. Such split also would make responsibilities clear: NVIDIA is to support the interfacing to its library (that may very well change over time unbeknownst to OQS) and OQS is responsible for the proper operation within/integration into the OQS APIs (that may change unbeknownst to NVIDIA). Or is the intention by NVIDIA to become committed, well committers and/or maintainers to OQS @stevenireeves ?

Nvidia maintaining it's own fork of liboqs is not what we want to do. The intention of NVIDIA is to support libOQS on portions of liboqs that utilize cuPQC as it's backend. So if there are significant changes that require alteration of the source files related to cupqc in liboqs, we will help there (so long as we have the internal support). I believe that @praveksharma is in the process of removing the cupqc metadata from PQ Crystals meta.yaml, correct me if I'm wrong.

I think these are talking about two slightly different things. Michael isn't suggesting that NVIDIA maintain a separate fork of liboqs. The way our code has been structured is that we have scripts to pull in source code from (self-contained) implementations of algorithms in other repositories, and add them to liboqs using code generation and patches.

This could be done that way, but since it is also only one file that is being added, I don't think it's worth the effort of setting up a separate upstream for that.

[baentsch]

Nvidia maintaining it's own fork of liboqs is not what we want to do.

This is not my ask: I only (meant to :) ask whether you'd want to maintain the wrapper code around the cuPQC code in a separate project -- complete with META.yml such as for copy_from_upstream to pull it in -- thus obviating the need for patching other implementations' META.yml files.

[SWilson4]

I added the --delete functionality for copy_from_upstream quite recently. I can add an exception for the cuda files so that they don't get cleaned out; it would be quite straightforward and IMO would make maintenance of those files a lot easier. Editing patch files instead of source files is a pain. How does this sound @praveksharma?

[praveksharma]

I added the --delete functionality for copy_from_upstream quite recently. I can add an exception for the cuda files so that they don't get cleaned out; it would be quite straightforward and IMO would make maintenance of those files a lot easier. Editing patch files instead of source files is a pain. How does this sound @praveksharma?

Thank you for offering @SWilson4 but I think it is more prudent to create a separate git repository to store cupqc_ml-kem metadata since the pqcrystals upstream is going to be deprecated soon in any case. If things work out okay @stevenireeves or someone else from Nvidia can ownership of the repo.

[stevenireeves]

Thank you for offering @SWilson4 but I think it is more prudent to create a separate git repository to store cupqc_ml-kem metadata since the pqcrystals upstream is going to be deprecated soon in any case. If things work out okay @stevenireeves or someone else from Nvidia can ownership of the repo.

@praveksharma I am not sure what metadata you want us to store. Do you want us to write the wrapper code in an additional repository?

[praveksharma]

I think it is more prudent to create a separate git repository to store cupqc_ml-kem metadata since the pqcrystals upstream is going to be deprecated soon in any case

I've updated the import mechanism to use https://github.com/praveksharma/cupqc-mlkem as an upstream. Ideally I would like to modify copy_from_upstream to not require ad hoc upstreams for situations such as this.

Could you please review the updated PR @SWilson4 @dstebila @baentsch ?

[stevenireeves]

@praveksharma looks like the build with OQS_USE_CUPQC is still failing in this PR. Although I can't see what is causing the failures. Did Neil's comments in the email help?

[praveksharma]

@praveksharma looks like the build with OQS_USE_CUPQC is still failing in this PR. Although I can't see what is causing the failures. Did Neil's comments in the email help?

@stevenireeves Yes, the comments were super helpful. I can succesfully build locally, this is mostly likely an issue with the CI config itself that I'm still troubleshooting.

[praveksharma]

@stevenireeves cmake in CI is failing with this message:

CMake Error at /usr/share/cmake-3.28/Modules/CMakeDetermineCUDACompiler.cmake:270 (message):
Failed to detect a default CUDA architecture.

I am unable to reproduce this error locally in a docker container provisioned from the same image used in CI. Do you know how to fix this?

[stevenireeves]

CMake Error at /usr/share/cmake-3.28/Modules/CMakeDetermineCUDACompiler.cmake:270 (message):
Failed to detect a default CUDA architecture.

I am unable to reproduce this error locally in a docker container provisioned from the same image used in CI. Do you know how to fix this?

@praveksharma ah, because CI has no GPU it can't detect a default. Try adding this flag to the CMake command.
-DCMAKE_CUDA_ARCHITECTURES=80

Specifically for CI.

[stevenireeves]

@praveksharma looks like that CI test is still failing.

No CMAKE_CUDA_COMPILER could be found.

Tell CMake where to find the compiler by setting either the environment
variable "CUDACXX" or the CMake cache entry CMAKE_CUDA_COMPILER to the full
path to the compiler, or to the compiler name if it is in the PATH.

You'll need to tell CMAKE where nvcc is.

[stevenireeves]

@praveksharma looks like that did the trick. Any other comments by the reviewers?

[SWilson4]

LGTM, with the caveat that we should eventually move away from a personal repo, as noted by @baentsch above.

[baentsch]

LGTM, with the caveat that we should eventually move away from a personal repo, as noted by @baentsch above.

Agreed: Please do this now or create an issue, ideally assigned, so this is not forgotten.

[praveksharma]

Agreed: Please do this now or create an issue, ideally assigned, so this is not forgotten.

@baentsch #2053 has been created to track this.

@stevenireeves do you want to move the upstream to your GitHub account?

[stevenireeves]

@baentsch #2053 has been created to track this.

@stevenireeves do you want to move the upstream to your GitHub account?

@praveksharma
I think it's fine where it is now especially if we are just going to refactor copy from upstream so it's irrelevant. If we can it would be great to get this merged today.

[stevenireeves]

@praveksharma @baentsch @dstebila anything else that needs to be done to get this in?

[praveksharma]

@praveksharma @baentsch @dstebila anything else that needs to be done to get this in?

I think this is good to merge, just need an additional approval.

[stevenireeves]

I think this is good to merge, just need an additional approval.

@praveksharma looks like we got what we needed