On 10/03/2016 04:03 PM, Greg Kurz wrote:
> On Mon, 3 Oct 2016 13:23:27 +0200
> Cédric Le Goater <c...@kaod.org> wrote:
>
>> On 09/29/2016 07:27 AM, David Gibson wrote:
>>> On Wed, Sep 28, 2016 at 08:51:28PM +0200, Laurent Vivier wrote:
>>>> Signed-off-by: Laurent Vivier <lviv...@redhat.com>
>>>> ---
>>>> tests/Makefile.include | 1 +
>>>> tests/libqos/pci-pc.c | 22 ----
>>>> tests/libqos/pci-spapr.c | 280
>>>> +++++++++++++++++++++++++++++++++++++++++++++++
>>>> tests/libqos/pci-spapr.h | 17 +++
>>>> tests/libqos/pci.c | 22 +++-
>>>> tests/libqos/rtas.c | 45 ++++++++
>>>> tests/libqos/rtas.h | 4 +
>>>> 7 files changed, 368 insertions(+), 23 deletions(-)
>>>> create mode 100644 tests/libqos/pci-spapr.c
>>>> create mode 100644 tests/libqos/pci-spapr.h
>>>>
>>>> diff --git a/tests/Makefile.include b/tests/Makefile.include
>>>> index 8162f6f..92c82d8 100644
>>>> --- a/tests/Makefile.include
>>>> +++ b/tests/Makefile.include
>>>> @@ -590,6 +590,7 @@ libqos-obj-y += tests/libqos/i2c.o
>>>> tests/libqos/libqos.o
>>>> libqos-spapr-obj-y = $(libqos-obj-y) tests/libqos/malloc-spapr.o
>>>> libqos-spapr-obj-y += tests/libqos/libqos-spapr.o
>>>> libqos-spapr-obj-y += tests/libqos/rtas.o
>>>> +libqos-spapr-obj-y += tests/libqos/pci-spapr.o
>>>> libqos-pc-obj-y = $(libqos-obj-y) tests/libqos/pci-pc.o
>>>> libqos-pc-obj-y += tests/libqos/malloc-pc.o tests/libqos/libqos-pc.o
>>>> libqos-pc-obj-y += tests/libqos/ahci.o
>>>> diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
>>>> index 1ae2d37..82066b8 100644
>>>> --- a/tests/libqos/pci-pc.c
>>>> +++ b/tests/libqos/pci-pc.c
>>>> @@ -255,28 +255,6 @@ void qpci_free_pc(QPCIBus *bus)
>>>> g_free(s);
>>>> }
>>>>
>>>> -void qpci_plug_device_test(const char *driver, const char *id,
>>>> - uint8_t slot, const char *opts)
>>>> -{
>>>> - QDict *response;
>>>> - char *cmd;
>>>> -
>>>> - cmd = g_strdup_printf("{'execute': 'device_add',"
>>>> - " 'arguments': {"
>>>> - " 'driver': '%s',"
>>>> - " 'addr': '%d',"
>>>> - " %s%s"
>>>> - " 'id': '%s'"
>>>> - "}}", driver, slot,
>>>> - opts ? opts : "", opts ? "," : "",
>>>> - id);
>>>> - response = qmp(cmd);
>>>> - g_free(cmd);
>>>> - g_assert(response);
>>>> - g_assert(!qdict_haskey(response, "error"));
>>>> - QDECREF(response);
>>>> -}
>>>> -
>>>> void qpci_unplug_acpi_device_test(const char *id, uint8_t slot)
>>>> {
>>>> QDict *response;
>>>> diff --git a/tests/libqos/pci-spapr.c b/tests/libqos/pci-spapr.c
>>>> new file mode 100644
>>>> index 0000000..78df823
>>>> --- /dev/null
>>>> +++ b/tests/libqos/pci-spapr.c
>>>> @@ -0,0 +1,280 @@
>>>> +/*
>>>> + * libqos PCI bindings for SPAPR
>>>> + *
>>>> + * This work is licensed under the terms of the GNU GPL, version 2 or
>>>> later.
>>>> + * See the COPYING file in the top-level directory.
>>>> + */
>>>> +
>>>> +#include "qemu/osdep.h"
>>>> +#include "libqtest.h"
>>>> +#include "libqos/pci-spapr.h"
>>>> +#include "libqos/rtas.h"
>>>> +
>>>> +#include "hw/pci/pci_regs.h"
>>>> +
>>>> +#include "qemu-common.h"
>>>> +#include "qemu/host-utils.h"
>>>> +
>>>> +
>>>> +/* From include/hw/pci-host/spapr.h */
>>>> +
>>>> +#define SPAPR_PCI_BASE_BUID 0x800000020000000ULL
>>>> +
>>>> +#define SPAPR_PCI_MEM_WIN_BUS_OFFSET 0x80000000ULL
>>>> +
>>>> +#define SPAPR_PCI_WINDOW_BASE 0x10000000000ULL
>>>> +#define SPAPR_PCI_WINDOW_SPACING 0x1000000000ULL
>>>> +#define SPAPR_PCI_MMIO_WIN_OFF 0xA0000000
>>>> +#define SPAPR_PCI_MMIO_WIN_SIZE (SPAPR_PCI_WINDOW_SPACING - \
>>>> + SPAPR_PCI_MEM_WIN_BUS_OFFSET)
>>>> +#define SPAPR_PCI_IO_WIN_OFF 0x80000000
>>>> +#define SPAPR_PCI_IO_WIN_SIZE 0x10000
>>>> +
>>>> +/* index is the phb index */
>>>> +
>>>> +#define BUIDBASE(index) (SPAPR_PCI_BASE_BUID + (index))
>>>> +#define PCIBASE(index) (SPAPR_PCI_WINDOW_BASE + \
>>>> + (index) * SPAPR_PCI_WINDOW_SPACING)
>>>> +#define IOBASE(index) (PCIBASE(index) +
>>>> SPAPR_PCI_IO_WIN_OFF)
>>>> +#define MMIOBASE(index) (PCIBASE(index) +
>>>> SPAPR_PCI_MMIO_WIN_OFF)
>>>> +
>>>> +typedef struct QPCIBusSPAPR {
>>>> + QPCIBus bus;
>>>> + QGuestAllocator *alloc;
>>>> +
>>>> + uint64_t pci_hole_start;
>>>> + uint64_t pci_hole_size;
>>>> + uint64_t pci_hole_alloc;
>>>> +
>>>> + uint32_t pci_iohole_start;
>>>> + uint32_t pci_iohole_size;
>>>> + uint32_t pci_iohole_alloc;
>>>> +} QPCIBusSPAPR;
>>>> +
>>>> +static uint8_t qpci_spapr_io_readb(QPCIBus *bus, void *addr)
>>>> +{
>>>> + uint64_t port = (uint64_t)addr;
>>>> + uint8_t v;
>>>> + if (port < SPAPR_PCI_IO_WIN_SIZE) {
>>>> + v = readb(IOBASE(0) + port);
>>>> + } else {
>>>> + v = readb(MMIOBASE(0) + port);
>>>> + }
>>>> + return v;
>>>> +}
>>>> +
>>>> +static uint16_t qpci_spapr_io_readw(QPCIBus *bus, void *addr)
>>>> +{
>>>> + uint64_t port = (uint64_t)addr;
>>>> + uint16_t v;
>>>> + if (port < SPAPR_PCI_IO_WIN_SIZE) {
>>>> + v = readw(IOBASE(0) + port);
>>>> + } else {
>>>> + v = readw(MMIOBASE(0) + port);
>>>> + }
>>>
>>> Ok, so, I've looked through the qtest stuff in more detail now, and
>>> I've got a better idea of how the endianness works. Some of my
>>> earlier comments were confused about which pieces were in the test
>>> case side and which were on the qtest accel stub side.
>>>
>>> So, what I think you need is an unconditional byteswap in each of the
>>> spapr pci IO functions. Why?
>>>
>>> * The plain readw() etc. functions return values read from memory
>>> assuming guest endianness. For guest native values in memory, so
>>> far so good.
>>> * Generic users of the pci read/write functions in qtest expect to
>>> get native values.
>>> * But PCI devices are always LE, not guest endian
>>>
>>> The guest endianness of spapr (as far as tswap() etc. are concerned)
>>> is BE, even though that's not really true in practice these days, so
>>> to correct for the PCI registers being read as BE when they should be
>>> LE we always need a swap.
>>>
>>> That should remove the need for swaps further up the test stack.
>>>
>>> Of course, "guest endianness" is a poorly defined concept, so longer
>>> term it might be better to completely replace the "readw" etc. qtest
>>> operations with explicit "readw_le" and "readw_be" ops.
>>
>>
>> I have a similar need for a unit test of the AST2400 flash controller.
>>
>> The flash module is accessible through a memory window and, when in
>> SPI mode, the commands are sent to the slave by writing at the beginning
>> of the region. Addresses are necessarily BE to be understood by the SPI
>> flash module. So, sequences like
>>
>> writeb(AST2400_FLASH_BASE, EN_4BYTE_ADDR);
>> writeb(AST2400_FLASH_BASE, READ);
>> writel(AST2400_FLASH_BASE, cpu_to_be32(addr));
>>
>> will just fail under a BE host as an extra swap is done by the qtest
>
> cpu_to_be32() is indeed what a real guest does... but the real guest runs
> with the target endianness, whereas the qtest program runs with the host
> endianness... so cpu_to_be32() looks wrong here.
>
>> layer. I have used memwrite to have endian-agnostic mem accessors.
>> Maybe something like below would be helpful in libqtest.h
>>
>>
>> +/*
>> + * Generic read/write helpers for a BE region
>> + */
>> +static inline void writew_be(uint64_t addr, uint16_t value)
>> +{
>> + value = cpu_to_be16(value);
>> + memwrite(addr, &value, sizeof(value));
>> +}
>> +
>> +static inline uint16_t readw_be(uint64_t addr)
>> +{
>> + uint16_t value;
>> +
>> + memread(addr, &value, sizeof(value));
>> + return be16_to_cpu(value);
>> +}
>> +
>> +static inline void writel_be(uint64_t addr, uint32_t value)
>> +{
>> + value = cpu_to_be32(value);
>> + memwrite(addr, &value, sizeof(value));
>> +}
>> +
>> +static inline uint32_t readl_be(uint64_t addr)
>> +{
>> + uint32_t value;
>> +
>> + memread(addr, &value, sizeof(value));
>> + return be32_to_cpu(value);
>> +}
>>
>> If this is correct, I can add the LE equivalents and send a patch.
>>
>
> The API is correct, but the implementation has the same issue: it uses
> cpu_to_be32() and bypasses the tswap32() in qtest... the result is that
> the target endianness is totally ignored.
>
> Let's suppose that a similar AST2400 platform exists with a BE processor:
> would the same test program work for both LE and BE cases ?
>
> Both the test program and QEMU run with the host endianness acutally,
> but only QEMU knows about the target endianness. So I guess, the qtest
> protocol should provide explicit BE/LE operations: the test program
> would ask for an access with a specific endianness and the qtest
> accelerator in QEMU would do the swap based on the target endianness.
Here is a RFC below, tested on a BE and LE host for a BE region.
Thanks,
C.
From: Cédric Le Goater <c...@kaod.org>
Subject: [PATCH] qtest: add an endianness parameter to read/write accessors
Date: Mon, 03 Oct 2016 16:52:48 +0200
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This adds {read,write}{w,l,q}_{be,le} routines to access guest memory
using a specific endianness. The protocol is simply extented to pass
"be", "le", "native".
Signed-off-by: Cédric Le Goater <c...@kaod.org>
---
qtest.c | 73 +++++++++++++++++++++++++++++++++++++++++-----
tests/libqtest.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++--------
tests/libqtest.h | 68 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 207 insertions(+), 20 deletions(-)
Index: qemu-aspeed.git/tests/libqtest.h
===================================================================
--- qemu-aspeed.git.orig/tests/libqtest.h
+++ qemu-aspeed.git/tests/libqtest.h
@@ -888,4 +888,72 @@ void qmp_fd_send(int fd, const char *fmt
QDict *qmp_fdv(int fd, const char *fmt, va_list ap);
QDict *qmp_fd(int fd, const char *fmt, ...);
+uint16_t qtest_readw_be(QTestState *s, uint64_t addr);
+uint32_t qtest_readl_be(QTestState *s, uint64_t addr);
+uint64_t qtest_readq_be(QTestState *s, uint64_t addr);
+
+static inline uint16_t readw_be(uint64_t addr)
+{
+ return qtest_readw_be(global_qtest, addr);
+}
+static inline uint32_t readl_be(uint64_t addr)
+{
+ return qtest_readl_be(global_qtest, addr);
+}
+static inline uint64_t readq_be(uint64_t addr)
+{
+ return qtest_readq_be(global_qtest, addr);
+}
+
+void qtest_writew_be(QTestState *s, uint64_t addr, uint16_t value);
+void qtest_writel_be(QTestState *s, uint64_t addr, uint32_t value);
+void qtest_writeq_be(QTestState *s, uint64_t addr, uint64_t value);
+
+static inline void writew_be(uint64_t addr, uint16_t value)
+{
+ qtest_writew_be(global_qtest, addr, value);
+}
+static inline void writel_be(uint64_t addr, uint32_t value)
+{
+ qtest_writel_be(global_qtest, addr, value);
+}
+static inline void writeq_be(uint64_t addr, uint64_t value)
+{
+ qtest_writeq_be(global_qtest, addr, value);
+}
+
+uint16_t qtest_readw_le(QTestState *s, uint64_t addr);
+uint32_t qtest_readl_le(QTestState *s, uint64_t addr);
+uint64_t qtest_readq_le(QTestState *s, uint64_t addr);
+
+static inline uint16_t readw_le(uint64_t addr)
+{
+ return qtest_readw_le(global_qtest, addr);
+}
+static inline uint32_t readl_le(uint64_t addr)
+{
+ return qtest_readl_le(global_qtest, addr);
+}
+static inline uint64_t readq_le(uint64_t addr)
+{
+ return qtest_readq_le(global_qtest, addr);
+}
+
+void qtest_writew_le(QTestState *s, uint64_t addr, uint16_t value);
+void qtest_writel_le(QTestState *s, uint64_t addr, uint32_t value);
+void qtest_writeq_le(QTestState *s, uint64_t addr, uint64_t value);
+
+static inline void writew_le(uint64_t addr, uint16_t value)
+{
+ qtest_writew_le(global_qtest, addr, value);
+}
+static inline void writel_le(uint64_t addr, uint32_t value)
+{
+ qtest_writel_le(global_qtest, addr, value);
+}
+static inline void writeq_le(uint64_t addr, uint64_t value)
+{
+ qtest_writeq_le(global_qtest, addr, value);
+}
+
#endif
Index: qemu-aspeed.git/qtest.c
===================================================================
--- qemu-aspeed.git.orig/qtest.c
+++ qemu-aspeed.git/qtest.c
@@ -257,6 +257,63 @@ static void qtest_irq_handler(void *opaq
}
}
+static inline bool is_target_big_endian(void)
+{
+#if defined(TARGET_WORDS_BIGENDIAN)
+ return true;
+#else
+ return false;
+#endif
+}
+
+static void qtest_tswap16s(uint16_t *s, gchar *endianness)
+{
+ if (endianness[0] != 'n') {
+ bool big_endian = (endianness[0] == 'b');
+
+ if (is_target_big_endian() != big_endian) {
+ bswap16s(s);
+ }
+ }
+ tswap16s(s);
+}
+
+static uint16_t qtest_tswap16(uint16_t s, gchar *endianness)
+{
+ qtest_tswap16s(&s, endianness);
+ return s;
+}
+
+static void qtest_tswap32s(uint32_t *s, gchar *endianness)
+{
+ if (endianness[0] != 'n') {
+ bool big_endian = (endianness[0] == 'b');
+
+ if (is_target_big_endian() != big_endian) {
+ bswap32s(s);
+ }
+ }
+ tswap32s(s);
+}
+
+static uint32_t qtest_tswap32(uint32_t s, gchar *endianness)
+{
+ qtest_tswap32s(&s, endianness);
+ return s;
+}
+
+static void qtest_tswap64s(uint64_t *s, gchar *endianness)
+{
+ if (endianness[0] != 'n') {
+ bool big_endian = (endianness[0] == 'b');
+
+ if (is_target_big_endian() != big_endian) {
+ bswap64s(s);
+ }
+ }
+ tswap64s(s);
+}
+
static void qtest_process_command(CharDriverState *chr, gchar **words)
{
const gchar *command;
@@ -372,7 +429,7 @@ static void qtest_process_command(CharDr
uint64_t addr;
uint64_t value;
- g_assert(words[1] && words[2]);
+ g_assert(words[1] && words[2] && words[3]);
g_assert(qemu_strtoull(words[1], NULL, 0, &addr) == 0);
g_assert(qemu_strtoull(words[2], NULL, 0, &value) == 0);
@@ -381,15 +438,15 @@ static void qtest_process_command(CharDr
cpu_physical_memory_write(addr, &data, 1);
} else if (words[0][5] == 'w') {
uint16_t data = value;
- tswap16s(&data);
+ qtest_tswap16s(&data, words[3]);
cpu_physical_memory_write(addr, &data, 2);
} else if (words[0][5] == 'l') {
uint32_t data = value;
- tswap32s(&data);
+ qtest_tswap32s(&data, words[3]);
cpu_physical_memory_write(addr, &data, 4);
} else if (words[0][5] == 'q') {
uint64_t data = value;
- tswap64s(&data);
+ qtest_tswap64s(&data, words[3]);
cpu_physical_memory_write(addr, &data, 8);
}
qtest_send_prefix(chr);
@@ -401,7 +458,7 @@ static void qtest_process_command(CharDr
uint64_t addr;
uint64_t value = UINT64_C(-1);
- g_assert(words[1]);
+ g_assert(words[1] && words[2]);
g_assert(qemu_strtoull(words[1], NULL, 0, &addr) == 0);
if (words[0][4] == 'b') {
@@ -411,14 +468,14 @@ static void qtest_process_command(CharDr
} else if (words[0][4] == 'w') {
uint16_t data;
cpu_physical_memory_read(addr, &data, 2);
- value = tswap16(data);
+ value = qtest_tswap16(data, words[2]);
} else if (words[0][4] == 'l') {
uint32_t data;
cpu_physical_memory_read(addr, &data, 4);
- value = tswap32(data);
+ value = qtest_tswap32(data, words[2]);
} else if (words[0][4] == 'q') {
cpu_physical_memory_read(addr, &value, 8);
- tswap64s(&value);
+ qtest_tswap64s(&value, words[2]);
}
qtest_send_prefix(chr);
qtest_sendf(chr, "OK 0x%016" PRIx64 "\n", value);
Index: qemu-aspeed.git/tests/libqtest.c
===================================================================
--- qemu-aspeed.git.orig/tests/libqtest.c
+++ qemu-aspeed.git/tests/libqtest.c
@@ -663,38 +663,70 @@ uint32_t qtest_inl(QTestState *s, uint16
}
static void qtest_write(QTestState *s, const char *cmd, uint64_t addr,
- uint64_t value)
+ uint64_t value, const char *endianness)
{
- qtest_sendf(s, "%s 0x%" PRIx64 " 0x%" PRIx64 "\n", cmd, addr, value);
+ qtest_sendf(s, "%s 0x%" PRIx64 " 0x%" PRIx64 " %s\n", cmd, addr, value,
+ endianness);
qtest_rsp(s, 0);
}
void qtest_writeb(QTestState *s, uint64_t addr, uint8_t value)
{
- qtest_write(s, "writeb", addr, value);
+ qtest_write(s, "writeb", addr, value, "native");
}
void qtest_writew(QTestState *s, uint64_t addr, uint16_t value)
{
- qtest_write(s, "writew", addr, value);
+ qtest_write(s, "writew", addr, value, "native");
}
void qtest_writel(QTestState *s, uint64_t addr, uint32_t value)
{
- qtest_write(s, "writel", addr, value);
+ qtest_write(s, "writel", addr, value, "native");
}
void qtest_writeq(QTestState *s, uint64_t addr, uint64_t value)
{
- qtest_write(s, "writeq", addr, value);
+ qtest_write(s, "writeq", addr, value, "native");
}
-static uint64_t qtest_read(QTestState *s, const char *cmd, uint64_t addr)
+void qtest_writew_be(QTestState *s, uint64_t addr, uint16_t value)
+{
+ qtest_write(s, "writew", addr, value, "be");
+}
+
+void qtest_writel_be(QTestState *s, uint64_t addr, uint32_t value)
+{
+ qtest_write(s, "writel", addr, value, "be");
+}
+
+void qtest_writeq_be(QTestState *s, uint64_t addr, uint64_t value)
+{
+ qtest_write(s, "writeq", addr, value, "be");
+}
+
+void qtest_writew_le(QTestState *s, uint64_t addr, uint16_t value)
+{
+ qtest_write(s, "writew", addr, value, "le");
+}
+
+void qtest_writel_le(QTestState *s, uint64_t addr, uint32_t value)
+{
+ qtest_write(s, "writel", addr, value, "le");
+}
+
+void qtest_writeq_le(QTestState *s, uint64_t addr, uint64_t value)
+{
+ qtest_write(s, "writeq", addr, value, "le");
+}
+
+static uint64_t qtest_read(QTestState *s, const char *cmd, uint64_t addr,
+ const char *endianness)
{
gchar **args;
uint64_t value;
- qtest_sendf(s, "%s 0x%" PRIx64 "\n", cmd, addr);
+ qtest_sendf(s, "%s 0x%" PRIx64 " %s\n", cmd, addr, endianness);
args = qtest_rsp(s, 2);
value = strtoull(args[1], NULL, 0);
g_strfreev(args);
@@ -704,22 +736,52 @@ static uint64_t qtest_read(QTestState *s
uint8_t qtest_readb(QTestState *s, uint64_t addr)
{
- return qtest_read(s, "readb", addr);
+ return qtest_read(s, "readb", addr, "native");
}
uint16_t qtest_readw(QTestState *s, uint64_t addr)
{
- return qtest_read(s, "readw", addr);
+ return qtest_read(s, "readw", addr, "native");
}
uint32_t qtest_readl(QTestState *s, uint64_t addr)
{
- return qtest_read(s, "readl", addr);
+ return qtest_read(s, "readl", addr, "native");
}
uint64_t qtest_readq(QTestState *s, uint64_t addr)
{
- return qtest_read(s, "readq", addr);
+ return qtest_read(s, "readq", addr, "native");
+}
+
+uint16_t qtest_readw_be(QTestState *s, uint64_t addr)
+{
+ return qtest_read(s, "readw", addr, "be");
+}
+
+uint32_t qtest_readl_be(QTestState *s, uint64_t addr)
+{
+ return qtest_read(s, "readl", addr, "be");
+}
+
+uint64_t qtest_readq_be(QTestState *s, uint64_t addr)
+{
+ return qtest_read(s, "readq", addr, "be");
+}
+
+uint16_t qtest_readw_le(QTestState *s, uint64_t addr)
+{
+ return qtest_read(s, "readw", addr, "le");
+}
+
+uint32_t qtest_readl_le(QTestState *s, uint64_t addr)
+{
+ return qtest_read(s, "readl", addr, "le");
+}
+
+uint64_t qtest_readq_le(QTestState *s, uint64_t addr)
+{
+ return qtest_read(s, "readq", addr, "le");
}
static int hex2nib(char ch)
