[Lguest] [PATCH 2/3] lguest: Simplify device descriptor helpers

[Mark McLoughlin]

On Sat, 2008-06-14 at 14:53 +1000, Rusty Russell wrote:
> On Friday 13 June 2008 23:04:59 Mark McLoughlin wrote:

> >  	vring_init(&vq->vring, num_descs, p, getpagesize());
> >
> > -	/* Append virtqueue to this device's descriptor.  We use
> > -	 * device_config() to get the end of the device's current virtqueues;
> > -	 * we check that we haven't added any config or feature information
> > -	 * yet, otherwise we'd be overwriting them. */
> > -	assert(dev->desc->config_len == 0 && dev->desc->feature_len == 0);
> > -	memcpy(device_config(dev), &vq->config, sizeof(vq->config));
> > +	/* Append virtqueue to this device's descriptor. */
> > +	memcpy(device_vq(dev) + dev->desc->num_vq,
> > +	       &vq->config, sizeof(vq->config));
> >  	dev->desc->num_vq++;
> 
> Removing this assertion is incorrect.  Because of the simple layout we use, we 
> really can't add virtqueues after we've added feature bits or config 
> variables.

Quite right. I just considered the assertion part of the explanation of
why device_config() was being used to get the vqconfig.

Updated patch below.

Cheers,
Mark.

From: Mark McLoughlin <markmc at redhat.com>
Subject: lguest: Simplify device descriptor helpers

Confusingly, we use device_config() in add_virtqueue() to
obtain a pointer to the next lguest_vqconfig pointer in
the device descriptor, under the assumption that we haven't
yet set up the feature bitmasks or device config.

There's enough going on here that one might get horribly
confused by this for a second.

Simplify the code by adding helpers similar to lg_vq(),
lg_features() and lg_config() functions from the kernel's
guest device code.

That way it's a bit more clear which config is which.

Signed-off-by: Mark McLoughlin <markmc at redhat.com>

Index: linux-2.6/Documentation/lguest/lguest.c
===================================================================
--- linux-2.6.orig/Documentation/lguest/lguest.c	2008-06-19 05:04:27.000000000 +0100
+++ linux-2.6/Documentation/lguest/lguest.c	2008-06-19 05:44:42.000000000 +0100
@@ -199,11 +199,25 @@
 #define le32_to_cpu(v32) (v32)
 #define le64_to_cpu(v64) (v64)
 
+/* The layout of the device page is a "struct lguest_device_desc" followed by a
+ * number of virtqueue descriptors, then two sets of feature bits, then an
+ * array of configuration bytes.  This routine returns a pointer to the
+ * virtqueue descriptors. */
+static struct lguest_vqconfig *device_vqconfig(const struct device *dev)
+{
+	return (void *)(dev->desc + 1);
+}
+
 /* The device virtqueue descriptors are followed by feature bitmasks. */
-static u8 *get_feature_bits(struct device *dev)
+static u8 *get_feature_bits(const struct device *dev)
+{
+	return (void *)(device_vqconfig(dev) + dev->desc->num_vq);
+}
+
+/* The device configuration follows the feature bitmasks */
+static u8 *device_config(const struct device *dev)
 {
-	return (u8 *)(dev->desc + 1)
-		+ dev->desc->num_vq * sizeof(struct lguest_vqconfig);
+	return get_feature_bits(dev) + dev->desc->feature_len * 2;
 }
 
 /*L:100 The Launcher code itself takes us out into userspace, that scary place
@@ -1061,17 +1075,6 @@
  * routines to allocate and manage them.
  */
 
-/* The layout of the device page is a "struct lguest_device_desc" followed by a
- * number of virtqueue descriptors, then two sets of feature bits, then an
- * array of configuration bytes.  This routine returns the configuration
- * pointer. */
-static u8 *device_config(const struct device *dev)
-{
-	return (void *)(dev->desc + 1)
-		+ dev->desc->num_vq * sizeof(struct lguest_vqconfig)
-		+ dev->desc->feature_len * 2;
-}
-
 /* This routine allocates a new "struct lguest_device_desc" from descriptor
  * table page just above the Guest's normal memory.  It returns a pointer to
  * that descriptor. */
@@ -1123,12 +1126,12 @@
 	/* Initialize the vring. */
 	vring_init(&vq->vring, num_descs, p, getpagesize());
 
-	/* Append virtqueue to this device's descriptor.  We use
-	 * device_config() to get the end of the device's current virtqueues;
-	 * we check that we haven't added any config or feature information
-	 * yet, otherwise we'd be overwriting them. */
+	/* Append virtqueue to this device's descriptor. We check that we haven't
+	 * added any config or feature information yet, otherwise we'd be
+	 * overwriting them. */
 	assert(dev->desc->config_len == 0 && dev->desc->feature_len == 0);
-	memcpy(device_config(dev), &vq->config, sizeof(vq->config));
+	memcpy(device_vqconfig(dev) + dev->desc->num_vq, &vq->config,
+	       sizeof(vq->config));
 	dev->desc->num_vq++;
 
 	verbose("Virtqueue page %#lx\n", to_guest_phys(p));