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

[Cédric Le Goater]

On 07/04/2016 01:14 PM, Peter Maydell wrote:
> 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,

yes. that felt really wrong.

> 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().

ok. thanks for the summary/explanation. 

>> 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".

The test (and the guest) is writing and reading data on the memory 
region used by the SPI controller. This 'data' is then passed on 
to the SPI flash module objects which expects BE order when there
are flash storage addresses are in the flow. 

So I think the test needs to use mem{write,read} and not write*.
The result looks correct in the code. I will send a patchset 
right after starting with this patch.

> Your problem here is entirely on the test code side, I think.

As the guest boots correctly, I would say yes also.

> 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.)

yes, we had a taste of that when virtio support was added for
the ppc64be guests :)

Thanks,

C.