Hi Lyude, thanks for the review!
On Sat May 31, 2025 at 7:22 AM JST, Lyude Paul wrote:
<snip>
>> + /// `target_mem`.
>> + ///
>> + /// `sec` is set if the loaded firmware is expected to run in secure
>> mode.
>> + fn dma_wr(
>> + &self,
>> + bar: &Bar0,
>> + dma_handle: bindings::dma_addr_t,
>> + target_mem: FalconMem,
>> + load_offsets: FalconLoadTarget,
>> + sec: bool,
>> + ) -> Result {
>> + const DMA_LEN: u32 = 256;
>> +
>> + // For IMEM, we want to use the start offset as a virtual address
>> tag for each page, since
>> + // code addresses in the firmware (and the boot vector) are virtual.
>> + //
>> + // For DMEM we can fold the start offset into the DMA handle.
>> + let (src_start, dma_start) = match target_mem {
>> + FalconMem::Imem => (load_offsets.src_start, dma_handle),
>> + FalconMem::Dmem => (
>> + 0,
>> + dma_handle + load_offsets.src_start as bindings::dma_addr_t,
>
> I wonder if maybe we shouldn't use dma_addr_t directly from bindings and add a
> proper type alias for it somewhere?
I guess so, let me see if I can easily change CoherentAllocation to
support that (and address Danilo's related comment).
>> +pub(crate) trait FalconHal<E: FalconEngine>: Sync {
>> + // Activates the Falcon core if the engine is a risvc/falcon dual
>> engine.
>> + fn select_core(&self, _falcon: &Falcon<E>, _bar: &Bar0) -> Result<()> {
>
> Could just be Result
>
>> + Ok(())
>> + }
>> +
>> + /// Returns the fused version of the signature to use in order to run a
>> HS firmware on this
>> + /// falcon instance. `engine_id_mask` and `ucode_id` are obtained from
>> the firmware header.
>> + fn get_signature_reg_fuse_version(
>> + &self,
>> + falcon: &Falcon<E>,
>> + bar: &Bar0,
>> + engine_id_mask: u16,
>> + ucode_id: u8,
>> + ) -> Result<u32>;
>> +
>> + // Program the boot ROM registers prior to starting a secure firmware.
>> + fn program_brom(&self, falcon: &Falcon<E>, bar: &Bar0, params:
>> &FalconBromParams)
>> + -> Result<()>;
>
> Same here
Urrk I've done this throughout the code. Fixed them all, thanks.
