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

[Marcel Apfelbaum]

On 05/02/2016 11:00 AM, Ladi Prosek wrote:
> On Sun, May 1, 2016 at 2:41 PM, Michael S. Tsirkin <mst at 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 <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.
>
> 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