Re: [Qemu-devel] [PATCH] qtest: add read/write accessors with a specific endianness

[Cédric Le Goater]

On 10/05/2016 03:53 PM, Peter Maydell wrote:
> On 5 October 2016 at 06:49, Cédric Le Goater <address@hidden> wrote:
>> On 10/05/2016 02:31 PM, Peter Maydell wrote:
>>> Here I definitely disagree. I think it makes much more sense
>>> for writes to be "what the guest CPU would write", because that's
>>> where we're injecting them. If we had a test framework where the
>>> test was talking directly to the device, then maybe, but we don't.
>>
>> This is how it all started with the test I wrote. Quite ignorant
>> of the middle layers, I used cpu_to_be32() as I would have done
>> on the guest and then realized that qtest was byte-swapping also
>> in some cases and so the test failed on a BE host.
> 
> At this point we're back to "I'd need to look at the actual test
> and device code to identify what's going wrong"...

ok. This is clearly not critical. When ever you have time. 

>>>> Basically, the existing byteswaps, instead of removing the need for
>>>> them in the testcase code, instead mean that target-conditional
>>>> byteswaps will be *required* in nearly every testcase.  It's a recipe
>>>> for endianness-unsafe testcases.
>>>
>>> I prefer the swaps to be explicit in the test cases. If your
>>> test case running on the real CPU would have had to do
>>> "swap to LE and then write this word", so does the test
>>> case in our test framework. If your test case just does
>>> "write the word", then so does the test.
>>>
>>> Most devices IME do not require byteswaps by guest code,
>>> and I think these functions are confusing -- if you try
>>> to use them to write tests for ARM devices, for instance,
>>> the _le versions of these functions will introduce an
>>> incorrect byteswap on a BE host, even though ARM CPUs and
>>> devices are typically all LE.
>>
>> hmmm, yes I agree :/ My scenario is for a ARM SoC running LE
>> on a BE host and accessing a BE device.
>>
>> So how do we handle the possible bswap due to the host and
>> guest endianness being different ?  I am confused. Should we
>> try to remove the tswap from qtest ?
> 
> Which tswap? Last time I worked through the stack of
> what happens I thought that we had the right set of
> swaps in the right places.

The one I am talking about are under qtest_process_command(), 
see below.

Thanks,

C. 


    } else if (strcmp(words[0], "writeb") == 0 ||
               strcmp(words[0], "writew") == 0 ||
               strcmp(words[0], "writel") == 0 ||
               strcmp(words[0], "writeq") == 0) {
        uint64_t addr;
        uint64_t value;

        g_assert(words[1] && words[2]);
        g_assert(qemu_strtoull(words[1], NULL, 0, &addr) == 0);
        g_assert(qemu_strtoull(words[2], NULL, 0, &value) == 0);

        if (words[0][5] == 'b') {
            uint8_t data = value;
            cpu_physical_memory_write(addr, &data, 1);
        } else if (words[0][5] == 'w') {
            uint16_t data = value;
            tswap16s(&data);
            cpu_physical_memory_write(addr, &data, 2);
        } else if (words[0][5] == 'l') {
            uint32_t data = value;
            tswap32s(&data);
            cpu_physical_memory_write(addr, &data, 4);
        } else if (words[0][5] == 'q') {
            uint64_t data = value;
            tswap64s(&data);
            cpu_physical_memory_write(addr, &data, 8);
        }
        qtest_send_prefix(chr);
        qtest_send(chr, "OK\n");
    } else if (strcmp(words[0], "readb") == 0 ||
               strcmp(words[0], "readw") == 0 ||
               strcmp(words[0], "readl") == 0 ||
               strcmp(words[0], "readq") == 0) {
        uint64_t addr;
        uint64_t value = UINT64_C(-1);

        g_assert(words[1]);
        g_assert(qemu_strtoull(words[1], NULL, 0, &addr) == 0);

        if (words[0][4] == 'b') {
            uint8_t data;
            cpu_physical_memory_read(addr, &data, 1);
            value = data;
        } else if (words[0][4] == 'w') {
            uint16_t data;
            cpu_physical_memory_read(addr, &data, 2);
            value = tswap16(data);
        } else if (words[0][4] == 'l') {
            uint32_t data;
            cpu_physical_memory_read(addr, &data, 4);
            value = tswap32(data);
        } else if (words[0][4] == 'q') {
            cpu_physical_memory_read(addr, &value, 8);
            tswap64s(&value);
        }
        qtest_send_prefix(chr);
        qtest_sendf(chr, "OK 0x%016" PRIx64 "\n", value);