Re: [PATCHv2 RFC] virtio-spec: flexible configuration layout

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Nov 16, 2011 at 10:17:39AM +0200, Sasha Levin wrote:
> On Wed, 2011-11-16 at 09:21 +0200, Michael S. Tsirkin wrote:
> > On Wed, Nov 16, 2011 at 10:28:52AM +1030, Rusty Russell wrote:
> > > On Fri, 11 Nov 2011 09:39:13 +0200, Sasha Levin <levinsasha928@gmail.com> wrote:
> > > > On Fri, Nov 11, 2011 at 6:24 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> > > > > (2) There's no huge win in keeping the same layout.  Let's make some
> > > > >    cleanups.  There are more users ahead of us then behind us (I
> > > > >    hope!).
> > > > 
> > > > Actually, if we already do cleanups, here are two more suggestions:
> > > > 
> > > > 1. Make 64bit features a one big 64bit block, instead of having 32bits
> > > > in one place and 32 in another.
> > > > 2. Remove the reserved fields out of the config (the ones that were
> > > > caused by moving the ISR and the notifications out).
> > > 
> > > Yes, those were exactly what I was thinking.  I left it vague because
> > > there might be others you can see if we're prepared to abandon the
> > > current format.
> > > 
> > > Cheers,
> > > Rusty.
> > 
> > Yes but driver code doesn't get any cleaner by moving the fields.
> > And in fact, the legacy support makes the code messier.
> > What are the advantages?
> > 

The advantages question is what should really balance out the overhead.

> What about splitting the parts which handle legacy code and new code?

Well, I considered that. Something along the lines of
#define VIRTIO_NEW_MSI_CONFIG_VECTOR        18
And so on for all registers.

This seems to add a significant maintainance burden because of code
duplication. Note that, for example, vector programming is affected.
Multiply that by the number of guest OSes.


> It'll make it easier playing with the new spec more freely

I'm really worried about maintaing drivers long term.
Ease of experimentation is secondary for me.

> and will also
> make it easier removing legacy code in the future since you'll need to
> simply delete a chunk of code instead of removing legacy bits out of
> working code with a surgical knife.

It's unlikely to be a single chunk: we'd have structures and macros
which are separate. So at least 3 chunks.

Just for fun, here's what's involved in removing legacy map
support on top of my patch. As you see there are 4 chunks:
structure decl, map, unmap, and msix enable/disable.
And finding them was as simple as looking for legacy_map.


---

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index d242fcc..6c4d2faf 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -64,9 +64,6 @@ struct virtio_pci_device
 
 	/* Various IO mappings: used for resource tracking only. */
 
-	/* Legacy BAR0: typically PIO. */
-	void __iomem *legacy_map;
-
 	/* Mappings specified by device capabilities: typically in MMIO */
 	void __iomem *isr_map;
 	void __iomem *notify_map;
@@ -81,11 +78,7 @@ struct virtio_pci_device
 static void virtio_pci_set_msix_enabled(struct virtio_pci_device *vp_dev, int enabled)
 {
 	vp_dev->msix_enabled = enabled;
-	if (vp_dev->device_map)
-		vp_dev->ioaddr_device = vp_dev->device_map;
-	else
-		vp_dev->ioaddr_device = vp_dev->legacy_map +
-			VIRTIO_PCI_CONFIG(vp_dev);
+	vp_dev->ioaddr_device = vp_dev->device_map;
 }
 
 static void __iomem *virtio_pci_map_cfg(struct virtio_pci_device *vp_dev, u8 cap_id,
@@ -147,8 +140,6 @@ err:
 
 static void virtio_pci_iounmap(struct virtio_pci_device *vp_dev)
 {
-	if (vp_dev->legacy_map)
-		pci_iounmap(vp_dev->pci_dev, vp_dev->legacy_map);
 	if (vp_dev->isr_map)
 		pci_iounmap(vp_dev->pci_dev, vp_dev->isr_map);
 	if (vp_dev->notify_map)
@@ -176,36 +167,15 @@ static int virtio_pci_iomap(struct virtio_pci_device *vp_dev)
 
 	if (!vp_dev->notify_map || !vp_dev->common_map ||
 	    !vp_dev->device_map) {
-		/*
-		 * If not all capabilities present, map legacy PIO.
-		 * Legacy access is at BAR 0. We never need to map
-		 * more than 256 bytes there, since legacy config space
-		 * used PIO which has this size limit.
-		 * */
-		vp_dev->legacy_map = pci_iomap(vp_dev->pci_dev, 0, 256);
-		if (!vp_dev->legacy_map) {
-			dev_err(&vp_dev->vdev.dev, "Unable to map legacy PIO");
-			goto err;
-		}
+		dev_err(&vp_dev->vdev.dev, "Unable to map IO");
+		goto err;
 	}
 
-	/* Prefer MMIO if available. If not, fallback to legacy PIO. */
-	if (vp_dev->common_map)
-		vp_dev->ioaddr = vp_dev->common_map;
-	else
-		vp_dev->ioaddr = vp_dev->legacy_map;
+	vp_dev->ioaddr = vp_dev->common_map;
 
-	if (vp_dev->device_map)
-		vp_dev->ioaddr_device = vp_dev->device_map;
-	else
-		vp_dev->ioaddr_device = vp_dev->legacy_map +
-			VIRTIO_PCI_CONFIG(vp_dev);
+	vp_dev->ioaddr_device = vp_dev->device_map;
 
-	if (vp_dev->notify_map)
-		vp_dev->ioaddr_notify = vp_dev->notify_map;
-	else
-		vp_dev->ioaddr_notify = vp_dev->legacy_map +
-			VIRTIO_PCI_QUEUE_NOTIFY;
+	vp_dev->ioaddr_notify = vp_dev->notify_map;
 
 	return 0;
 err: