On Thu Sep 4, 2025 at 3:57 PM JST, Alistair Popple wrote:
<snip>
>> > +}
>> > +
>> > +// This next section contains constants and structures hand-coded from
>> > the GSP
>> > +// headers We could replace these with bindgen versions, but that's a bit
>> > of a
>> > +// pain because they basically end up pulling in the world (ie.
>> > definitions for
>> > +// every rpc method). So for now the hand-coded ones are fine. They are
>> > just
>> > +// structs so we can easily move to bindgen generated ones if/when we
>> > want to.
>> > +
>> > +// A GSP RPC header
>> > +#[repr(C)]
>> > +#[derive(Debug, Clone)]
>> > +struct GspRpcHeader {
>> > + header_version: u32,
>> > + signature: u32,
>> > + length: u32,
>> > + function: u32,
>> > + rpc_result: u32,
>> > + rpc_result_private: u32,
>> > + sequence: u32,
>> > + cpu_rm_gfid: u32,
>> > +}
>>
>> This is the equivalent of `rpc_message_header_v03_00` in OpenRM. The
>> fact it is versioned makes me a bit nervous. :) If the layout change
>> somehow, we are in for a fun night of debugging. This is where having an
>> opaque abstraction built on top of a bindgen-generated type would be
>> handy: if the layout changes in an incompatible way, when the
>> abstraction would break at compile-time.
>
> Argh! I guess I wrote that before we were generating the headers and I never
> thought to go back and change that. Will fix these to use the generated
> binding.
>
> I will sync up with you offline but I'm not really understanding the point
> here.
> If a bindgen generated type changes in some incompatible way wouldn't we
> already
> get a compile-time error? And if the bindgen generated type changes, what's to
> say the rest of the logic hasn't also changed?
>
> Whilst I'm not totally opposed to something like this it just seems premature
> - the ABI is supposed to be stabalising and in practice none of the structures
> we care about appear to have changed in the 3 years since 525.53 was released.
> So IHMO it would be better to just deal with these changes if (not when) they
> happen rather than try and create an abstraction now, especially as this is
> only
> supposed to become more stable.
While I also hope we will achieve some level of ABI stability, I want to
provision a bit just in case this goal turns out to be too idealistic.
At the moment we are touching bindings internals a bit everywhere in the
`gsp` module. As the driver matures, that trend can only continue - if
one day we realize that we need a firmware version abstraction after
all, it will get increasingly difficult (and time-consuming) to shoehorn
back as time goes. It is much easier to do this from the start.
Also, having a proper abstraction objectively results in better code. I
will share the bits I have written with you for testing purposes, but I
think you will agree that this makes the GSP module much cleaner,
focused on the higher-level operations, while the lower-level code is
divided into easy-to-understand methods, right next to the type they
manipulate instead of being inlined as part of the sub-logic of another
function. Even without the prospect of multiple firmware versions, it is
worth doing.
<snip>
>> Doing so is valuable for clarity as well as future compatibility, as it
>> clearly shows in a single page of code how the header is used. Here is
>> all the code operating on it, in a single block instead of being spread
>> through this file:
>>
>> impl MsgqTxHeader {
>> pub(crate) fn new(msgq_size: u32, msg_count: u32,
>> rx_hdr_offset: u32) -> Self {
>> Self(bindings::msgqTxHeader {
>> version: 0,
>> size: msgq_size,
>> msgSize: GSP_PAGE_SIZE
>> as u32,
>> msgCount: msg_count as
>> u32,
>> writePtr: 0,
>> flags: 1,
>> rxHdrOff: rx_hdr_offset,
>> entryOff: GSP_PAGE_SIZE
>> as u32,
>> })
>> }
>>
>> pub(crate) fn write_ptr(&self) -> u32 {
>> let ptr = (&self.0.writePtr) as *const
>> u32;
>>
>> unsafe { ptr.read_volatile() }
>> }
>>
>> pub(crate) fn set_write_ptr(&mut self, val: u32) {
>> let ptr = (&mut self.0.writePtr) as
>> *mut u32;
>>
>> unsafe { ptr.write_volatile(val) }
>> }
>> }
>
> Yes, this makes a lot of sense although I'm still not seeing the value of the
> [repr(transparent)] representation. Hopefully you can explain during out sync
> up ;)
This type is shared with the GSP, so we must ensure that its layout is
exactly the same as the C structure it wraps.
