Re: [PATCH v12 00/10] Implementation of NPI Mailbox and GMAC Networking Module

[Peter Maydell]

On Sat, 13 Jan 2024 at 12:27, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Wed, 10 Jan 2024 at 23:42, Nabih Estefan <nabihestefan@google.com> wrote:
> >
> > From: Nabih Estefan Diaz <nabihestefan@google.com>
> >
> > [Changes since v11]
> > Was running into error syncing into master. It seemed to be related to a
> > hash problem introduced in patchset 10 (unrelated to the macOS build
> > issue). carried the patches from v9 (before the syncing problem) and
> > added the fixes from patchsets 10 and 11 to remove the hash error.
> >
>
> I found some more issues with this which I've noted in the
> individual patches: in particular the patchset is supposed
> to compile after every patch, and to get that to happen
> a few sections of code needed to be in different patches.
> There were also a couple of other niggles.
>
> However, since the fixes were minor, I have made them myself
> in applying this series to target-arm.next, to save you having
> to do yet another respin.

It turns out that the tests don't pass on a big-endian host.

Some of this is just bugs in the test case code, like this,
where the protocol between the device and the chardev
expects the offset in little-endian byte order but the
test case was not ensuring that:

diff --git a/tests/qtest/npcm7xx_pci_mbox-test.c
b/tests/qtest/npcm7xx_pci_mbox-test.c
index 341642e6371..d1714f44517 100644
--- a/tests/qtest/npcm7xx_pci_mbox-test.c
+++ b/tests/qtest/npcm7xx_pci_mbox-test.c
@@ -101,6 +101,7 @@ static void receive_data(uint64_t offset, uint8_t
*buf, size_t len)
     ssize_t rv;

     while (len > 0) {
+        uint8_t offset_bytes[8];
         uint8_t size;

         if (len >= 8) {
@@ -118,7 +119,8 @@ static void receive_data(uint64_t offset, uint8_t
*buf, size_t len)
         rv = write(fd, &op, 1);
         g_assert_cmpint(rv, ==, 1);
         /* Write offset */
-        rv = write(fd, (uint8_t *)&offset, sizeof(uint64_t));
+        stq_le_p(offset_bytes, offset);
+        rv = write(fd, offset_bytes, sizeof(uint64_t));
         g_assert_cmpint(rv, ==, sizeof(uint64_t));
         /* Write size */
         g_assert_cmpint(write(fd, &size, 1), ==, 1);
@@ -141,6 +143,7 @@ static void send_data(uint64_t offset, const
uint8_t *buf, size_t len)

     while (len > 0) {
         uint8_t size;
+        uint8_t offset_bytes[8];

         if (len >= 8) {
             size = 8;
@@ -157,7 +160,8 @@ static void send_data(uint64_t offset, const
uint8_t *buf, size_t len)
         rv = write(fd, &op, 1);
         g_assert_cmpint(rv, ==, 1);
         /* Write offset */
-        rv = write(fd, (uint8_t *)&offset, sizeof(uint64_t));
+        stq_le_p(offset_bytes, offset);
+        rv = write(fd, offset_bytes, sizeof(uint64_t));
         g_assert_cmpint(rv, ==, sizeof(uint64_t));
         /* Write size */
         g_assert_cmpint(write(fd, &size, 1), ==, 1);


(Side note, if you find yourself casting a uint64_t* to
to a uint8_t* this is almost always a sign of endianness
problems. There are similar casts in the device implementation
which are also flags of problems: see below.)

However this isn't sufficient for the test cases to pass,
because the code in the device itself seems to be confused.
For data sent *to* the device, the OP_WRITE code expects
to see the data in little-endian order, which it then
writes to the guest memory as LE. However for data
read *from* the device, npcm7xx_pci_mbox_send_response()
does not do anything about endianness and so the data
is read from the guest memory as LE and then sent out
on the wire in host-endianness order.

Because I don't know what the intention was here I'm not
going to try to fix this up. Please can you have a look
at this? I would also recommend that you write up somewhere
(even if just in a comment) exactly what the protocol
specification for the on-the-wire format is. That will
make it much easier to determine whether a bug is on the
sending side or the receiving side.


For the other minor stuff which I fixed up locally before
sending the pull request: I suggest that you take the
patches as they appeared there, i.e. patches 10..18 from here:
 20240116151228.2430754-1-peter.maydell@linaro.org/">https://patchew.org/QEMU/20240116151228.2430754-1-peter.maydell@linaro.org/
But I've just noticed that I seem to have messed up the
author state on some of those patches (probably while I
was squashing patches into each other etc) -- please
check through and correct those back to the original
authorship attribution.

thanks
-- PMM