Re: [PATCH/RFC 4/9] Basic guest virtual devices infrastructure

[Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>]

On Friday 11 May 2007, Carsten Otte wrote:

> This patch adds support for a new bus type that manages paravirtualized
> devices. The bus uses the s390 diagnose instruction to query devices, and
> match them with the corresponding drivers.

It seems that the diagnose instruction is really the only s390 specific
thing in here, right? I guess this part of your series is the first one
that we should have in an architecture independent way.

There may also be the chance of merging this with existing virtual
buses like the one for the ps3, which also just exists using
hypercalls.

> +int vdev_match(struct device * dev, struct device_driver *drv)
> +{
> +	struct vdev *vdev = to_vdev(dev);
> +	struct vdev_driver *vdrv = to_vdrv(drv);
> +
> +	if (vdev->vdev_type == vdrv->vdev_type)
> +		return 1;
> +
> +	return 0;
> +}

Why invent device type numbers? On open firmware, we just do a string compare,
which more intuitive, and means you don't need any further 

> +int vdev_probe(struct device * dev)
> +{
> +	struct vdev *vdev = to_vdev(dev);
> +	struct vdev_driver *vdrv = to_vdrv(dev->driver);
> +
> +	return vdrv->probe(vdev);
> +}

This abstraction is unnecessary, just do the do_vdev() conversion inside
of the individual drivers.

> +
> +struct device vdev_bus = {
> +	.bus_id  = "vdev0",
> +	.release = vdev_bus_release
> +};
> 
> +static void vdev_bus_release (struct device *device)
> +{
> +	/* noop, static bus object */
> +}

Just make the root of your devices a platform_device, then you don't need
to do dirty tricks like this.

> +static int vdev_scan_coldplug(void)
> +{
> +	int rc;
> +	struct vdev *device;
> +
> +	do {
> +		device = kzalloc(sizeof(struct vdev), GFP_ATOMIC);
> +		if (!device) {
> +			rc = -ENOMEM;
> +			goto out;
> +		}
> +		rc = vdev_diag_hotplug(device->symname, device->hostid);
> +		if (rc == -ENODEV)
> +			break;
> +		if (rc < 0) {
> +			printk (KERN_WARNING "vdev: error %d detecting" \
> +				" initial devices\n", rc);
> +			break;
> +		}
> +		device->vdev_type = rc;
> +
> +		//sanity: are strings terminated?
> +		if ((strnlen(device->symname, 128) == 128) ||
> +		    (strnlen(device->hostid, 128) == 128)) {
> +			// warn and discard device
> +			printk ("vdev: illegal device entry received\n");
> +			break;
> +		}
> +
> +		rc = vdevice_register(device);
> +		if (rc) {
> +			kfree(device);
> +		} else
> +			switch (device->vdev_type) {
> +			case VDEV_TYPE_DISK:
> +				printk (KERN_INFO "vdev: storage device " \
> +					"detected: %s\n", device->symname);
> +				break;
> +			case VDEV_TYPE_NET:
> +				printk (KERN_INFO "vdev: network device " \
> +					"detected: %s\n", device->symname);
> +				break;
> +			default:
> +				printk (KERN_INFO "vdev: unknown device " \
> +					"detected: %s\n", device->symname);
> +			}
> +	} while(1);
> +	kfree (device);
> + out:
> +	return 0;
> +}

Interesting concept of probing the bus -- so you just ask if there are
any new devices, right?

> +#define VDEV_TYPE_DISK     0
> +#define VDEV_TYPE_NET      1
> +
> +struct vdev {
> +	unsigned int        vdev_type;
> +	char                symname[128];
> +	char                hostid[128];
> +	struct vdev_driver *driver;
> +	struct device       dev;
> +	void                *drv_private;
> +};

You shouldn't need the driver and drv_private fields -- they are already
present in struct device.

	Arnd <><

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/