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

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

On Mon, Jun 09, 2014 at 11:02:57AM +0200, Andrew Jones wrote:
> On Fri, Jun 06, 2014 at 08:39:18PM +0200, Christoffer Dall wrote:
> > 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?
> 
> I can do that, but it'll require a typedef for struct virtio_dev to
> appease the compiler.
> 
> > 
> > > +	++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....
> 
> Right. I was just skipping past that complexity for now, as mach-virt's
> DT is currently "nice" (32-bit only addrs). I agree we need to handle
> LPAE addrs on arm eventually though.
> 

So I think the tradeoff here is very small, it's super easy to fix it so
you can support physical addresses > 32 bits in this part of the code.
The difficult thing comes when you start dereferencing them, but since
we may add virtual addresses at some point too, I really think adding a
trivial ioremap function and start using that will make our lives much
easier down the line:

/* We don't support virtual addresses yet, trivial identity implementation */
void __iomem *ioremap(phys_addr_t phys_addr)
{
	assert(sizeof(long) == 8 || !(base >> 32));
	return (void *)(unsigned long)phys_addr;
}

But I can submit that patch after this code goes in if you think it's
too much of a headache at this point?

Thanks,
-Christoffer