Re: [PATCH 6/9] Introduce virtio-testdev

[Christoffer Dall <christoffer.dall@linaro.org>]

On Mon, Oct 14, 2013 at 06:23:32PM +0200, Andrew Jones wrote:
> virtio-testdev is a communication channel to qemu through virtio that
> can be used by test code to send commands. The only command currently
> implemented is EXIT, which allows the test code to exit with a given
> status code.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  lib/bswap.h          |   7 +++
>  lib/libcflat.h       |  10 ++++
>  lib/virtio-testdev.c | 126 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/virtio-testdev.h |   9 ++++
>  4 files changed, 152 insertions(+)
>  create mode 100644 lib/bswap.h
>  create mode 100644 lib/virtio-testdev.c
>  create mode 100644 lib/virtio-testdev.h
> 
> diff --git a/lib/bswap.h b/lib/bswap.h
> new file mode 100644
> index 0000000000000..e63c4d37a8b9a
> --- /dev/null
> +++ b/lib/bswap.h
> @@ -0,0 +1,7 @@
> +#ifndef _BSWAP_H_
> +#define _BSWAP_H_
> +#define le32_to_cpu(x) (x)
> +#define cpu_to_le32(x) (x)
> +#define le16_to_cpu(x) (x)
> +#define cpu_to_le16(x) (x)
> +#endif
> diff --git a/lib/libcflat.h b/lib/libcflat.h
> index a1be635ab4ee9..41791194657d0 100644
> --- a/lib/libcflat.h
> +++ b/lib/libcflat.h
> @@ -37,6 +37,16 @@ typedef _Bool bool;
>  #define true 1
>  #define false 0
>  
> +typedef u32 compat_ptr_t;
> +static inline void *compat_ptr(compat_ptr_t ptr)
> +{
> +    return (void *)(unsigned long)ptr;
> +}
> +static inline compat_ptr_t ptr_to_compat(void *ptr)
> +{
> +    return (u32)(unsigned long)ptr;
> +}
> +
>  extern void halt(int code);
>  extern void exit(int code);
>  
> diff --git a/lib/virtio-testdev.c b/lib/virtio-testdev.c
> new file mode 100644
> index 0000000000000..900921302c344
> --- /dev/null
> +++ b/lib/virtio-testdev.c

There's an interesting mix of space and tab indentations in this file...

I assume using the kernel coding style would be approprate here...
Actually, that could be added to the readme too. (Or whichever format we
choose).

Could you fix?

> @@ -0,0 +1,126 @@
> +#include "libcflat.h"
> +#include "iomaps.h"
> +#include "bswap.h"
> +
> +#define TESTDEV_MAJOR_VER 1
> +#define TESTDEV_MINOR_VER 1
> +
> +#define VIRTIO_MMIO_DEVICEID 0x8
> +#define VIRTIO_ID_TESTDEV 0xffff
> +#define DEV_CONFIG_BASE 0x100
> +#define DEV_CONFIG_SIZE 0x100
> +
> +enum { VERSION = 1, CLEAR, EXIT, };
> +enum { TOKEN, NARGS, NRETS, ARG1, ARG2, ARG3, ARG4, };
> +
> +#define RET1(nargs) (ARG1 + (nargs) + 0)
> +#define RET2(nargs) (ARG1 + (nargs) + 1)
> +#define RET3(nargs) (ARG1 + (nargs) + 2)
> +#define RET4(nargs) (ARG1 + (nargs) + 3)
> +
> +static u32 *testdev_base;
> +
> +/*
> + * We have to write all args; nargs, nrets, ... first to
> + * avoid executing token's operation until all args are in
> + * place. Then issue the op, and then read the rets. Reading
> + * the rets (or just sanity checking by reading base[0]) will
> + * read a zero into qemu's copy of the token, which allows us
> + * to prepare additional ops without re-executing the last one.
> + */
> +void virtio_testdev(u32 token, u32 nargs, u32 nrets, ...)
> +{
> +    volatile u32 *base = testdev_base;
> +    volatile u32 *tdp = base + 1;
> +    va_list va;
> +
> +    if (!testdev_base)
> +	return;
> +
> +    *tdp++ = cpu_to_le32(nargs);
> +    *tdp++ = cpu_to_le32(nrets);
> +
> +    va_start(va, nrets);
> +
> +    while (nargs--)
> +	*tdp++ = cpu_to_le32(va_arg(va, unsigned));
> +
> +    /* this runs the op, but then resets base[0] to zero */
> +    base[0] = cpu_to_le32(token);
> +
> +    /* sanity check */
> +    if (base[0] != 0) {
> +	printf("virtio-testdev base[0] should always be zero!\n");
> +	halt(EIO);
> +    }
> +
> +    while (nrets--) {
> +	u32 *r = va_arg(va, unsigned *);
> +	*r = le32_to_cpu(*tdp++);
> +    }
> +
> +    va_end(va);
> +}
> +
> +void virtio_testdev_version(u32 *version)
> +{
> +    virtio_testdev(VERSION, 0, 1, version);
> +}
> +
> +void virtio_testdev_clear(void)
> +{
> +    virtio_testdev(CLEAR, 0, 0);
> +}
> +
> +void virtio_testdev_exit(int code)
> +{
> +    virtio_testdev(EXIT, 1, 0, code);
> +}
> +
> +void virtio_testdev_init(void)
> +{
> +    struct iomap *m;
> +    u32 version, i;
> +    u16 major, minor;
> +
> +    m = iomaps_find("virtio_mmio");
> +    if (!m) {
> +	printf("No virtio-mmio transports for virtio-testdev found!\n");
> +	halt(ENODEV);
> +    }
> +
> +    for (i = 0; i < m->nr; ++i) {
> +	volatile u32 *base = (u32 *)compat_ptr(m->addrs[i]);
> +	u32 devid32 = base[VIRTIO_MMIO_DEVICEID / 4];

do we have readl/writel in this framework?  If not, maybe we should to
indicate IO read/writes and ensure the compiler doesn't reorder things
and that we have the necessary memory barriers etc...?

That would apply to virtio_testdev above as well.

> +	u16 devid = devid32 & 0xffff;
> +	devid = le16_to_cpu(devid);
> +	if (devid == VIRTIO_ID_TESTDEV) {
> +	    testdev_base = (u32 *)compat_ptr(m->addrs[i] + DEV_CONFIG_BASE);
> +	    break;
> +	}
> +    }
> +
> +    if (!testdev_base) {
> +	printf("Can't find virtio-testdev. Is '-device virtio-testdev' "
> +	       "on the qemu command line?\n");
> +	halt(ENODEV);
> +    }
> +
> +    virtio_testdev_version(&version);
> +    major = version >> 16;
> +    minor = version & 0xffff;
> +
> +    if (major != TESTDEV_MAJOR_VER || minor < TESTDEV_MINOR_VER) {
> +	char *u = "qemu";
> +	if (major > TESTDEV_MAJOR_VER)
> +	    u = "kvm-unit-tests";
> +	printf("Refusing to run with virtio-testdev major = %d, minor = %d\n",
> +			major, minor);

"Refusing to run"?
How about "incompatible version of virtio-testdev"?

> +	printf("Update %s.\n", u);
> +	exit(ENODEV);
> +    }
> +
> +    if (minor > TESTDEV_MINOR_VER)
> +	printf("testdev has new features. "
> +	       "An update of kvm-unit-tests may be possible.\n");
> +}
> diff --git a/lib/virtio-testdev.h b/lib/virtio-testdev.h
> new file mode 100644
> index 0000000000000..c10bbfb591d0b
> --- /dev/null
> +++ b/lib/virtio-testdev.h
> @@ -0,0 +1,9 @@
> +#ifndef _VIRTIO_TESTDEV_H_
> +#define _VIRTIO_TESTDEV_H_
> +#include "libcflat.h"
> +
> +extern void virtio_testdev_init(void);
> +extern void virtio_testdev_version(u32 *version);
> +extern void virtio_testdev_clear(void);
> +extern void virtio_testdev_exit(int code);
> +#endif
> -- 
> 1.8.1.4
>