Refactor/mailbox

[mmerklinger]

• Cleans up register, field, and status definitions
• Changes name of typedef
• Refactors the request function into multiple functions

This is a refactor for future features.

[anarchuser]

Suggested change

	unsigned int mailbox_message_to_register_value (mailbox_message_t * message);
	unsigned int mailbox_message_to_register_value (mailbox_message_t message);

mailbox_message_t is 4 bytes long, there's no need to pass it as pointer. Generally, as long as structs are 8 bytes maximum, you can safely pass them by value - after all, the pointer you'd pass otherwise is 8 bytes.

Passing by value ensures no modifications happen and, since the compiler can reason about this and doesn't need another indirection, is likely to be faster.

If you do want to pass by reference, you should probably at least make it mailbox_message const * message, as promise that you don't modify it.

The same goes for mailbox_read and mailbox_write; none of these modify message

[anarchuser]

As far as I can tell, what you try to do here is to merge 28 bits of data with 4 bits of channel into the 32 bit message the mailbox expects. I.e., you try to combine message into this format:

|            data            |chnl|
|^^^^^^^^^^^^^^^^^^^^^^^^^^^^|^^^^|
|-------------28-------------|--4-|

Now, have a look at the structure of mailbox_message_t:

typedef struct {
    unsigned int data    : 28;// most significant bits contain shared memory address
    channel_t    channel : 4; // least four significant bits contain channel
} mailbox_message_t;

Incidentally, this structure is 4 bytes long and gets packed into this format:

|            data            |chnl|
|^^^^^^^^^^^^^^^^^^^^^^^^^^^^|^^^^|
|-------------28-------------|--4-|

That means the data is already in the proper format; all we have to do now is tell the compiler that it is what it should be:

• Casting (assuming you pass message by value, otherwise omit the &):

Suggested change

	return ((unsigned int) ((long) message->data) & ~0xF) | (message->channel & 0xF);
	return * (unsigned int *) & message;

• Bit-wise OR:

Suggested change

	return ((unsigned int) ((long) message->data) & ~0xF) | (message->channel & 0xF);
	return (unsigned int) (message->data << 4) | message->channel; // no need to specifically clear bits

[anarchuser]

I just realised this issue was caused by me not communicating clearly how mailbox_message_t should be used -
message.data is supposed to store the 28 most significant bits of the memory address after it has been truncated to a 32bit integer. We know the 4 least significant bits are 0 anyways. I should've made that clear and provide an appropriate Initialise function.

[mmerklinger]

I changed the source to pass mailbox_message_t by value. Unfortunately, the cast you suggested compiled, but didn't work on the running machine. I had errors on the console. Therefore, I simplified it to return (message.data & ~0xF) | message.channel;.

[anarchuser]

It is dependent on this change: https://github.com/anarchuser/rose/pull/63/files/d06e110f36f681d0931ad45cffad47b15c71abb9#r767591035

[mmerklinger]

I saw that, but see my comment below. As soon as I do the cast like this the boot hangs at src/common/rng.c:15:[0] Initialised RNG successfully.

[anarchuser]

as for why it hangs is because if data doesn't point to a proper message, the mailbox doesn't do anything

[mmerklinger]

Yes, because otherwise the above mentioned crash happens. In the function where I cast the mailbox message for the register I zero out the 4 Bit.

[anarchuser]

so you get the crash if 1) you shift and 2) you do not zero out the 4 LSB in the register function?

[mmerklinger]

Correct.

[anarchuser]

okay, then leave it be for now. Maybe I'll have a look at it tomorrow, the error is probably more subtle

[anarchuser]

Suggested change

	mailbox_message_t message = {buffer, channel};
	mailbox_message_t message = {(unsigned int) buffer >> 4, channel}; // remove least significant 4 bits of buffer

see my explanation for mailbox_message_to_register_value

[mmerklinger]

This fails to compile with invalid operands to binary >> (have ‘volatile unsigned int *’ and ‘int’). Tried to solve it with a cast to type long.

[anarchuser]

preferably cast to unsigned int but both should work

[anarchuser]

I updated the suggestion

[mmerklinger]

In that case I got a compiler warning because unsigned int is not necessarily same length as the pointer. This is why I took long.

[anarchuser]

fair. In the end, we cast it to 32 bit implicitly