CCF icon indicating copy to clipboard operation
CCF copied to clipboard

Deplatforming

Open eddyashton opened this issue 5 months ago • 5 comments

The build-time distinction between SNP and Virtual is unhelpful, results in a huge amount of duplication in our CMake. This was necessary on SGX, where the targets had significantly different build options, but is no longer doing anything.

This PR removes the COMPILE_TARGET option from CMake. This has a bunch of knock-on effects, that I think fall into a few distinct camps:

  1. De-duplicated targets. We no longer have ccf_kv.host and ccf_kv.snp, we just have ccf_kv. I've manually tried to verify in each instance that the calls to each target were always identical, but I may have missed some.
  2. Removal of PLATFORM_ preprocessor define. Most places were #if PLATFORM_VIRTUAL || PLATFORM_SNP, so the #if/#endif pair can be trivially removed. Deciding what kind of quote we produce is now a run-time decision (each binary is capable of trying to produce either format), based on the existing enclave.platform config argument. I think that ought to be promoted to a CLI argument (security critical), but that's deferred.
  3. Test clarity. Functions that check whether an ioctl device is available (in C++ and Python) are now named to show that they're testing for SNP_SUPPORT. The decision of whether this run (of an e2e test or a node) should try to get a virtual or SNP attesation is driven by configuration, not by this capability detection.
  4. Simpler library names. We still (for now) produce a single cchost and then a .so for each "enclave" application. But those applications are now libjs_generic.so, rather than libjs_generic.virtual.so/libjs_generic.snp.so.
  5. Gluing those together it turns out it is still convenient to have a global "default test platform" in CMake, so that we can indicate whether we think we're testing virtual or SNP. That looks a bit like COMPILE_TARGET, but it's now called CCF_TEST_PLATFORM, and hopefully used in few enough places to clarify intent. Perhaps the default value should be derived from capability detection? TBD.

I've only run a handful of builds and tests locally, this is likely missing some obvious breakages, so opening as a Draft initially and awaiting CI coverage.

Also, I think this is a big enough breaking change that it shouldn't affect 6.0, so I propose a release/6.x branch.

Follow-ups:

  • #6623
  • De-duplicate builds in CI: build once and copy to test runners
  • Tear out more enclave/platform distinctions in the test infra. I'm always wary of how hard we can go on this while maintaining some lts_ story, but I think right now we can be pretty aggressive? But let's to it separately to derisk.

eddyashton avatar Jun 11 '25 14:06 eddyashton