diff mbox

[v6,07/17] add minimal virtio support for devtree virtio-mmio

Message ID 1405066787-5793-8-git-send-email-drjones@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andrew Jones July 11, 2014, 8:19 a.m. UTC
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>
Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
---
v6:
 - switch to using alloc()
 - s/vmdev/vm_dev/ to be consistent with kernel naming
 - check for virtio magic in vm_dt_match
v5:
 - use same virtio struct names as kernel
 - no need to alloc a new virtio_config_ops for each virtio device
 - use ioremap
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   | 158 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/virtio.h   |  89 ++++++++++++++++++++++++++++++++
 3 files changed, 250 insertions(+)
 create mode 100644 lib/virtio.c
 create mode 100644 lib/virtio.h

Comments

Paolo Bonzini July 11, 2014, 8:32 a.m. UTC | #1
Il 11/07/2014 10:19, Andrew Jones ha scritto:
> +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_device *(*device_bind)(u32 devid);
> +};
> +
> +static struct virtio_device *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,
> +	},
> +},
> +};
> +

If you put this in lib/virtio.c, it is overengineered.  It would make 
sense if something else provided virtio_bind_busses[][].

I suggest that you drop it and split this file in four:

lib/virtio.c
lib/virtio-mmio.c
lib/virtio-mmio-dt.c
lib/arm/virtio.c

where virtio_bind is in lib/arm/virtio.c.

Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andrew Jones July 11, 2014, 9:08 a.m. UTC | #2
On Fri, Jul 11, 2014 at 10:32:51AM +0200, Paolo Bonzini wrote:
> Il 11/07/2014 10:19, Andrew Jones ha scritto:
> >+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_device *(*device_bind)(u32 devid);
> >+};
> >+
> >+static struct virtio_device *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,
> >+	},
> >+},
> >+};
> >+
> 
> If you put this in lib/virtio.c, it is overengineered.  It would make sense
> if something else provided virtio_bind_busses[][].

yeah, I acknowledged in the v4 changelog that it was unnecessary at the
moment, but it's there to reduce the ugly hardcodeding that this simplified
version currently needs, and who knows what the future shall bring.

> 
> I suggest that you drop it and split this file in four:
> 
> lib/virtio.c
> lib/virtio-mmio.c

This split already crossed my mind.

> lib/virtio-mmio-dt.c

I'm not sure we need this one, but OK.

> lib/arm/virtio.c
> 
> where virtio_bind is in lib/arm/virtio.c.
>

Well, virtio_bind will still need to be in lib/virtio.c, but just as
a wrapper to arch_virtio_bind. And, I'm inclined to keep virtio_bind_busses
in arm's arch_virtio_bind.

drew
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paolo Bonzini July 11, 2014, 9:27 a.m. UTC | #3
Il 11/07/2014 11:08, Andrew Jones ha scritto:
>> > lib/arm/virtio.c
>> >
>> > where virtio_bind is in lib/arm/virtio.c.
>> >
> Well, virtio_bind will still need to be in lib/virtio.c, but just as
> a wrapper to arch_virtio_bind.

Ok, that's just a naming thing.

> And, I'm inclined to keep virtio_bind_busses
> in arm's arch_virtio_bind.

Why?  To support virtio-pci in the future?  It seems like a good thing 
to have (future support for virtio-pci) but even then you'd have only 
two tests and that's already the exception.  The common case would be 
just one.  You could write that as

     struct virtio_device *arch_virtio_bind(u32 devid)
     {
         struct virtio_device *vdev;

	vdev = arch_virtio_mmio_bind(devid);
	if (!vdev)
	    vdev = arch_virtio_pci_bind(devid);
	return vdev;
     }

(I don't see kvm-unit-tests using ACPI in the future.  Having DT+ACPI x 
mmio+pci would be a good reason to have the array, but even then it's 
premature and these are unit tests not an OS...).

Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andrew Jones July 11, 2014, 9:36 a.m. UTC | #4
On Fri, Jul 11, 2014 at 11:27:58AM +0200, Paolo Bonzini wrote:
> Il 11/07/2014 11:08, Andrew Jones ha scritto:
> >>> lib/arm/virtio.c
> >>>
> >>> where virtio_bind is in lib/arm/virtio.c.
> >>>
> >Well, virtio_bind will still need to be in lib/virtio.c, but just as
> >a wrapper to arch_virtio_bind.
> 
> Ok, that's just a naming thing.
> 
> >And, I'm inclined to keep virtio_bind_busses
> >in arm's arch_virtio_bind.
> 
> Why?  To support virtio-pci in the future?  It seems like a good thing to
> have (future support for virtio-pci) but even then you'd have only two tests
> and that's already the exception.  The common case would be just one.  You
> could write that as
> 
>     struct virtio_device *arch_virtio_bind(u32 devid)
>     {
>         struct virtio_device *vdev;
> 
> 	vdev = arch_virtio_mmio_bind(devid);
> 	if (!vdev)
> 	    vdev = arch_virtio_pci_bind(devid);
> 	return vdev;
>     }
> 
> (I don't see kvm-unit-tests using ACPI in the future.  Having DT+ACPI x
> mmio+pci would be a good reason to have the array, but even then it's

Yes, that was the reason.

> premature and these are unit tests not an OS...).

But, true and true. I guess I'll drop the table for now.

drew
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andrew Jones July 15, 2014, 5:22 p.m. UTC | #5
On Fri, Jul 11, 2014 at 11:08:28AM +0200, Andrew Jones wrote:
> On Fri, Jul 11, 2014 at 10:32:51AM +0200, Paolo Bonzini wrote:
> > Il 11/07/2014 10:19, Andrew Jones ha scritto:
> > >+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_device *(*device_bind)(u32 devid);
> > >+};
> > >+
> > >+static struct virtio_device *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,
> > >+	},
> > >+},
> > >+};
> > >+
> > 
> > If you put this in lib/virtio.c, it is overengineered.  It would make sense
> > if something else provided virtio_bind_busses[][].
> 
> yeah, I acknowledged in the v4 changelog that it was unnecessary at the
> moment, but it's there to reduce the ugly hardcodeding that this simplified
> version currently needs, and who knows what the future shall bring.
> 
> > 
> > I suggest that you drop it and split this file in four:
> > 
> > lib/virtio.c
> > lib/virtio-mmio.c
> 
> This split already crossed my mind.

I made this split.

> 
> > lib/virtio-mmio-dt.c
> 
> I'm not sure we need this one, but OK.

I opted not to make this one. To me it seems reasonable to keep virtio-mmio
and virtio-mmio-dt code together, if for nothing else just to lean more
on the reduce file count side of the trade-off.

> 
> > lib/arm/virtio.c
> > 
> > where virtio_bind is in lib/arm/virtio.c.
> >
> 
> Well, virtio_bind will still need to be in lib/virtio.c, but just as
> a wrapper to arch_virtio_bind. And, I'm inclined to keep virtio_bind_busses
> in arm's arch_virtio_bind.
> 

I dropped the idea of depending on arch-specific implementations being
necessary. As the whole reason to get rid of my table is to simplify
things, then I think all we need for now is

virtio_bind
  virtio_mmio_bind
    virtio_mmio_dt_bind

to maintain abstractions and give room for expansions (virtio-pci).

drew
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/lib/libcflat.h b/lib/libcflat.h
index 9f76d6741344d..57bdb92a3e1b4 100644
--- a/lib/libcflat.h
+++ b/lib/libcflat.h
@@ -56,6 +56,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..8e48d364bec7e
--- /dev/null
+++ b/lib/virtio.c
@@ -0,0 +1,158 @@ 
+/*
+ * 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 "alloc.h"
+#include "devicetree.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_device *(*device_bind)(u32 devid);
+};
+
+static struct virtio_device *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_device *virtio_bind(u32 devid)
+{
+	struct virtio_bind_bus *bus;
+	struct virtio_device *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_device *vdev, unsigned offset,
+		   void *buf, unsigned len)
+{
+	struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
+	u8 *p = buf;
+	unsigned i;
+
+	for (i = 0; i < len; ++i)
+		p[i] = readb(vm_dev->base + VIRTIO_MMIO_CONFIG + offset + i);
+}
+
+static void vm_set(struct virtio_device *vdev, unsigned offset,
+		   const void *buf, unsigned len)
+{
+	struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
+	const u8 *p = buf;
+	unsigned i;
+
+	for (i = 0; i < len; ++i)
+		writeb(p[i], vm_dev->base + VIRTIO_MMIO_CONFIG + offset + i);
+}
+
+static const struct virtio_config_ops vm_config_ops = {
+	.get = vm_get,
+	.set = vm_set,
+};
+
+static void vm_device_init(struct virtio_mmio_device *vm_dev)
+{
+	vm_dev->vdev.id.device = readl(vm_dev->base + VIRTIO_MMIO_DEVICE_ID);
+	vm_dev->vdev.id.vendor = readl(vm_dev->base + VIRTIO_MMIO_VENDOR_ID);
+	vm_dev->vdev.config = &vm_config_ops;
+}
+
+/******************************************************
+ * virtio-mmio device tree support
+ ******************************************************/
+
+struct vm_dt_info {
+	u32 devid;
+	void *base;
+};
+
+static int vm_dt_match(const struct dt_device *dev, int fdtnode)
+{
+	struct vm_dt_info *info = (struct vm_dt_info *)dev->info;
+	struct dt_pbus_reg base;
+	u32 magic;
+
+	dt_device_bind_node((struct dt_device *)dev, fdtnode);
+
+	assert(dt_pbus_get_base(dev, &base) == 0);
+	info->base = ioremap(base.addr, base.size);
+
+	magic = readl(info->base + VIRTIO_MMIO_MAGIC_VALUE);
+	if (magic != ('v' | 'i' << 8 | 'r' << 16 | 't' << 24))
+		return false;
+
+	return readl(info->base + VIRTIO_MMIO_DEVICE_ID) == info->devid;
+}
+
+static struct virtio_device *vm_dt_device_bind(u32 devid)
+{
+	struct virtio_mmio_device *vm_dev;
+	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;
+
+	vm_dev = alloc(sizeof(*vm_dev));
+	if (!vm_dev)
+		return NULL;
+
+	vm_dev->base = info.base;
+	vm_device_init(vm_dev);
+
+	return &vm_dev->vdev;
+}
diff --git a/lib/virtio.h b/lib/virtio.h
new file mode 100644
index 0000000000000..16ebe7e0a7e70
--- /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_device_id {
+	u32 device;
+	u32 vendor;
+};
+
+struct virtio_device {
+	struct virtio_device_id id;
+	const struct virtio_config_ops *config;
+};
+
+struct virtio_config_ops {
+	void (*get)(struct virtio_device *vdev, unsigned offset,
+		    void *buf, unsigned len);
+	void (*set)(struct virtio_device *vdev, unsigned offset,
+		    const void *buf, unsigned len);
+};
+
+extern struct virtio_device *virtio_bind(u32 devid);
+
+static inline u8
+virtio_config_readb(struct virtio_device *vdev, unsigned offset)
+{
+	u8 val;
+	vdev->config->get(vdev, offset, &val, 1);
+	return val;
+}
+
+static inline u16
+virtio_config_readw(struct virtio_device *vdev, unsigned offset)
+{
+	u16 val;
+	vdev->config->get(vdev, offset, &val, 2);
+	return val;
+}
+
+static inline u32
+virtio_config_readl(struct virtio_device *vdev, unsigned offset)
+{
+	u32 val;
+	vdev->config->get(vdev, offset, &val, 4);
+	return val;
+}
+
+static inline void
+virtio_config_writeb(struct virtio_device *vdev, unsigned offset, u8 val)
+{
+	vdev->config->set(vdev, offset, &val, 1);
+}
+
+static inline void
+virtio_config_writew(struct virtio_device *vdev, unsigned offset, u16 val)
+{
+	vdev->config->set(vdev, offset, &val, 2);
+}
+
+static inline void
+virtio_config_writel(struct virtio_device *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_device(vdev_ptr) \
+	container_of(vdev_ptr, struct virtio_mmio_device, vdev)
+
+struct virtio_mmio_device {
+	struct virtio_device vdev;
+	void *base;
+};
+
+#endif /* _VIRTIO_H_ */