On 05/01/2016 03:41 PM, Michael S. Tsirkin 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 <lpro...@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.
Right, I forgot the legacy virtio devices are DMA capable.
Please disregard this comment.
Thanks,
Marcel
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
_______________________________________________
ipxe-devel mailing list
ipxe-devel@lists.ipxe.org
https://lists.ipxe.org/mailman/listinfo.cgi/ipxe-devel
