Re: [PATCH RFC v2 9/9] tests/qtest: add test for memory region access

[Peter Xu <peterx@redhat.com>]

On Mon, Sep 01, 2025 at 05:57:57PM +0100, Peter Maydell wrote:
> On Fri, 22 Aug 2025 at 10:26, CJ Chen <cjchen@igel.co.jp> wrote:
> >
> > From: Tomoyuki Hirose <hrstmyk811m@gmail.com>
> >
> > This commit adds a qtest for accessing various memory regions. The
> > qtest checks the correctness of handling the access to memory regions
> > by using 'memaccess-testdev'.
> >
> > Signed-off-by: CJ Chen <cjchen@igel.co.jp>
> > Co-developed-by: CJ Chen <cjchen@igel.co.jp>
> > Reported-by: Tomoyuki Hirose <hrstmyk811m@gmail.com>
> > ---
> >  tests/qtest/memaccess-test.c | 597 +++++++++++++++++++++++++++++++++++
> >  tests/qtest/meson.build      |   9 +
> >  2 files changed, 606 insertions(+)
> >  create mode 100644 tests/qtest/memaccess-test.c
> 
> There seems to be a lot of duplication in these test functions
> (for instance, aren't all of little_b_valid(), little_b_invalid(),
> big_b_valid() and big_b_invalid() identical?  and the various
> _invalid functions seem to have if() blocks where the code in
> the if and the else halves is the same).

Besides that, I don't yet understand some of the test code on endianess,
this might be relevant to the question I raised in the other reply.

Taking example of big_w_valid() test:

static void big_w_valid(QTestState *qts, hwaddr offset)
{
    if (qtest_big_endian(qts)) {
        qtest_writew(qts, base + offset + 0, 0x1100);                     <--- [1]
        qtest_writew(qts, base + offset + 1, 0x3322);                     <--- [2]
        qtest_writew(qts, base + offset + 2, 0x5544);
        qtest_writew(qts, base + offset + 3, 0x7766);
        qtest_writew(qts, base + offset + 4, 0x9988);
        qtest_writew(qts, base + offset + 5, 0xbbaa);
        qtest_writew(qts, base + offset + 6, 0xddcc);
        qtest_writew(qts, base + offset + 7, 0xffee);
        g_assert_cmphex(qtest_readw(qts, base + offset + 0), ==, 0x1133); <--- [3]
        g_assert_cmphex(qtest_readw(qts, base + offset + 1), ==, 0x3355);
        g_assert_cmphex(qtest_readw(qts, base + offset + 2), ==, 0x5577);
        g_assert_cmphex(qtest_readw(qts, base + offset + 3), ==, 0x7799);
        g_assert_cmphex(qtest_readw(qts, base + offset + 4), ==, 0x99bb);
        g_assert_cmphex(qtest_readw(qts, base + offset + 5), ==, 0xbbdd);
        g_assert_cmphex(qtest_readw(qts, base + offset + 6), ==, 0xddff);
        g_assert_cmphex(qtest_readw(qts, base + offset + 7), ==, 0xffee);
    } else {
        qtest_writew(qts, base + offset + 0, 0x1100);                     <--- [4]
        qtest_writew(qts, base + offset + 1, 0x3322);                     <--- [5]
        qtest_writew(qts, base + offset + 2, 0x5544);
        qtest_writew(qts, base + offset + 3, 0x7766);
        qtest_writew(qts, base + offset + 4, 0x9988);
        qtest_writew(qts, base + offset + 5, 0xbbaa);
        qtest_writew(qts, base + offset + 6, 0xddcc);
        qtest_writew(qts, base + offset + 7, 0xffee);
        g_assert_cmphex(qtest_readw(qts, base + offset + 0), ==, 0x2200); <--- [6]
        g_assert_cmphex(qtest_readw(qts, base + offset + 1), ==, 0x4422);
        g_assert_cmphex(qtest_readw(qts, base + offset + 2), ==, 0x6644);
        g_assert_cmphex(qtest_readw(qts, base + offset + 3), ==, 0x8866);
        g_assert_cmphex(qtest_readw(qts, base + offset + 4), ==, 0xaa88);
        g_assert_cmphex(qtest_readw(qts, base + offset + 5), ==, 0xccaa);
        g_assert_cmphex(qtest_readw(qts, base + offset + 6), ==, 0xeecc);
        g_assert_cmphex(qtest_readw(qts, base + offset + 7), ==, 0xffee);
    }
}

It tests on all MRs that are (1) device using big endianess, (2)
.valid.min_access_size=2, (3) .valid.unaligned=true.

First of all, I don't understand why a test case needs to behave
differently according to the TARGET endianess, aka, qtest_big_endian().
IIUC, each of the qtest_writew() should request a WRITE with an integer
value to be applied to the MMIO region, when we know the endianess of the
region (in this case, big endian), we know exactly how it will be read out.

Taking above steps [1-3] as example.  Here [1+2] will write two words to
offset 0x0, 0x1 correspondingly:

  - [1] WRITE(addr=0x0, size=2, data=0x1100)
  - [2] WRITE(addr=0x1, size=2, data=0x3322)

Here, IMHO the result should not depend on the internal property of the
systems (e.g. MR .impl values, after we have unaligned support memory core
should resolve all of these issues by either split 2B MMIO into two 1B, or
do proper padding on start/end to amplify the write if necessary).  Because
we know the device / MR is big endianess, so we should know the result of
the write already, as below:

  - After [1] WRITE(addr=0x0, size=2, data=0x1100), data should look like:

    [0x11, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, ...]
     ^^^^^^^^^^^
    Here it should always follow device's endianess.

  - After [2] WRITE(addr=0x1, size=2, data=0x3322), data should look like:

    [0x11, 0x33, 0x22, 0x00, 0x00, 0x00, 0x00, 0x00, ...]
           ^^^^^^^^^^^
    Here it should always follow device's endianess.

Above would be verified by step [3].  Basically it verifies READ(addr=0x0,
size=2) would result in 0x1133, which looks correct.

However the problem is, when GUEST is little endian, the test, even if
written the same data [4-5], expecting different results [6].  That's the
part I don't understand.  I think it would make sense if [6] should also
verify the same as [3].  IOW, the chunk in the "if" section looks like the
right thing to test for both big/little GUEST endianess.

-- 
Peter Xu