Re: [Qemu-devel] [PATCH v6 3/3] tests: add RTAS command in the protocol

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 14715 bytes --]

On Mon, Sep 12, 2016 at 07:09:37PM +0200, Laurent Vivier wrote:
> 
> 
> On 12/09/2016 06:17, David Gibson wrote:
> > On Thu, Sep 08, 2016 at 09:00:07PM +0200, Laurent Vivier wrote:
> >> Add a first test to validate the protocol:
> >>
> >> - rtas/get-time-of-day compares the time
> >>   from the guest with the time from the host.
> >>
> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> >> ---
> >> v6:
> >> - rebase
> >>
> >> v5:
> >> - use qtest_spapr_boot() instead of machine_alloc_init()
> >>
> >> v4:
> >> - use qemu_strtoXXX() instead strtoXX()
> >>
> >> v3:
> >> - use mktimegm() instead of timegm()
> >>
> >> v2:
> >> - add a missing space in qrtas_call() prototype
> >>
> >>  hw/ppc/spapr_rtas.c         | 19 ++++++++++++
> >>  include/hw/ppc/spapr_rtas.h | 10 +++++++
> >>  qtest.c                     | 17 +++++++++++
> >>  tests/Makefile.include      |  3 ++
> >>  tests/libqos/rtas.c         | 71 +++++++++++++++++++++++++++++++++++++++++++++
> >>  tests/libqos/rtas.h         | 11 +++++++
> >>  tests/libqtest.c            | 10 +++++++
> >>  tests/libqtest.h            | 15 ++++++++++
> >>  tests/rtas-test.c           | 40 +++++++++++++++++++++++++
> >>  9 files changed, 196 insertions(+)
> >>  create mode 100644 include/hw/ppc/spapr_rtas.h
> >>  create mode 100644 tests/libqos/rtas.c
> >>  create mode 100644 tests/libqos/rtas.h
> >>  create mode 100644 tests/rtas-test.c
> >>
> >> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> >> index 27b5ad4..b80c1db 100644
> >> --- a/hw/ppc/spapr_rtas.c
> >> +++ b/hw/ppc/spapr_rtas.c
> >> @@ -37,6 +37,7 @@
> >>  
> >>  #include "hw/ppc/spapr.h"
> >>  #include "hw/ppc/spapr_vio.h"
> >> +#include "hw/ppc/spapr_rtas.h"
> >>  #include "hw/ppc/ppc.h"
> >>  #include "qapi-event.h"
> >>  #include "hw/boards.h"
> >> @@ -692,6 +693,24 @@ target_ulong spapr_rtas_call(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >>      return H_PARAMETER;
> >>  }
> >>  
> >> +uint64_t qtest_rtas_call(char *cmd, uint32_t nargs, uint64_t args,
> >> +                         uint32_t nret, uint64_t rets)
> >> +{
> >> +    int token;
> >> +
> >> +    for (token = 0; token < RTAS_TOKEN_MAX - RTAS_TOKEN_BASE; token++) {
> >> +        if (strcmp(cmd, rtas_table[token].name) == 0) {
> >> +            sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> >> +            PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
> >> +
> >> +            rtas_table[token].fn(cpu, spapr, token + RTAS_TOKEN_BASE,
> >> +                                 nargs, args, nret, rets);
> >> +            return H_SUCCESS;
> >> +        }
> >> +    }
> >> +    return H_PARAMETER;
> >> +}
> > 
> > I may be misunderstanding how qtest works here.  But wouldn't it test
> 
> >From the point of view of the machine, qtest is an accelerator: so the
> CPU is stopped, and the qtest accelerator open a socket allowing another
> process to read/write/in/out/... the memory of the machine.
> This allows to test the machine hardware or the hypervisor, not the CPU
> or the firmware. kvm-unit-tests is better for this kind of test.

Ah!  Yes, I was misunderstanding.  So the qtest script is basically
replacing the guest's cpu.

> The qtest protocol is human readable: the second process sends strings
> like "read 0x1000" and the qtest accelerator answers something like "OK
> 0" or "ERROR". This is why, from my point of view, using the name of the
> service rather than the token number seems better.

I see your point.

> > a bit more of the common code paths if rather than doing your own
> > token lookup here, you actually generated a full guest style rtas
> > parameter buffer (token + args + rets in one structure), then dispatch
> 
> We can't do the token lookup at the level of the second process as the
> token is extracted from the OF device tree and for the moment we don't
> have the functions in this process to scan the OF device tree. It is
> possible as we can read the guest memory, but the functions are not
> here. I plan to add this kind of feature, for instance to read the
> memory size, but for the moment nothing is done.

Right. Possible, but fiddly.  Leaving it until later seems fair
enough.

> > through h_rtas, or spapr_rtas_call?
> > 
> >> +
> >>  void spapr_rtas_register(int token, const char *name, spapr_rtas_fn fn)
> >>  {
> >>      assert((token >= RTAS_TOKEN_BASE) && (token < RTAS_TOKEN_MAX));
> >> diff --git a/include/hw/ppc/spapr_rtas.h b/include/hw/ppc/spapr_rtas.h
> >> new file mode 100644
> >> index 0000000..383611f
> >> --- /dev/null
> >> +++ b/include/hw/ppc/spapr_rtas.h
> >> @@ -0,0 +1,10 @@
> >> +#ifndef HW_SPAPR_RTAS_H
> >> +#define HW_SPAPR_RTAS_H
> >> +/*
> >> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> >> + * See the COPYING file in the top-level directory.
> >> + */
> >> +
> >> +uint64_t qtest_rtas_call(char *cmd, uint32_t nargs, uint64_t args,
> >> +                         uint32_t nret, uint64_t rets);
> >> +#endif /* HW_SPAPR_RTAS_H */
> >> diff --git a/qtest.c b/qtest.c
> >> index 4c94708..beb62b4 100644
> >> --- a/qtest.c
> >> +++ b/qtest.c
> >> @@ -28,6 +28,9 @@
> >>  #include "qemu/option.h"
> >>  #include "qemu/error-report.h"
> >>  #include "qemu/cutils.h"
> >> +#ifdef TARGET_PPC64
> >> +#include "hw/ppc/spapr_rtas.h"
> >> +#endif
> >>  
> >>  #define MAX_IRQ 256
> >>  
> >> @@ -531,6 +534,20 @@ static void qtest_process_command(CharDriverState *chr, gchar **words)
> >>  
> >>          qtest_send_prefix(chr);
> >>          qtest_send(chr, "OK\n");
> >> +#ifdef TARGET_PPC64
> >> +    } else if (strcmp(words[0], "rtas") == 0) {
> >> +        uint64_t res, args, ret;
> >> +        unsigned long nargs, nret;
> >> +
> >> +        g_assert(qemu_strtoul(words[2], NULL, 0, &nargs) == 0);
> >> +        g_assert(qemu_strtoull(words[3], NULL, 0, &args) == 0);
> >> +        g_assert(qemu_strtoul(words[4], NULL, 0, &nret) == 0);
> >> +        g_assert(qemu_strtoull(words[5], NULL, 0, &ret) == 0);
> >> +        res = qtest_rtas_call(words[1], nargs, args, nret, ret);
> >> +
> >> +        qtest_send_prefix(chr);
> >> +        qtest_sendf(chr, "OK %"PRIu64"\n", res);
> >> +#endif
> >>      } else if (qtest_enabled() && strcmp(words[0], "clock_step") == 0) {
> >>          int64_t ns;
> >>  
> >> diff --git a/tests/Makefile.include b/tests/Makefile.include
> >> index 91df9f2..fd61f97 100644
> >> --- a/tests/Makefile.include
> >> +++ b/tests/Makefile.include
> >> @@ -265,6 +265,7 @@ check-qtest-ppc64-y += tests/prom-env-test$(EXESUF)
> >>  check-qtest-ppc64-y += tests/drive_del-test$(EXESUF)
> >>  check-qtest-ppc64-y += tests/postcopy-test$(EXESUF)
> >>  check-qtest-ppc64-y += tests/boot-serial-test$(EXESUF)
> >> +check-qtest-ppc64-y += tests/rtas-test$(EXESUF)
> >>  
> >>  check-qtest-sh4-y = tests/endianness-test$(EXESUF)
> >>  
> >> @@ -578,6 +579,7 @@ libqos-obj-y = tests/libqos/pci.o tests/libqos/fw_cfg.o tests/libqos/malloc.o
> >>  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-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
> >> @@ -592,6 +594,7 @@ tests/m48t59-test$(EXESUF): tests/m48t59-test.o
> >>  tests/endianness-test$(EXESUF): tests/endianness-test.o
> >>  tests/spapr-phb-test$(EXESUF): tests/spapr-phb-test.o $(libqos-obj-y)
> >>  tests/prom-env-test$(EXESUF): tests/prom-env-test.o $(libqos-obj-y)
> >> +tests/rtas-test$(EXESUF): tests/rtas-test.o $(libqos-spapr-obj-y)
> >>  tests/fdc-test$(EXESUF): tests/fdc-test.o
> >>  tests/ide-test$(EXESUF): tests/ide-test.o $(libqos-pc-obj-y)
> >>  tests/ahci-test$(EXESUF): tests/ahci-test.o $(libqos-pc-obj-y)
> >> diff --git a/tests/libqos/rtas.c b/tests/libqos/rtas.c
> >> new file mode 100644
> >> index 0000000..d5f4ced
> >> --- /dev/null
> >> +++ b/tests/libqos/rtas.c
> >> @@ -0,0 +1,71 @@
> >> +/*
> >> + * 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/rtas.h"
> >> +
> >> +static void qrtas_copy_args(uint64_t target_args, uint32_t nargs,
> >> +                            uint32_t *args)
> >> +{
> >> +    int i;
> >> +
> >> +    for (i = 0; i < nargs; i++) {
> >> +        writel(target_args + i * sizeof(uint32_t), args[i]);
> >> +    }
> >> +}
> >> +
> >> +static void qrtas_copy_ret(uint64_t target_ret, uint32_t nret, uint32_t *ret)
> >> +{
> >> +    int i;
> >> +
> >> +    for (i = 0; i < nret; i++) {
> >> +        ret[i] = readl(target_ret + i * sizeof(uint32_t));
> >> +    }
> >> +}
> >> +
> >> +static uint64_t qrtas_call(QGuestAllocator *alloc, const char *name,
> >> +                           uint32_t nargs, uint32_t *args,
> >> +                           uint32_t nret, uint32_t *ret)
> >> +{
> >> +    uint64_t res;
> >> +    uint64_t target_args, target_ret;
> >> +
> >> +    target_args = guest_alloc(alloc, (nargs + nret) * sizeof(uint32_t));
> >> +    target_ret = guest_alloc(alloc, nret * sizeof(uint32_t));
> > 
> > Again, possibly I'm misunderstanding something, but it looks like
> > you're over-allocating here - target_args seems to get enough space
> > for both args and rets, then target_ret gets additional space for the
> > rets as well.
> 
> You're right.
> 
> >> +    qrtas_copy_args(target_args, nargs, args);
> >> +    res = qtest_rtas_call(global_qtest, name,
> >> +                          nargs, target_args, nret, target_ret);
> >> +    qrtas_copy_ret(target_ret, nret, ret);
> >> +
> >> +    guest_free(alloc, target_ret);
> >> +    guest_free(alloc, target_args);
> >> +
> >> +    return res;
> >> +}
> >> +
> >> +int qrtas_get_time_of_day(QGuestAllocator *alloc, struct tm *tm, uint32_t *ns)
> >> +{
> >> +    int res;
> >> +    uint32_t ret[8];
> >> +
> >> +    res = qrtas_call(alloc, "get-time-of-day", 0, NULL, 8, ret);
> >> +    if (res != 0) {
> >> +        return res;
> >> +    }
> >> +
> >> +    res = ret[0];
> >> +    memset(tm, 0, sizeof(*tm));
> >> +    tm->tm_year = ret[1] - 1900;
> >> +    tm->tm_mon = ret[2] - 1;
> >> +    tm->tm_mday = ret[3];
> >> +    tm->tm_hour = ret[4];
> >> +    tm->tm_min = ret[5];
> >> +    tm->tm_sec = ret[6];
> >> +    *ns = ret[7];
> >> +
> >> +    return res;
> >> +}
> >> diff --git a/tests/libqos/rtas.h b/tests/libqos/rtas.h
> >> new file mode 100644
> >> index 0000000..a1b60a8
> >> --- /dev/null
> >> +++ b/tests/libqos/rtas.h
> >> @@ -0,0 +1,11 @@
> >> +/*
> >> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> >> + * See the COPYING file in the top-level directory.
> >> + */
> >> +
> >> +#ifndef LIBQOS_RTAS_H
> >> +#define LIBQOS_RTAS_H
> >> +#include "libqos/malloc.h"
> >> +
> >> +int qrtas_get_time_of_day(QGuestAllocator *alloc, struct tm *tm, uint32_t *ns);
> >> +#endif /* LIBQOS_RTAS_H */
> >> diff --git a/tests/libqtest.c b/tests/libqtest.c
> >> index eb00f13..c9dd57b 100644
> >> --- a/tests/libqtest.c
> >> +++ b/tests/libqtest.c
> >> @@ -751,6 +751,16 @@ void qtest_memread(QTestState *s, uint64_t addr, void *data, size_t size)
> >>      g_strfreev(args);
> >>  }
> >>  
> >> +uint64_t qtest_rtas_call(QTestState *s, const char *name,
> >> +                         uint32_t nargs, uint64_t args,
> >> +                         uint32_t nret, uint64_t ret)
> >> +{
> >> +    qtest_sendf(s, "rtas %s %u 0x%"PRIx64" %u 0x%"PRIx64"\n",
> >> +                name, nargs, args, nret, ret);
> >> +    qtest_rsp(s, 0);
> >> +    return 0;
> >> +}
> >> +
> >>  void qtest_add_func(const char *str, void (*fn)(void))
> >>  {
> >>      gchar *path = g_strdup_printf("/%s/%s", qtest_get_arch(), str);
> >> diff --git a/tests/libqtest.h b/tests/libqtest.h
> >> index 37f37ad..1badb76 100644
> >> --- a/tests/libqtest.h
> >> +++ b/tests/libqtest.h
> >> @@ -318,6 +318,21 @@ uint64_t qtest_readq(QTestState *s, uint64_t addr);
> >>  void qtest_memread(QTestState *s, uint64_t addr, void *data, size_t size);
> >>  
> >>  /**
> >> + * qtest_rtas_call:
> >> + * @s: #QTestState instance to operate on.
> >> + * @name: name of the command to call.
> >> + * @nargs: Number of args.
> >> + * @args: Guest address to read args from.
> >> + * @nret: Number of return value.
> >> + * @ret: Guest address to write return values to.
> >> + *
> >> + * Call an RTAS function
> >> + */
> >> +uint64_t qtest_rtas_call(QTestState *s, const char *name,
> >> +                         uint32_t nargs, uint64_t args,
> >> +                         uint32_t nret, uint64_t ret);
> >> +
> >> +/**
> >>   * qtest_bufread:
> >>   * @s: #QTestState instance to operate on.
> >>   * @addr: Guest address to read from.
> >> diff --git a/tests/rtas-test.c b/tests/rtas-test.c
> >> new file mode 100644
> >> index 0000000..3bca36b
> >> --- /dev/null
> >> +++ b/tests/rtas-test.c
> >> @@ -0,0 +1,40 @@
> >> +#include "qemu/osdep.h"
> >> +#include "qemu/cutils.h"
> >> +#include "libqtest.h"
> >> +
> >> +#include "libqos/libqos-spapr.h"
> >> +#include "libqos/rtas.h"
> >> +
> >> +static void test_rtas_get_time_of_day(void)
> >> +{
> >> +    QOSState *qs;
> >> +    struct tm tm;
> >> +    uint32_t ns;
> >> +    uint64_t ret;
> >> +    time_t t1, t2;
> >> +
> >> +    qs = qtest_spapr_boot("");
> >> +
> >> +    t1 = time(NULL);
> >> +    ret = qrtas_get_time_of_day(qs->alloc, &tm, &ns);
> >> +    g_assert_cmpint(ret, ==, 0);
> >> +    t2 = mktimegm(&tm);
> >> +    g_assert(t2 - t1 < 5); /* 5 sec max to run the test */
> >> +
> >> +    qtest_spapr_shutdown(qs);
> >> +}
> >> +
> >> +int main(int argc, char *argv[])
> >> +{
> >> +    const char *arch = qtest_get_arch();
> >> +
> >> +    g_test_init(&argc, &argv, NULL);
> >> +
> >> +    if (strcmp(arch, "ppc64") == 0) {
> >> +        qtest_add_func("rtas/get-time-of-day", test_rtas_get_time_of_day);
> >> +    } else {
> >> +        g_assert_not_reached();
> >> +    }
> >> +
> >> +    return g_test_run();
> >> +}
> > 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]