Thread (7 messages) 7 messages, 4 authors, 2011-11-16

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

flat view

From: "Michael S. Tsirkin" <mst@redhat.com>
Date: 2011-11-16 09:09:00
Also in: kvm, lkml
Subsystem: the rest, virtio core · Maintainers: Linus Torvalds, "Michael S. Tsirkin", Jason Wang, Eugenio Pérez

Possibly related (same subject, not in this thread)

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:
quoted
On Wed, Nov 16, 2011 at 10:28:52AM +1030, Rusty Russell wrote:
quoted
On Fri, 11 Nov 2011 09:39:13 +0200, Sasha Levin [off-list ref] wrote:
quoted
On Fri, Nov 11, 2011 at 6:24 AM, Rusty Russell [off-list ref] wrote:
quoted
(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:
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help