Add documentation for Image Loading #48
Conversation
mlvisaya
requested review from
swenson,
parvathib,
mhatrevi,
vsonims and
helloxiling
- Update images - Add detailed steps for the image loading - Remove extra code - Use async in the interface methods
- Changed APIs - Added sequence diagram - Changed example SOC design - Add detailed steps
- update the steps in image loading - update the SOC sample
docs/src/image_loading.md
Outdated
| 4. Caliptra RT indicates to Recovery I/F that it is ready for the SOC manifest image (refer to [Caliptra Subsystem Recovery Sequence](https://github.com/chipsalliance/Caliptra/blob/main/doc/Caliptra.md#caliptra-subsystem-recovery-sequence) for the detailed steps). | ||
| 5. Retrieve SOC Manifest | ||
|
|
||
| 1. If image is coming from BMC, BMC FW transfers manifest to Recovery I/F |
There was a problem hiding this comment.
Minor: It might be better to rephrase as "If image is coming from BMC, SoC manifest is retrieved from Recovery I/F", since we don't need to define BMC FW behavior in our spec.
There was a problem hiding this comment.
I think you're right about not mentioning BMC FW. Although the for the Flash use case, the SoC manifest is also retrieved from Recovery I/F.
Should I just say BMC Recovery Agent and follow the wording in step 11 of https://github.com/chipsalliance/Caliptra/blob/main/doc/Caliptra.md#subsystem-boot-flow ?
There was a problem hiding this comment.
yeah, it's good to reword as BMC's Recovery Agent.
| /// # Returns | ||
| /// - `Ok()`: Image has been loaded and authorized succesfully. | ||
| /// - `Err(DynError)`: Indication of the failure to load or authorize the image. | ||
| async fn load_and_authorize(&self, image_id: u32, address: u64) -> Result<(), DynError>; |
There was a problem hiding this comment.
Is there a reason that we define load_and_authorize as one function instead splitting into two?
There was a problem hiding this comment.
I think that is how we typically do it. We generally do both steps as once to reduce back and forth, since we aren't really going to want to do one without the other, right?
There was a problem hiding this comment.
On one hand, splitting them to 2 might offer some flexibility to the user, but I can't think of a use-case where the user just loads it and not authorize it immediately. I think the advantage of 1 function is just it's simpler to use, and as Chris said reduces back and forth with the application. Also i'd like to prevent them from calling authorize() first before calling load(), so making it atomic just ensures these 2 steps are followed in sequence.
There was a problem hiding this comment.
I was thinking from the perspective of interface definition, usually one function just does one specific job.
I agree that reducing back-and-forth interaction with app in our scenario is a valuable point. Let's keep it as it is.
There was a problem hiding this comment.
I have a few suggestions on modifying the diagram:
- Let's use
flash controllerrather than SPI to represent flash interface consistently. - Flash storage should be connected to flash controller.
- It might be better to replace the box
BMCwith something likePLDM -T5 FW package for streaming boot. BMC is not on the equivalent position of flash storage.
There was a problem hiding this comment.
Ack. How about "PLDM FW Update Agent" instead of BMC?
docs/src/image_loading.md
Outdated
| 11. Caliptra RT FW will instruct Caliptra HW to read MCU SRAM and generate the hash (Caliptra HW will use the SHA accelerator) | ||
| 12. Caliptra RT FW will use this hash and verify it against the hash in the SOC manifest. | ||
| 13. Once digest is verified, the EXEC/GO bit is set | ||
| 14. The EXEC/GO bit sets a Caliptra wire to MCU (as a consequence of setting the EXEC/GO bit in the previous step) |
There was a problem hiding this comment.
Probably you could combine 14, 15, and maybe even 16.
docs/src/image_loading.md
Outdated
| 15. MCU ROM detects Caliptra'go' wire to the MCU got set. | ||
| 16. MCU ROM passes a parameter using the FW HandOff table to indicate the image source (i.e. the image source where it booted from) |
There was a problem hiding this comment.
| 15. MCU ROM detects Caliptra'go' wire to the MCU got set. | |
| 16. MCU ROM passes a parameter using the FW HandOff table to indicate the image source (i.e. the image source where it booted from) | |
| 15. MCU ROM detects Caliptra 'go' wire to the MCU got set. | |
| 16. MCU ROM passes a parameter using the FW HandOff table to indicate the image source (i.e., the image source where it booted from) |
| /// # Returns | ||
| /// - `Ok()`: Image has been loaded and authorized succesfully. | ||
| /// - `Err(DynError)`: Indication of the failure to load or authorize the image. | ||
| async fn load_and_authorize(&self, image_id: u32, address: u64) -> Result<(), DynError>; |
There was a problem hiding this comment.
I think that is how we typically do it. We generally do both steps as once to reduce back and forth, since we aren't really going to want to do one without the other, right?
- Updated Diagram (renamed BMC to PLDM Update Agent, remove SOC Config, replaced SPI with Flash Controller) - Updated Sequence Diagram - Various enhancement on the step description
|
Thanks!! |
No description provided.