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

[Andrew Jones <drjones@redhat.com>]

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