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

[Marcel Apfelbaum]

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