Re: [PATCH 12/17] Introduce virtio-testdev

[Andrew Jones <drjones@redhat.com>]

On Sat, Feb 01, 2014 at 06:27:37PM -0800, Christoffer Dall wrote:
> On Tue, Jan 21, 2014 at 05:21:58PM +0100, 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>
> > 
> > ---
> > v3:
> >  - switched from iomaps to devicetree
> >  - fixed Christoffer's nits
> >  - don't call halt() explicitly, let exit() fall back to it
> >  - vm_{get,set} now {read,write} byte by byte like the kernel
> > v2:
> >  - use readl/writel framework (libio) [Christoffer Dall]
> >  - keep the virtio abstraction in virtio-testdev [Alexander Graf]
> > ---
> >  lib/libcflat.h       |   3 ++
> >  lib/virtio-testdev.c | 139 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  lib/virtio-testdev.h |   9 ++++
> >  lib/virtio.c         | 126 ++++++++++++++++++++++++++++++++++++++++++++++
> >  lib/virtio.h         |  74 +++++++++++++++++++++++++++
> >  5 files changed, 351 insertions(+)
> >  create mode 100644 lib/virtio-testdev.c
> >  create mode 100644 lib/virtio-testdev.h
> >  create mode 100644 lib/virtio.c
> >  create mode 100644 lib/virtio.h
> > 
> > diff --git a/lib/libcflat.h b/lib/libcflat.h
> > index fdaaf2a8ab31d..84b8783bacfdc 100644
> > --- a/lib/libcflat.h
> > +++ b/lib/libcflat.h
> > @@ -60,6 +60,9 @@ extern long atol(const char *ptr);
> >  #define ARRAY_SIZE(_a)  (sizeof(_a)/sizeof((_a)[0]))
> >  
> >  #define offsetof(TYPE, MEMBER) __builtin_offsetof (TYPE, MEMBER)
> > +#define container_of(ptr, type, member) ({				\
> > +		const typeof( ((type *)0)->member ) *__mptr = (ptr);	\
> > +		(type *)( (char *)__mptr - offsetof(type,member) );})
> >  
> >  #define __unused __attribute__((__unused__))
> >  #define NULL ((void *)0UL)
> > diff --git a/lib/virtio-testdev.c b/lib/virtio-testdev.c
> > new file mode 100644
> > index 0000000000000..d4a69ede12a7a
> > --- /dev/null
> > +++ b/lib/virtio-testdev.c
> > @@ -0,0 +1,139 @@
> > +#include "libcflat.h"
> > +#include "virtio.h"
> > +
> > +#define TESTDEV_MAJOR_VER 1
> > +#define TESTDEV_MINOR_VER 1
> > +
> > +#define VIRTIO_ID_TESTDEV 0xffff
> > +
> > +#define CONFIG_SIZE 64
> > +
> > +enum {
> > +	VERSION = 1,
> > +	CLEAR,
> > +	EXIT,
> > +};
> > +
> > +#define TOKEN_OFFSET		0x0
> > +#define NARGS_OFFSET		0x4
> > +#define NRETS_OFFSET		0x8
> > +#define ARG_OFFSET(n)		(0xc + (n) * 4)
> > +#define __RET_OFFSET(nargs, n)	(ARG_OFFSET(nargs) + (n) * 4)
> > +
> > +static struct virtio_dev *testdev;
> > +
> > +static u32 testdev_readl(unsigned offset)
> > +{
> > +	if (offset > (CONFIG_SIZE - 4)) {
> > +		printf("%s: offset 0x%x to big!\n", __func__, offset);
> > +		exit(EINVAL);
> > +	}
> > +
> > +	return virtio_config_readl(testdev, offset);
> > +}
> > +
> > +static void testdev_writel(unsigned offset, u32 val)
> > +{
> > +	if (offset > (CONFIG_SIZE - 4)) {
> > +		printf("%s: offset 0x%x to big!\n", __func__, offset);
> > +		exit(EINVAL);
> > +	}
> > +
> > +	virtio_config_writel(testdev, offset, val);
> > +}
> > +
> > +/*
> > + * We have to write all args; nargs, nrets, ... first to avoid executing
> > + * the token's operation until all args are in place. Then issue the op,
> > + * and then read the return values. Reading the return values (or just
> > + * sanity checking by reading token) 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, ...)
> > +{
> > +	va_list va;
> > +	unsigned off;
> > +	u32 n;
> > +
> > +	if (!testdev)
> > +		return;
> > +
> > +	testdev_writel(NARGS_OFFSET, nargs);
> > +	testdev_writel(NRETS_OFFSET, nrets);
> > +
> > +	va_start(va, nrets);
> > +
> > +	off = ARG_OFFSET(0);
> > +	n = nargs;
> > +	while (n--) {
> > +		testdev_writel(off, va_arg(va, unsigned));
> > +		off += 4;
> > +	}
> > +
> > +	/* this runs the op, but then resets token to zero */
> > +	testdev_writel(TOKEN_OFFSET, token);
> > +
> > +	/* sanity check */
> > +	if (testdev_readl(TOKEN_OFFSET) != 0) {
> > +		printf("virtio-testdev token should always read as zero!\n");
> > +		exit(EIO);
> > +	}
> > +
> > +	off = __RET_OFFSET(nargs, 0);
> > +	n = nrets;
> > +	while (n--) {
> > +		u32 *r = va_arg(va, unsigned *);
> > +		*r = testdev_readl(off);
> > +		off += 4;
> > +	}
> > +
> > +	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)
> > +{
> > +	u16 major, minor;
> > +	u32 version;
> > +
> > +	testdev = virtio_bind(VIRTIO_ID_TESTDEV);
> > +	if (testdev == NULL) {
> > +		printf("Can't find virtio-testdev. "
> > +		       "Is '-device virtio-testdev' "
> > +		       "on the qemu command line?\n");
> > +		exit(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("Incompatible version of virtio-testdev: major = %d, "
> > +		       "minor = %d\n", major, minor);
> > +		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
> > diff --git a/lib/virtio.c b/lib/virtio.c
> > new file mode 100644
> > index 0000000000000..8abc8b2399a08
> > --- /dev/null
> > +++ b/lib/virtio.c
> > @@ -0,0 +1,126 @@
> > +#include "libcflat.h"
> > +#include "devicetree.h"
> > +#include "libio.h"
> > +#include "heap.h"
> > +#include "virtio.h"
> > +
> > +struct virtio_dt_device_data {
> > +	u32 device;
> > +	struct dt_bus *bus;
> > +	int node;
> > +	/* cached variables */
> > +	u32 address_cells, size_cells;
> > +};
> > +
> > +static void virtio_dt_device_data_init(struct virtio_dt_device_data *data,
> > +				       u32 device, struct dt_bus *bus)
> > +{
> > +	memset(data, 0, sizeof(struct virtio_dt_device_data));
> > +	data->device = device;
> > +	data->bus = bus;
> > +}
> > +
> > +static int vm_dt_bus_match(const struct dt_bus *bus, int nodeoffset);
> > +static struct virtio_dev *vm_bind(struct virtio_dt_device_data *data);
> > +
> > +struct virtio_dev *virtio_bind(u32 device)
> > +{
> > +	struct virtio_dt_device_data data;
> > +	struct dt_bus bus;
> > +	const char *compatible;
> > +
> > +	/*
> > +	 * currently we only support virtio-mmio
> > +	 */
> > +	compatible = "virtio,mmio";
> > +	dt_bus_init_defaults(&bus, compatible);
> > +	bus.match = vm_dt_bus_match;
> > +	bus.private = (void *)&data;
> > +	virtio_dt_device_data_init(&data, device, &bus);
> > +
> > +	data.node = dt_bus_find_device_compatible(&bus, compatible);
> > +	if (data.node < 0) {
> > +		printf("virtio bind for device id 0x%x failed, "
> > +		       "fdt_error: %s\n", device, dt_strerror(data.node));
> > +		return NULL;
> > +	}
> > +
> > +	return vm_bind(&data);
> > +}
> > +
> > +static int virtio_dt_bus_translate_reg(int nodeoffset,
> > +				       const struct dt_bus *bus,
> > +				       int regidx, void **addr,
> > +				       size_t *size)
> 
> so this is really virtio_dt_bus_get_addr right?

right. Actually, we could probably always assume regidx will be zero,
remove that param, and then rename this.

> 
> > +{
> > +	struct virtio_dt_device_data *data =
> > +		(struct virtio_dt_device_data *)bus->private;
> > +
> > +	return __dt_bus_translate_reg(nodeoffset, bus, regidx,
> > +			&data->address_cells, &data->size_cells, addr, size);
> > +}
> > +
> > +/******************************************************
> > + * virtio_mmio
> > + ******************************************************/
> > +
> > +static int vm_dt_bus_match(const struct dt_bus *bus, int nodeoffset)
> > +{
> > +	struct virtio_dt_device_data *data =
> > +		(struct virtio_dt_device_data *)bus->private;
> > +	void *addr;
> > +	int ret;
> > +
> > +	ret = virtio_dt_bus_translate_reg(nodeoffset, bus, 0, &addr, NULL);
> > +	if (ret < 0) {
> > +		printf("can't get reg! fdt_error: %s\n", dt_strerror(ret));
> > +		return ret;
> > +	}
> > +
> > +	return readl(addr + VIRTIO_MMIO_DEVICE_ID) == data->device;
> > +}
> > +
> > +#define to_virtio_mmio_dev(vdev_ptr) \
> > +	container_of(vdev_ptr, struct virtio_mmio_dev, vdev)
> > +
> > +static void vm_get(struct virtio_dev *vdev, unsigned offset,
> > +		   void *buf, unsigned len)
> > +{
> > +	struct virtio_mmio_dev *vmdev = to_virtio_mmio_dev(vdev);
> > +	u8 *p = buf;
> > +	unsigned i;
> > +
> > +	for (i = 0; i < len; ++i)
> > +		p[i] = readb(vmdev->base + VIRTIO_MMIO_CONFIG + offset + i);
> > +}
> > +
> > +static void vm_set(struct virtio_dev *vdev, unsigned offset,
> > +		   const void *buf, unsigned len)
> > +{
> > +	struct virtio_mmio_dev *vmdev = to_virtio_mmio_dev(vdev);
> > +	const u8 *p = buf;
> > +	unsigned i;
> > +
> > +	for (i = 0; i < len; ++i)
> > +		writeb(p[i], vmdev->base + VIRTIO_MMIO_CONFIG + offset + i);
> > +}
> > +
> > +static struct virtio_dev *vm_bind(struct virtio_dt_device_data *data)
> > +{
> > +	struct virtio_mmio_dev *vmdev;
> > +	void *page;
> > +
> > +	page = alloc_page();
> > +	vmdev = page;
> > +	vmdev->vdev.config = page + sizeof(struct virtio_mmio_dev);
> > +
> > +	vmdev->vdev.id.device = data->device;
> > +	vmdev->vdev.id.vendor = -1;
> > +	vmdev->vdev.config->get = vm_get;
> > +	vmdev->vdev.config->set = vm_set;
> > +
> > +	(void)virtio_dt_bus_translate_reg(data->node, data->bus, 0,
> > +						&vmdev->base, NULL);
> 
> didn't you already do this in vm_dt_bus_mach() ?

yes, but we didn't save the result, so we need to go back to the
node's reg property again and get it. At least we know which node
to go to and {address,size}_cells are cached. Hmm, we might as
well add an addr field to virtio_dt_device_data, allowing match
to save it there. I'll do that.

> 
> > +
> > +	return &vmdev->vdev;
> > +}
> > diff --git a/lib/virtio.h b/lib/virtio.h
> > new file mode 100644
> > index 0000000000000..c30a8dcd105cd
> > --- /dev/null
> > +++ b/lib/virtio.h
> > @@ -0,0 +1,74 @@
> > +#ifndef _VIRTIO_H_
> > +#define _VIRTIO_H_
> > +#include "libcflat.h"
> > +
> > +#define VIRTIO_MMIO_DEVICE_ID	0x008
> > +#define VIRTIO_MMIO_CONFIG	0x100
> > +
> > +struct virtio_devid {
> > +	u32 device;
> > +	u32 vendor;
> > +};
> > +
> > +struct virtio_dev {
> > +	struct virtio_devid id;
> > +	struct virtio_conf_ops *config;
> > +};
> > +
> > +struct virtio_conf_ops {
> > +	void (*get)(struct virtio_dev *vdev, unsigned offset,
> > +		    void *buf, unsigned len);
> > +	void (*set)(struct virtio_dev *vdev, unsigned offset,
> > +		    const void *buf, unsigned len);
> > +};
> > +
> > +struct virtio_mmio_dev {
> > +	struct virtio_dev vdev;
> > +	void *base;
> > +};
> > +
> > +static inline u8
> > +virtio_config_readb(struct virtio_dev *vdev, unsigned offset)
> > +{
> > +	u8 val;
> > +	vdev->config->get(vdev, offset, &val, 1);
> > +	return val;
> > +}
> > +
> > +static inline u16
> > +virtio_config_readw(struct virtio_dev *vdev, unsigned offset)
> > +{
> > +	u16 val;
> > +	vdev->config->get(vdev, offset, &val, 2);
> > +	return val;
> > +}
> > +
> > +static inline u32
> > +virtio_config_readl(struct virtio_dev *vdev, unsigned offset)
> > +{
> > +	u32 val;
> > +	vdev->config->get(vdev, offset, &val, 4);
> > +	return val;
> > +}
> > +
> > +static inline void
> > +virtio_config_writeb(struct virtio_dev *vdev, unsigned offset, u8 val)
> > +{
> > +	vdev->config->set(vdev, offset, &val, 1);
> > +}
> > +
> > +static inline void
> > +virtio_config_writew(struct virtio_dev *vdev, unsigned offset, u16 val)
> > +{
> > +	vdev->config->set(vdev, offset, &val, 2);
> > +}
> > +
> > +static inline void
> > +virtio_config_writel(struct virtio_dev *vdev, unsigned offset, u32 val)
> > +{
> > +	vdev->config->set(vdev, offset, &val, 4);
> > +}
> > +
> > +extern struct virtio_dev *virtio_bind(u32 device);
> > +
> > +#endif
> > -- 
> > 1.8.1.4
> > 
> 
> otherwise:
> 
> Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>