Re: [Qemu-arm] [Qemu-devel] [PATCH v5 9/9] tests: add a m25p80 test

[Peter Maydell]

On 4 July 2016 at 10:15, Cédric Le Goater <address@hidden> wrote:
> On 07/02/2016 08:05 PM, mar.krzeminski wrote:
>>
>>
>> W dniu 28.06.2016 o 20:24, Cédric Le Goater pisze:

>>> +static void test_erase_sector(void)
>>> +{
>>> +    uint32_t some_page_addr = 0x600 * PAGE_SIZE;
>>> +    uint32_t page[PAGE_SIZE / 4];
>>> +    int i;
>>> +
>>> +    spi_conf(CONF_ENABLE_W0);
>>> +
>>> +    spi_ctrl_start_user();
>>> +    writeb(AST2400_FLASH_BASE, WREN);
>>> +    writeb(AST2400_FLASH_BASE, EN_4BYTE_ADDR);
>>> +    writeb(AST2400_FLASH_BASE, ERASE_SECTOR);
>>> +    writel(AST2400_FLASH_BASE, cpu_to_be32(some_page_addr));
>> Hi,
>>
>> I think you should not make any byte swapping because
>> qtest for all write* instructions (see qtest.c:377).
>
> yes. on a BE, we should not byte swap.
>
> When on a LE host, the cpu_to_be32() call in the test byte-swaps
> the address, tswap32() in qtest.c does nothing as the host endian
> and guest endian are in sync and so the address reaches the
> controller region in the expected order.
>
> When on a BE host, the cpu_to_be32() does nothing but tswap32() in
> qtest.c reverses the order and so the address reaches the controller
> region in the wrong order (LE).
>
> I see two possible fixes :
>
> 1 - replace the read* routines by out* (no tswap there)

This would be wrong: "out*" is a request for an I/O access,
which is meaningless for ARM. (On x86 it's an actual outb/outw/outl
instruction.) In any case it wouldn't change the number of
byteswaps that happen:

 * "outl" qtest command calls cpu_outl(), which calls
   stl_p() on the value, to do "store in target endianness
   to buffer"; stl_p() will do a byteswap if the target and
   host endianness differ. The buffer is then passed to
   address_space_write(), which is a wrapper for
   address_space_rw(), which treats its buffer as a pile
   of bytes to be written.
 * "writel" qtest command does a tswap32s() on the data, which
   converts it to target endianness in a local variable.
   The address of the variable is then passed to
   cpu_physical_memory_write(), which is just a thin wrapper
   around address_space_rw(), which as above treats its
   buffer as a pile of bytes.

So the two things end up the same, it's just that for writel
we do the conversion from a host-ordered value to a
target-ordered value in a direct tswapl(), but for outl
this happens implicitly in cpu_outl()'s stl_p().

> 2 - add an extra byteswap on BE hosts using qtest_big_endian() as
>     in virtio-blk-test.c. see virtio_blk_fix_request()

What is the test actually supposed to be doing?
writel() says "write this 32 bit value as if the (guest) CPU
wrote it to memory with a 32-bit write instruction".

Your problem here is entirely on the test code side, I think.
It should be issuing the same commands down the qtest protocol
for any host endianness, but as it stands on a little endian
host it will issue "writel 0x20000000 0x600" whereas on a big
endian host it will issue "writel 0x20000000 0x60000".

(virtio is special because it has some weird ideas about
endianness due to being a virtual-only device with a spec
written by all-the-world-is-little-endian x86 people, so
it's a bad example to copy.)

thanks
-- PMM