RFR: 8329538: Accelerate P256 on x86_64 using Montgomery intrinsic [v2]

[Anthony Scarpino]

On Wed, 10 Apr 2024 18:02:38 GMT, Volodymyr Paprotski <duke at openjdk.org> wrote:

> > In `ECOperations.java`, if I understand this correctly, it is to replace the existing `PointMultiplier` with montgomery-based PointMuliplier. But when I look at the code, I see both are still options. If I read this correctly, it checks for the old `IntegerFieldModuloP`, then looks for the new `IntegerMontgomeryFieldModuloP`. It appears to use the new one always. Why doesn't it just replace the old implementation entry in the `fields` Map? Is there a reason to keep it around?
> 
> Hmm, thats a good point I haven't fully considered; i.e. (if I read correctly) "for `CurveDB.P_256` remove the fallback path to non-montgomery entirely".. that might also help in cleaning a few things up in the construction. Maybe even get rid of this nested ECOperations inside ECOperations.. Perhaps nesting isnt a big deal, but all attempts to make the ECC stack clearer is positive!
> 
> One functional reason that might justify keeping it as-is, is fuzz-testing; with the fallback available, I am able to write the included Fuzz tests and have them check the values against the existing implementation. While I also included a few KAT tests using openssl-generated values, the fuzz tests check millions of values and it does add a lot more certainty about correctness of this code.

I hadn't looked at your fuzz test until you mentioned it.  I see you are using reflection to change the values.  Is that what you mean by "fallback"?  I'm assuming there is no to access the older implementation without reflection.

> 
> Can it be removed? For the operations that do not involve multiplication (i.e. `setSum(*)`), montgomery is expensive. I think I did go through the uses of this code some time back (i.e. ECDHE, ECDSA and KeyGeneration) and existing IntegerPolynomialP256 is no longer used (I should verify that again) and only P256OrderField remains non-montgomery. So removing references to IntegerPolynomialP256 in ECOperations should be possible and cleaner. Removing IntegerPolynomialP256 from MontgomeryIntegerPolynomialP256 is harder (fromMontgomery() uses IntegerPolynomialP256) but perhaps also worth some thought..
> 
> I tend to like `ECOperationsFuzzTest.java` and would prefer to keep it, but it could also be chucked up as part of 'scaffolding' and removed in name of code quality?

I wouldn't rip out the old implementation.  I have been wondering if we should make the older implementation available, maybe by security property.  I was looking at the static Maps at the top of `ECOperations`, `forParameters`, and the constructors where it checks if the `montgomeryOps` was null or set.  It would be nice if we could have one set of `fields` Maps by putting the montgomery entry into the `fields` to replace it.  I think that should work because `IntegerMontgomeryFieldModuloP` extends `IntegerFieldModuloP`.  `instanceof` or other `montgomeryOps`  checks would still need to exist because not all the `fields` support mongomery, and the older implementation would still be accessible for your fuzz tester.   At least that is my theory.

> 
> Thanks @ascarpino
> 
> PS: Perhaps there is some middle ground, remove the `ECOperations montgomeryOps` nesting, and construct (somehow?? singleton makes most things inaccessible..) the reference ECOperations in the fuzz test instead.. not sure how yet, but perhaps worth a further thought..

It would be nice to remove the nesting and it would be nice to be a singleton.  Maybe some combination of what I mentioned above chance can help that.  I haven't fully thought this out either.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/18583#issuecomment-2050148475