Re: [PATCH v4 11/19] add minimal virtio support for devtree virtio-mmio

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

On Thu, Apr 10, 2014 at 06:56:52PM +0200, Andrew Jones wrote:
> Support the bare minimum of virtio to enable access to the virtio-mmio
> config space of a device. Currently this implementation must use a
> device tree to find the device.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
> v4:
>  - split from the virtio-testdev patch
>  - search a table to "discover" that the device must be DT/virtio-mmio,
>    which doesn't change anything, but looks less hacky than comments
>    saying the device must be DT/virtio-mmio...
>  - manage own pool of virtio-mmio pre-allocated device structures in
>    order to avoid needing access to the heap
> ---
>  lib/libcflat.h |   3 ++
>  lib/virtio.c   | 166 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/virtio.h   |  89 +++++++++++++++++++++++++++++++
>  3 files changed, 258 insertions(+)
>  create mode 100644 lib/virtio.c
>  create mode 100644 lib/virtio.h
> 
> diff --git a/lib/libcflat.h b/lib/libcflat.h
> index c7c31be1cc8e5..5bb66d01dfc53 100644
> --- a/lib/libcflat.h
> +++ b/lib/libcflat.h
> @@ -62,6 +62,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 NULL ((void *)0UL)
>  
> diff --git a/lib/virtio.c b/lib/virtio.c
> new file mode 100644
> index 0000000000000..e7161ff591e4c
> --- /dev/null
> +++ b/lib/virtio.c
> @@ -0,0 +1,166 @@
> +/*
> + * Copyright (C) 2014, Red Hat Inc, Andrew Jones <drjones@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +#include "libcflat.h"
> +#include "devicetree.h"
> +#include "asm/spinlock.h"
> +#include "asm/io.h"
> +#include "virtio.h"
> +
> +enum virtio_hwdesc_type {
> +	VIRTIO_HWDESC_TYPE_DT = 0,	/* device tree */
> +	NR_VIRTIO_HWDESC_TYPES,
> +};
> +
> +enum virtio_bus_type {
> +	VIRTIO_BUS_TYPE_MMIO = 0,	/* virtio-mmio */
> +	NR_VIRTIO_BUS_TYPES,
> +};
> +
> +struct virtio_bind_bus {
> +	bool (*hwdesc_probe)(void);
> +	struct virtio_dev *(*device_bind)(u32 devid);
> +};
> +
> +static struct virtio_dev *vm_dt_device_bind(u32 devid);
> +
> +static struct virtio_bind_bus
> +virtio_bind_busses[NR_VIRTIO_HWDESC_TYPES][NR_VIRTIO_BUS_TYPES] = {
> +
> +[VIRTIO_HWDESC_TYPE_DT] = {
> +
> +	[VIRTIO_BUS_TYPE_MMIO] = {
> +		.hwdesc_probe = dt_available,
> +		.device_bind = vm_dt_device_bind,
> +	},
> +},
> +};
> +
> +struct virtio_dev *virtio_bind(u32 devid)
> +{
> +	struct virtio_bind_bus *bus;
> +	struct virtio_dev *dev;
> +	int i, j;
> +
> +	for (i = 0; i < NR_VIRTIO_HWDESC_TYPES; ++i) {
> +		for (j = 0; j < NR_VIRTIO_BUS_TYPES; ++j) {
> +
> +			bus = &virtio_bind_busses[i][j];
> +
> +			if (!bus->hwdesc_probe())
> +				continue;
> +
> +			dev = bus->device_bind(devid);
> +			if (dev)
> +				return dev;
> +		}
> +	}
> +
> +	return NULL;
> +}
> +
> +/******************************************************
> + * virtio-mmio support (config space only)
> + ******************************************************/
> +
> +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);
> +}
> +
> +#define NR_VM_DEVICES 32
> +static struct spinlock vm_lock;
> +static struct virtio_mmio_dev vm_devs[NR_VM_DEVICES];
> +static struct virtio_conf_ops vm_confs[NR_VM_DEVICES];
> +static int nr_vm_devs;
> +
> +static struct virtio_mmio_dev *vm_new_device(u32 devid)
> +{
> +	struct virtio_mmio_dev *vmdev;
> +
> +	if (nr_vm_devs >= NR_VM_DEVICES)
> +		return NULL;
> +
> +	spin_lock(&vm_lock);
> +	vmdev = &vm_devs[nr_vm_devs];
> +	vmdev->vdev.config = &vm_confs[nr_vm_devs];

this seems a bit weird, will we ever share vm_confs between vm_devs or
change the vm_conf pointer later on?  Could vm_conf just be part of the
vm_dev struct and you could just have a virtio_mmio_conf_init() to set
the get/set functions?

> +	++nr_vm_devs;
> +	spin_unlock(&vm_lock);
> +
> +	vmdev->vdev.id.device = devid;
> +	vmdev->vdev.id.vendor = -1;
> +	vmdev->vdev.config->get = vm_get;
> +	vmdev->vdev.config->set = vm_set;
> +
> +	return vmdev;
> +}
> +
> +/******************************************************
> + * virtio-mmio device tree support
> + ******************************************************/
> +
> +struct vm_dt_info {
> +	u32 devid;
> +	void *base;

won't work for LPAE and 32-bit arm systems, can't you use
dt_device_bind_node?

I guess that requires you to have an ioremap function, but it sure seems
cleaner to give up there if you don't have proper memory mapping
features yet....

> +};
> +
> +static int vm_dt_match(const struct dt_device *dev, int fdtnode)
> +{
> +	struct vm_dt_info *info = (struct vm_dt_info *)dev->info;
> +	dt_pbus_addr_t base;
> +
> +	dt_device_bind_node((struct dt_device *)dev, fdtnode);
> +
> +	assert(dt_pbus_get_baseaddr(dev, &base) == 0);
> +	assert(sizeof(long) == 8 || !(base >> 32));

again, this assert will break with >32bit LPAE addresses on 32-bit arm systems.

so, not sure if it's an assert, or if you should test for it and print
a more benign error for these types of devices.

> +
> +	info->base = (void *)(unsigned long)base;

why do you need to cast it twice?

> +
> +	return readl(info->base + VIRTIO_MMIO_DEVICE_ID) == info->devid;
> +}
> +
> +static struct virtio_dev *vm_dt_device_bind(u32 devid)
> +{
> +	struct virtio_mmio_dev *vmdev;
> +	struct dt_device dt_dev;
> +	struct dt_bus dt_bus;
> +	struct vm_dt_info info;
> +	int node;
> +
> +	dt_bus_init_defaults(&dt_bus);
> +	dt_bus.match = vm_dt_match;
> +
> +	info.devid = devid;
> +
> +	dt_device_init(&dt_dev, &dt_bus, &info);
> +
> +	node = dt_device_find_compatible(&dt_dev, "virtio,mmio");
> +	assert(node >= 0 || node == -FDT_ERR_NOTFOUND);
> +
> +	if (node == -FDT_ERR_NOTFOUND)
> +		return NULL;
> +
> +	vmdev = vm_new_device(devid);
> +	vmdev->base = info.base;
> +
> +	return &vmdev->vdev;
> +}
> diff --git a/lib/virtio.h b/lib/virtio.h
> new file mode 100644
> index 0000000000000..163b9912c4e05
> --- /dev/null
> +++ b/lib/virtio.h
> @@ -0,0 +1,89 @@
> +#ifndef _VIRTIO_H_
> +#define _VIRTIO_H_
> +/*
> + * A minimal implementation of virtio for virtio-mmio config space
> + * access.
> + *
> + * Copyright (C) 2014, Red Hat Inc, Andrew Jones <drjones@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +#include "libcflat.h"
> +
> +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);
> +};
> +
> +extern struct virtio_dev *virtio_bind(u32 devid);
> +
> +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);
> +}
> +
> +/******************************************************
> + * virtio-mmio
> + ******************************************************/
> +
> +#define VIRTIO_MMIO_DEVICE_ID	0x008
> +#define VIRTIO_MMIO_CONFIG	0x100
> +
> +#define to_virtio_mmio_dev(vdev_ptr) \
> +	container_of(vdev_ptr, struct virtio_mmio_dev, vdev)
> +
> +struct virtio_mmio_dev {
> +	struct virtio_dev vdev;
> +	void *base;
> +};
> +
> +#endif /* _VIRTIO_H_ */
> -- 
> 1.8.1.4
> 

-Christoffer