[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] tests/microbit-test: Add tests for nRF51 NVMC
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH] tests/microbit-test: Add tests for nRF51 NVMC |
Date: |
Wed, 30 Jan 2019 09:53:34 +0800 |
On Tue, Jan 29, 2019 at 7:47 PM Peter Maydell <address@hidden> wrote:
> On Mon, 28 Jan 2019 at 19:01, Peter Maydell <address@hidden> wrote:
> > This assertion fails on PPC64 hosts:
> > ERROR:/home/pm215/qemu/tests/microbit-test.c:181:fill_and_erase:
> > assertion failed (qtest_readl(qts, base + i * 4) == i): (16777216 ==
> > 1)
> >
> > I suspect an endianness issue, given that 16777216 is 0x0100_0000.
> > The test looks OK, though -- a word write ought to come back
> > the same on a word read -- so maybe it's in the implementation?
>
> The problem here is that the read path goes directly to the
> underlying host memory, but the write path goes via the
> flash_write() function, and the two disagree about the
> representation of the underlying storage.
>
> I think that declaring s->storage to be a uint32_t* is a mistake --
> the underlying RAM needs to be treated as a byte array, not as an
> array of host-endian-order 32-bit words (because the direct-to-RAM
> path effectively treats it as a byte array). Then, since the
> flash_write() function is part of a DEVICE_LITTLE_ENDIAN
> MemoryRegionOps, its 'value' argument is a 32-bit LE value, and
> it should do the modification of s->storage via something like
> oldval = ldl_le_p(s->storage + offset);
> oldval &= value;
> stl_le_p(s->storage + offset, oldval);
>
> Another reference to s->storage also simplifies when we fix its
> type: s->storage + value / 4 becomes just s->storage + value.
>
> NB: untested!
Thanks for looking into this! I'll retest on a big-endian host and
send a new revision.
Stefan