[dpe] Make SVN in DeriveContext optional#3963
Conversation
DPE added an SVN field to `DeriveContext`. To avoid making a breaking change to Caliptra integrators, this will append a default SVN value (`0`) to the command if it is not present. This also adds a test to make sure it is handled correctly.
754a093 to
9418cf0
Compare
|
|
||
| if is_derive_context_cmd && is_missing_svn && has_space_for_svn { | ||
| cmd.data[cmd_size..cmd_size + size_of::<u32>()].fill(0); | ||
| cmd.data_size += size_of::<u32>() as u32; |
There was a problem hiding this comment.
What do you think about parsing what we know about the command and serializing a new one; I think it makes the code a bit easier to read / maintainable, but a much more verbose.
| // Strip the last 4 bytes (svn u32 field) to simulate caller without SVN | ||
| let dpe_cmd_buf_no_svn = &dpe_cmd_buf[..dpe_cmd_buf.len() - 4]; | ||
| cmd_data[cmd_hdr_buf.len()..cmd_hdr_buf.len() + dpe_cmd_buf_no_svn.len()] | ||
| .copy_from_slice(dpe_cmd_buf_no_svn); |
There was a problem hiding this comment.
Alternatively could re-define the older version of the command struct, rather than manipulating the serialized form
|
|
||
| let is_derive_context_cmd = hdr.cmd_id == Command::DERIVE_CONTEXT; | ||
| let is_missing_svn = cmd_size == expected_no_svn_len; | ||
| let has_space_for_svn = cmd_size + size_of::<u32>() <= cmd.data.len(); |
There was a problem hiding this comment.
This check here would mean the message still has to be provided with additional space, which would require a change from integrators right?
One thing we could do is detect if a DeriveContext command is being executed and allocate a full buffer then copy over the slice and parse from the new buffer. It would give us a default value of SVN, and not require an integrator change, though that seems a bit brittle to me.
Another option, would be to provide the ability for the DPE library to handle both provided and non-provided svns, likely by providing a way to deserialize either?
This does raise a good point of how we wish to handle DPE command versioning though.
There was a problem hiding this comment.
Suppose we add another set of invoke dpe commands? That helps make the breaking change explicit and gives us a way to translate the previous API in an obvious way.
There is some boiler plate for a new command like this but hopefully it doesn't result in too much code space, since we can just translate the command and call the updated API
There was a problem hiding this comment.
I decided to mock up handling it in the DPE library. I think I like it more, but it does break the mold a little bit. What do you guys think chipsalliance/caliptra-dpe#613?
There was a problem hiding this comment.
I'd prefer handling the translation in caliptra since it's strictly an issue with the caliptra API, and not necessarily the DPE API
3 participants
DPE added an SVN field to
DeriveContext. To avoid making a breaking change to Caliptra integrators, this will append a default SVN value (0) to the command if it is not present.This also adds a test to make sure it is handled correctly.