On Mon, 26 Feb 2024 at 00:04, Sergey Kambalin <serg.oker@gmail.com> wrote:
>
> Introducing Raspberry Pi 4B model.
> It contains new BCM2838 SoC, PCIE subsystem,
> RNG200, Thermal sensor and Genet network controller.
>
> It can work with recent linux kernels 6.x.x.
> Two avocado tests was added to check that.
>
> Unit tests has been made as read/write operations
> via mailbox properties.
>
> Genet integration test is under development.
>
> Every single commit
> 1) builds without errors
> 2) passes regression tests
> 3) passes style check*
> *the only exception is bcm2838-mbox-property-test.c file
> containing heavy macros usage which cause a lot of
> false-positives of checkpatch.pl.
Hi; I had to drop the qtest patches from what I'm going to
apply upstream, because it turns out that they don't pass
if the host system is big-endian. This is because you read
out memory from the guest directly into a host struct:
that only works if the host happens to be the same endianness
(little) as the guest.
One possible way to deal with this would be the following:
+/*
+ * Read and write fields that are in little-endian order. Based on
+ * the linux-user/qemu.h __put_user_e and __get_user_e macros.
+ */
+#define put_guest_field(x, hptr) \
+ do { \
+ (__builtin_choose_expr(sizeof(*(hptr)) == 1, stb_p, \
+ __builtin_choose_expr(sizeof(*(hptr)) == 2, stw_le_p, \
+ __builtin_choose_expr(sizeof(*(hptr)) == 4, stl_le_p, \
+ __builtin_choose_expr(sizeof(*(hptr)) == 8, stq_le_p, abort)))) \
+ ((hptr), (x)), (void)0); \
+ } while (0)
+
+#define get_guest_field(x, hptr) \
+ do { \
+ ((x) = (typeof(*hptr))( \
+ __builtin_choose_expr(sizeof(*(hptr)) == 1, ldub_p, \
+ __builtin_choose_expr(sizeof(*(hptr)) == 2, lduw_le_p, \
+ __builtin_choose_expr(sizeof(*(hptr)) == 4, ldl_le_p, \
+ __builtin_choose_expr(sizeof(*(hptr)) == 8, ldq_le_p, abort)))) \
+ (hptr)), (void)0); \
+ } while (0)
+
/*----------------------------------------------------------------------------*/
#define DECLARE_TEST_CASE(testname, ...)
\
__attribute__((weak))
\
@@ -59,38 +82,41 @@ FIELD(GET_CLOCK_STATE_CMD, NPRES, 1, 1)
} mailbox_buffer = { 0 };
\
\
QTestState *qts = qtest_init("-machine raspi4b");
\
+ uint32_t req_resp_code, tag_id, size_stat, size, success;
\
\
- mailbox_buffer.header.size = sizeof(mailbox_buffer);
\
- mailbox_buffer.header.req_resp_code = MBOX_PROCESS_REQUEST;
\
+ put_guest_field(sizeof(mailbox_buffer),
&mailbox_buffer.header.size); \
+ put_guest_field(MBOX_PROCESS_REQUEST,
\
+ &mailbox_buffer.header.req_resp_code);
\
\
- mailbox_buffer.tag.id = TEST_TAG(testname);
\
- mailbox_buffer.tag.value_buffer_size = MAX(
\
+ put_guest_field(TEST_TAG(testname), &mailbox_buffer.tag.id);
\
+ put_guest_field(MAX(
\
sizeof(mailbox_buffer.tag.request.value),
\
- sizeof(mailbox_buffer.tag.response.value));
\
- mailbox_buffer.tag.request.zero = 0;
\
+ sizeof(mailbox_buffer.tag.response.value)),
\
+ &mailbox_buffer.tag.value_buffer_size);
\
+ put_guest_field(0, &mailbox_buffer.tag.request.zero);
\
\
- mailbox_buffer.end_tag = RPI_FWREQ_PROPERTY_END;
\
+ put_guest_field(RPI_FWREQ_PROPERTY_END,
&mailbox_buffer.end_tag); \
\
if (SETUP_FN_NAME(testname, __VA_ARGS__)) {
\
SETUP_FN_NAME(testname,
__VA_ARGS__)(&mailbox_buffer.tag); \
}
\
\
qtest_memwrite(qts, MBOX_TEST_MESSAGE_ADDRESS,
\
- &mailbox_buffer, sizeof(mailbox_buffer));
\
+ &mailbox_buffer, sizeof(mailbox_buffer));
\
qtest_mbox1_write_message(qts, MBOX_CHANNEL_ID_PROPERTY,
\
- MBOX_TEST_MESSAGE_ADDRESS);
\
+ MBOX_TEST_MESSAGE_ADDRESS);
\
\
qtest_mbox0_read_message(qts, MBOX_CHANNEL_ID_PROPERTY,
\
- &mailbox_buffer, sizeof(mailbox_buffer));
\
+ &mailbox_buffer,
sizeof(mailbox_buffer)); \
\
- g_assert_cmphex(mailbox_buffer.header.req_resp_code, ==,
MBOX_SUCCESS);\
+ get_guest_field(req_resp_code,
&mailbox_buffer.header.req_resp_code); \
+ g_assert_cmphex(req_resp_code, ==, MBOX_SUCCESS);
\
+ get_guest_field(tag_id, &mailbox_buffer.tag.id);
\
+ g_assert_cmphex(tag_id, ==, TEST_TAG(testname));
\
\
- g_assert_cmphex(mailbox_buffer.tag.id, ==,
TEST_TAG(testname)); \
-
\
- uint32_t size =
FIELD_EX32(mailbox_buffer.tag.response.size_stat, \
- MBOX_SIZE_STAT, SIZE);
\
- uint32_t success =
FIELD_EX32(mailbox_buffer.tag.response.size_stat, \
- MBOX_SIZE_STAT, SUCCESS);
\
+ get_guest_field(size_stat,
&mailbox_buffer.tag.response.size_stat); \
+ size = FIELD_EX32(size_stat, MBOX_SIZE_STAT, SIZE);
\
+ success = FIELD_EX32(size_stat, MBOX_SIZE_STAT, SUCCESS);
\
g_assert_cmpint(size, ==,
sizeof(mailbox_buffer.tag.response.value)); \
g_assert_cmpint(success, ==, 1);
\
\
@@ -110,7 +136,9 @@ FIELD(GET_CLOCK_STATE_CMD, NPRES, 1, 1)
/*----------------------------------------------------------------------------*/
DECLARE_TEST_CASE(GET_FIRMWARE_REVISION) {
- g_assert_cmpint(tag->response.value.revision, ==, FIRMWARE_REVISION);
+ uint32_t rev;
+ get_guest_field(rev, &tag->response.value.revision);
+ g_assert_cmpint(rev, ==, FIRMWARE_REVISION);
}
which I have tested works for the one test case that I updated here.
(The field comparison part could probably be made less clunky.)
Or you could do something else.
The test also failed to link on Macos:
https://gitlab.com/qemu-project/qemu/-/jobs/6267744541
Undefined symbols for architecture arm64:
"_FRAMEBUFFER_GET_ALPHA_MODE__setup", referenced from:
_FRAMEBUFFER_GET_ALPHA_MODE__test in bcm2838-mbox-property-test.c.o
"_FRAMEBUFFER_GET_DEPTH__setup", referenced from:
_FRAMEBUFFER_GET_DEPTH__test in bcm2838-mbox-property-test.c.o
(etc)
I'm not sure why that is, but I would be suspicious that your
use of the __attribute__((weak)) for the setup functions is not
portable.
thanks
-- PMM