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

[Ladi Prosek]

On Sun, May 1, 2016 at 2:41 PM, Michael S. Tsirkin <mst at redhat.com> wrote:
> 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.

I agree that the name (inherited from Linux) kind of implies
immutability. I could maybe move enable_pci_device out to the caller
but I believe that having it close to the virtio_pci_map_capability
call is better readability-wise.

>> >+        }
>> >      }
>> >
>> >      /* 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.

I was thinking about calling enable_pci_device right inside
virtio_pci_map_capability which would also kill this pattern. We would
be reading and writing (because of the read-only latency timer) PCI
registers repeatedly, or three times to be exact, though.

>>
>> >+    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