Re: [PATCH v6 00/41] Raspberry Pi 4B machine

[Peter Maydell <peter.maydell@linaro.org>]

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