[ipxe-devel] [PATCH v2 3/3] [virtio] Enable and restore PCI device correctly

[Marcel Apfelbaum]

On 05/02/2016 04:11 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
> restored 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 | 41 +++++++++++++++++++++++++++++++++++++++--
>   2 files changed, 49 insertions(+), 2 deletions(-)
>
> diff --git a/src/drivers/bus/virtio-pci.c b/src/drivers/bus/virtio-pci.c
> index a1c805a..0b196b5 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:
> +                pci_enable_device(vdev->pci, PCI_COMMAND_IO, 0);
> +                break;
> +            case VIRTIO_PCI_REGION_MEMORY:
> +                pci_enable_device(vdev->pci, PCI_COMMAND_MEM, 0);
> +                break;
> +        }
>       }
>
>       /* 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..5b15836 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 = pci_enable_device ( pci,
> +		PCI_COMMAND_MASTER | PCI_COMMAND_IO, 0 );
>   	vp_reset ( ioaddr );
>
>   	/* Load MAC address */
> @@ -487,12 +491,34 @@ static int virtnet_probe_legacy ( struct pci_device *pci ) {
>   	unregister_netdev ( netdev );
>    err_register_netdev:
>   	vp_reset ( ioaddr );
> +	pci_restore_device ( pci, virtnet->old_pci_command );
>   	netdev_nullify ( netdev );
>   	netdev_put ( netdev );
>   	return rc;
>   }
>
>   /**
> + * Determine if the device uses at least one region of the given type
> + *
> + * @v vdev	Virtio-net device data
> + * @v type	The region type to test
> + * @ret result	Non-zero if the type is used, zero otherwise
> + */
> +static int virtnet_uses_region_type ( struct virtio_pci_modern_device *vdev,
> +				      unsigned type ) {
> +	if ( ( vdev->common.flags & VIRTIO_PCI_REGION_TYPE_MASK ) == type ) {
> +		return 1;
> +	}
> +	if ( ( vdev->isr.flags & VIRTIO_PCI_REGION_TYPE_MASK ) == type ) {
> +		return 1;
> +	}
> +	if ( ( vdev->device.flags & VIRTIO_PCI_REGION_TYPE_MASK ) == type ) {
> +		return 1;
> +	}
> +	return 0;
> +}
> +
> +/**
>    * Probe PCI device, modern virtio 1.0
>    *
>    * @v pci	PCI device
> @@ -504,6 +530,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 +589,14 @@ 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_uses_region_type ( &virtnet->vdev, VIRTIO_PCI_REGION_PORT ) ) {
> +		pci_command |= PCI_COMMAND_IO;
> +	}
> +	if ( virtnet_uses_region_type ( &virtnet->vdev, VIRTIO_PCI_REGION_MEMORY ) ) {
> +		pci_command |= PCI_COMMAND_MEM;
> +	}
> +	virtnet->old_pci_command = pci_enable_device ( pci, pci_command, 0 );
>
>   	/* Reset the device and set initial status bits */
>   	vpm_reset ( &virtnet->vdev );
> @@ -601,6 +635,7 @@ static int virtnet_probe_modern ( struct pci_device *pci, int *found_dev ) {
>   err_register_netdev:
>   err_mac_address:
>   	vpm_reset ( &virtnet->vdev );
> +	pci_restore_device ( pci, virtnet->old_pci_command );
>   	netdev_nullify ( netdev );
>   	netdev_put ( netdev );
>
> @@ -638,6 +673,8 @@ static void virtnet_remove ( struct pci_device *pci ) {
>   	struct net_device *netdev = pci_get_drvdata ( pci );
>   	struct virtnet_nic *virtnet = netdev->priv;
>
> +	pci_restore_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 );
>


Reviewed-by: Marcel Apfelbaum <marcel at redhat.com>

Thanks,
Marcel