Re: [PATCH v6 07/17] add minimal virtio support for devtree virtio-mmio

[Andrew Jones <drjones@redhat.com>]

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