Conversation
davidchisnall
left a comment
davidchisnall
left a comment
There was a problem hiding this comment.
Thanks for posting this for early review. I've left some comments inline, I hope they're helpful.
…ase should be similar
nwf-msr
left a comment
nwf-msr
left a comment
There was a problem hiding this comment.
This looks like a good start. Good job onboarding enough xmake and __cheri_ annotations to be dangerous. :)
Sorry if any of my comments &| questions below are misguided as a result of having spent too much time with software.
| * this statement returns the less signifance bit | ||
| * to show the halted status | ||
| */ | ||
| return dmaInterface->status & 0x1; |
There was a problem hiding this comment.
Could you repurpose control's bottom bit rather than needing a separate status word?
| * claim the memory | ||
| */ | ||
|
|
||
| size_t sourceClaimSize = heap_claim(dmaHeap, sourceAddress); |
There was a problem hiding this comment.
I wonder if this is sufficient grounds to want a heap_claim(SObj heapCapability, void*... objects) that amortizes the compartment call overhead. Similarly for heap_free(). @davidchisnall?
There was a problem hiding this comment.
That’s a good idea. In particular, the call could atomically succeed or fail, which simplifies error handling for the caller. I think we’d want to bound the maximum number of objects that can be claimed in a single operation and we can’t used variadic an across compartment calls so it would have to pass an array and a length.
| * A function below claims the source and target addresses of the DMA interface. | ||
| * While, DMA is in progress, these addresses should not be freed | ||
| */ | ||
| __cheri_compartment("dma") int claim_dma(uint32_t *sourceAddress, |
There was a problem hiding this comment.
I think these (claim and free) functions are not what you mean to be exporting from the compartment. I was expecting something something more like write_conf (though probably a fusion of write_conf, write_strides, and start), I think.
To support multiple users of DMA, we probably want that "memmove_but_with_dma" call, whatever it's named, to take a Timeout for waiting on the DMA engine to become available and for it to use a futex to communicate with the engine's IRQ handler? But that's perhaps a discussion for later.
Co-authored-by: Nathaniel Filardo <105816689+nwf-msr@users.noreply.github.com>
Co-authored-by: Nathaniel Filardo <105816689+nwf-msr@users.noreply.github.com>
| * claim the memory | ||
| */ | ||
|
|
||
| size_t sourceClaimSize = heap_claim(dmaHeap, sourceAddress); |
There was a problem hiding this comment.
That’s a good idea. In particular, the call could atomically succeed or fail, which simplifies error handling for the caller. I think we’d want to bound the maximum number of objects that can be claimed in a single operation and we can’t used variadic an across compartment calls so it would have to pass an array and a length.
| /** | ||
| * Setting source and target addresses, and length fields | ||
| */ | ||
| device().sourceAddress = *sourceAddress; |
There was a problem hiding this comment.
This line looks wrong. You’re dereferencing the pointer and using whatever it happens to point to as the address. I think you want to be using the address from the capability. Same comment for the next line.
…ase should be similar
Co-authored-by: Nathaniel Filardo <105816689+nwf-msr@users.noreply.github.com>
Co-authored-by: Nathaniel Filardo <105816689+nwf-msr@users.noreply.github.com>
|
I swear I hit "cancel review" to get rid of some stale notes. Sorry for the noise. |
4 participants
[Not a candidate for actually merging yet!]
Thanks for the comments Wes! I have already checked them, however will still create a PR here for David with older version before changing/coding other pieces.
Thanks,
Best wishes