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@xxxxxxxxx> wrote:
> > > > On Fri, Nov 11, 2011 at 6:24 AM, Rusty Russell <rusty@xxxxxxxxxxxxxxx> 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:
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
