[ipxe-devel] [PATCH 2/2] [virtio] Enable and disable PCI device correctly

[Michael S. Tsirkin]

On Sun, May 01, 2016 at 03:25:07PM +0300, Marcel Apfelbaum wrote:
> On 04/28/2016 06:34 PM, Ladi Prosek wrote:
> >The driver enabled both memory and I/O access even if they were
> >not usable, e.g. BAR not mapped by BIOS. This commit fixes it by
> >enabling only the BAR types actually used. The device is also
> >reverted to the original state (in terms of the command PCI
> >register) on removal.
> >
> >Signed-off-by: Ladi Prosek <lprosek at redhat.com>
> >---
> >  src/drivers/bus/virtio-pci.c | 10 ++++++++++
> >  src/drivers/net/virtio-net.c | 30 ++++++++++++++++++++++++++++--
> >  2 files changed, 38 insertions(+), 2 deletions(-)
> >
> >diff --git a/src/drivers/bus/virtio-pci.c b/src/drivers/bus/virtio-pci.c
> >index a1c805a..2a7f539 100644
> >--- a/src/drivers/bus/virtio-pci.c
> >+++ b/src/drivers/bus/virtio-pci.c
> >@@ -387,6 +387,16 @@ int vpm_find_vqs(struct virtio_pci_modern_device *vdev,
> >          if (err) {
> >              goto err_map_notify;
> >          }
> >+
> >+        /* enable memory or I/O access if not already enabled */
> >+        switch (vq->notification.flags & VIRTIO_PCI_REGION_TYPE_MASK) {
> >+            case VIRTIO_PCI_REGION_PORT:
> >+                enable_pci_device(vdev->pci, PCI_COMMAND_IO);
> >+                break;
> >+            case VIRTIO_PCI_REGION_MEMORY:
> >+                enable_pci_device(vdev->pci, PCI_COMMAND_MEM);
> >+                break;
> 
> It seems a little strange that the device is enabled inside a "query" function.
> 
> 
> >+        }
> >      }
> >
> >      /* Select and activate all queues. Has to be done last: once we do
> >diff --git a/src/drivers/net/virtio-net.c b/src/drivers/net/virtio-net.c
> >index fe0fd4b..3f35806 100644
> >--- a/src/drivers/net/virtio-net.c
> >+++ b/src/drivers/net/virtio-net.c
> >@@ -92,6 +92,9 @@ struct virtnet_nic {
> >  	/** 0 for legacy, 1 for virtio 1.0 */
> >  	int virtio_version;
> >
> >+	/** Content of the command register before we initialized the device */
> >+	unsigned old_pci_command;
> >+
> >  	/** Virtio 1.0 device data */
> >  	struct virtio_pci_modern_device vdev;
> >
> >@@ -463,7 +466,8 @@ static int virtnet_probe_legacy ( struct pci_device *pci ) {
> >  	       virtnet, pci->dev.name, ioaddr, pci->irq );
> >
> >  	/* Enable PCI bus master and reset NIC */
> >-	adjust_pci_device ( pci );
> >+	virtnet->old_pci_command = enable_pci_device ( pci,
> >+		PCI_COMMAND_MASTER | PCI_COMMAND_IO );
> 
> I remember MSI/MSI-X is optional for legacy virtio devices, yet
> you enable Bus Master unconditionally.

virtio vqs can't work without bus mastering.
As all devices have vqs, BM is always required.

> I have not seen other virtio
> implementations, so this may be the way to go. (I would expect to be
> conditioned based on the existence of the PCI MSI capability)

Not really I think.

> >  	vp_reset ( ioaddr );
> >
> >  	/* Load MAC address */
> >@@ -487,6 +491,7 @@ static int virtnet_probe_legacy ( struct pci_device *pci ) {some capability/feature flag
> >  	unregister_netdev ( netdev );
> >   err_register_netdev:
> >  	vp_reset ( ioaddr );
> >+	disable_pci_device ( pci, virtnet->old_pci_command );
> >  	netdev_nullify ( netdev );
> >  	netdev_put ( netdev );
> >  	return rc;
> >@@ -504,6 +509,7 @@ static int virtnet_probe_modern ( struct pci_device *pci, int *found_dev ) {
> >  	struct virtnet_nic *virtnet;
> >  	u64 features;
> >  	int rc, common, isr, notify, config, device;
> >+	unsigned pci_command;
> >
> >  	common = virtio_pci_find_capability ( pci, VIRTIO_PCI_CAP_COMMON_CFG );
> >  	if ( ! common ) {
> >@@ -562,7 +568,24 @@ static int virtnet_probe_modern ( struct pci_device *pci, int *found_dev ) {
> >  	}
> >
> >  	/* Enable the PCI device */
> >-	adjust_pci_device ( pci );
> >+	pci_command = PCI_COMMAND_MASTER;
> >+	if ( ( virtnet->vdev.common.flags & VIRTIO_PCI_REGION_TYPE_MASK ) ==
> >+		VIRTIO_PCI_REGION_PORT ||
> >+	     ( virtnet->vdev.isr.flags & VIRTIO_PCI_REGION_TYPE_MASK ) ==
> >+		VIRTIO_PCI_REGION_PORT ||
> >+	     ( device && ( virtnet->vdev.device.flags & VIRTIO_PCI_REGION_TYPE_MASK ) ==
> >+		VIRTIO_PCI_REGION_PORT ) ) {
> >+		pci_command |= PCI_COMMAND_IO;
> >+	}
> >+	if ( ( virtnet->vdev.common.flags & VIRTIO_PCI_REGION_TYPE_MASK ) ==
> >+		VIRTIO_PCI_REGION_MEMORY ||
> >+	     ( virtnet->vdev.isr.flags & VIRTIO_PCI_REGION_TYPE_MASK ) ==
> >+		VIRTIO_PCI_REGION_MEMORY ||
> >+	     ( device && ( virtnet->vdev.device.flags & VIRTIO_PCI_REGION_TYPE_MASK ) ==
> >+		VIRTIO_PCI_REGION_MEMORY ) ) {
> >+		pci_command |= PCI_COMMAND_MEM;
> >+	}
> 
> I find the above hard to parse and at least one more () is missing for each ==.
> I am not saying I know how to do this better :)


Write a function getting region type and saying whether it's in use.
Call it twice.


> 
> >+	virtnet->old_pci_command = enable_pci_device ( pci, pci_command );
> >
> >  	/* Reset the device and set initial status bits */
> >  	vpm_reset ( &virtnet->vdev );
> >@@ -601,6 +624,7 @@ static int virtnet_probe_modern ( struct pci_device *pci, int *found_dev ) {
> >  err_register_netdev:
> >  err_mac_address:
> >  	vpm_reset ( &virtnet->vdev );
> >+	disable_pci_device ( pci, virtnet->old_pci_command );
> >  	netdev_nullify ( netdev );
> >  	netdev_put ( netdev );
> >
> >@@ -638,6 +662,8 @@ static void virtnet_remove ( struct pci_device *pci ) {
> >  	struct net_device *netdev = pci_get_drvdata ( pci );
> >  	struct virtnet_nic *virtnet = netdev->priv;
> >
> >+	disable_pci_device ( pci, virtnet->old_pci_command );
> >+
> >  	virtio_pci_unmap_capability ( &virtnet->vdev.device );
> >  	virtio_pci_unmap_capability ( &virtnet->vdev.isr );
> >  	virtio_pci_unmap_capability ( &virtnet->vdev.common );
> >
> 
> 
> Thanks,
> Marcel