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

[Marcel Apfelbaum]

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. 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)

>   	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 :)

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