On 05/02/2016 11:00 AM, Ladi Prosek wrote:
On Sun, May 1, 2016 at 2:41 PM, Michael S. Tsirkin <m...@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 <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.
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.
You can make the call to latency register optional by adding another parameter
to enable_pci_device, say bool update_latency.
Also, maybe instead of writing to PCI registers in virtio_pci_map_capability,
you can save the info on virtio_nic struct like you did for unsigned
old_pci_command;
Just a thought, I only had a quick look at the code.
Thanks,
Marcel
+ 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
