[ipxe-devel] [PATCH v2 4/4] [virtio] Add virtio-net 1.0 support

Michael S. Tsirkin mst at redhat.com
Tue Apr 5 12:41:38 UTC 2016


On Tue, Apr 05, 2016 at 01:15:24PM +0200, Ladi Prosek wrote:
> On Sun, Apr 3, 2016 at 1:50 PM, Michael S. Tsirkin <mst at redhat.com> wrote:
> > On Mon, Mar 14, 2016 at 03:48:25PM +0100, Ladi Prosek wrote:
> >> This commit makes virtio-net support devices with VEN 0x1af4
> >> and DEV 0x1041, which is how non-transitional (modern-only)
> >> virtio-net devices are exposed on the PCI bus.
> >>
> >> Transitional devices supporting both the old 0.9.5 and new 1.0
> >> version of the virtio spec are driven using the new protocol.
> >> Legacy devices are driven using the old protocol, same as before
> >> this commit.
> >>
> >> Signed-off-by: Ladi Prosek <lprosek at redhat.com>
> >> ---
> >>  src/drivers/net/virtio-net.c | 252 +++++++++++++++++++++++++++++++++++++++++--
> >>  1 file changed, 241 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/src/drivers/net/virtio-net.c b/src/drivers/net/virtio-net.c
> >> index 1c535f7..27003bc 100644
> >> --- a/src/drivers/net/virtio-net.c
> >> +++ b/src/drivers/net/virtio-net.c
> >> @@ -88,6 +88,9 @@ struct virtnet_nic {
> >>       /** Base pio register address */
> >>       unsigned long ioaddr;
> >>
> >> +     /** Virtio 1.0 device data */
> >> +     struct virtio_pci_modern_device vdev;
> >> +
> >>       /** RX/TX virtqueues */
> >>       struct vring_virtqueue *virtqueue;
> >>
> >> @@ -98,7 +101,7 @@ struct virtnet_nic {
> >>       unsigned int rx_num_iobufs;
> >>
> >>       /** Virtio net packet header, we only need one */
> >> -     struct virtio_net_hdr empty_header;
> >> +     struct virtio_net_hdr_modern empty_header;
> >>  };
> >>
> >>  /** Add an iobuf to a virtqueue
> >> @@ -115,6 +118,9 @@ static void virtnet_enqueue_iob ( struct net_device *netdev,
> >>       struct vring_virtqueue *vq = &virtnet->virtqueue[vq_idx];
> >>       unsigned int out = ( vq_idx == TX_INDEX ) ? 2 : 0;
> >>       unsigned int in = ( vq_idx == TX_INDEX ) ? 0 : 2;
> >> +     size_t header_len = virtnet->ioaddr
> >> +             ? sizeof ( virtnet->empty_header.legacy )
> >> +             : sizeof ( virtnet->empty_header );
> >>       struct vring_list list[] = {
> >>               {
> >>                       /* Share a single zeroed virtio net header between all
> >
> > it would be cleaner to check a feature bit.
> 
> Not sure what exactly you mean. We don't enable any features so we
> know that the header is unused.

You acknowledge VIRTIO_1 feature, don't you?

> I can save the accepted features
> somewhere and recheck them here but it would be basically just an
> assert.

I mean something like this:

+     size_t header_len = virtnet->virtio1
+             ? sizeof ( virtnet->empty_header )
+             : sizeof ( virtnet->empty_header.legacy );


> >> @@ -123,7 +129,7 @@ static void virtnet_enqueue_iob ( struct net_device *netdev,
> >>                        * header fields get used.
> >>                        */
> >>                       .addr = ( char* ) &virtnet->empty_header,
> >> -                     .length = sizeof ( virtnet->empty_header ),
> >> +                     .length = header_len,
> >>               },
> >>               {
> >>                       .addr = ( char* ) iobuf->data,
> >> @@ -135,7 +141,7 @@ static void virtnet_enqueue_iob ( struct net_device *netdev,
> >>               virtnet, iobuf, vq_idx );
> >>
> >>       vring_add_buf ( vq, list, out, in, iobuf, 0 );
> >> -     vring_kick ( NULL, virtnet->ioaddr, vq, 1 );
> >> +     vring_kick ( &virtnet->vdev, virtnet->ioaddr, vq, 1 );
> >>  }
> >>
> >>  /** Try to keep rx virtqueue filled with iobufs
> >> @@ -164,12 +170,12 @@ static void virtnet_refill_rx_virtqueue ( struct net_device *netdev ) {
> >>       }
> >>  }
> >>
> >> -/** Open network device
> >> +/** Open network device, legacy virtio 0.9.5
> >>   *
> >>   * @v netdev Network device
> >>   * @ret rc   Return status code
> >>   */
> >> -static int virtnet_open ( struct net_device *netdev ) {
> >> +static int virtnet_open_legacy ( struct net_device *netdev ) {
> >>       struct virtnet_nic *virtnet = netdev->priv;
> >>       unsigned long ioaddr = virtnet->ioaddr;
> >>       u32 features;
> >> @@ -210,6 +216,77 @@ static int virtnet_open ( struct net_device *netdev ) {
> >>       return 0;
> >>  }
> >>
> >> +/** Open network device, modern virtio 1.0
> >> + *
> >> + * @v netdev Network device
> >> + * @ret rc   Return status code
> >> + */
> >> +static int virtnet_open_modern ( struct net_device *netdev ) {
> >> +     struct virtnet_nic *virtnet = netdev->priv;
> >> +     u64 features;
> >> +     u8 status;
> >> +
> >> +     /* Negotiate features */
> >> +     features = vpm_get_features ( &virtnet->vdev );

You should also bail out if VIRTIO_F_VERSION_1 isn't set.
Maybe same for VIRTIO_NET_F_MAC if not offered if you depend on it
(do you?)

> >> +     vpm_set_features ( &virtnet->vdev, features & (
> >> +             ( 1ULL << VIRTIO_NET_F_MAC ) |
> >> +             ( 1ULL << VIRTIO_F_VERSION_1 ) |
> >> +             ( 1ULL << VIRTIO_F_ANY_LAYOUT ) ) );
> >> +     vpm_add_status ( &virtnet->vdev, VIRTIO_CONFIG_S_FEATURES_OK );
> >> +
> >> +     status = vpm_get_status ( &virtnet->vdev );
> >> +     if ( ! ( status & VIRTIO_CONFIG_S_FEATURES_OK ) ) {
> >> +             DBGC ( virtnet, "VIRTIO-NET %p device didn't accept features\n",
> >> +                    virtnet );
> >> +             vpm_add_status ( &virtnet->vdev, VIRTIO_CONFIG_S_FAILED );
> >> +             return -EINVAL;
> >> +     }
> >> +
> >> +     /* Allocate virtqueues */
> >> +     virtnet->virtqueue = zalloc ( QUEUE_NB *
> >> +                                   sizeof ( *virtnet->virtqueue ) );
> >> +     if ( ! virtnet->virtqueue ) {
> >> +             vpm_add_status ( &virtnet->vdev, VIRTIO_CONFIG_S_FAILED );
> >> +             return -ENOMEM;
> >> +     }
> >> +
> >> +     /* Initialize rx/tx virtqueues */
> >> +     if ( vpm_find_vqs ( &virtnet->vdev, QUEUE_NB, virtnet->virtqueue ) ) {
> >> +             DBGC ( virtnet, "VIRTIO-NET %p cannot register queues\n",
> >> +                    virtnet );
> >> +             free ( virtnet->virtqueue );
> >> +             virtnet->virtqueue = NULL;
> >> +             vpm_add_status ( &virtnet->vdev, VIRTIO_CONFIG_S_FAILED );
> >> +             return -ENOENT;
> >> +     }
> >> +
> >> +     /* Disable interrupts before starting */
> >> +     netdev_irq ( netdev, 0 );
> >> +
> >> +     vpm_add_status ( &virtnet->vdev, VIRTIO_CONFIG_S_DRIVER_OK );
> >> +
> >> +     /* Initialize rx packets */
> >> +     INIT_LIST_HEAD ( &virtnet->rx_iobufs );
> >> +     virtnet->rx_num_iobufs = 0;
> >> +     virtnet_refill_rx_virtqueue ( netdev );
> >> +     return 0;
> >> +}
> >> +
> >> +/** Open network device
> >> + *
> >> + * @v netdev Network device
> >> + * @ret rc   Return status code
> >> + */
> >> +static int virtnet_open ( struct net_device *netdev ) {
> >> +     struct virtnet_nic *virtnet = netdev->priv;
> >> +
> >> +     if ( virtnet->ioaddr ) {
> >> +             return virtnet_open_legacy ( netdev );
> >> +     } else {
> >> +             return virtnet_open_modern ( netdev );
> >> +     }
> >> +}
> >> +
> >>  /** Close network device
> >>   *
> >>   * @v netdev Network device
> >> @@ -218,10 +295,19 @@ static void virtnet_close ( struct net_device *netdev ) {
> >>       struct virtnet_nic *virtnet = netdev->priv;
> >>       struct io_buffer *iobuf;
> >>       struct io_buffer *next_iobuf;
> >> +     int i;
> >>
> >> -     vp_reset ( virtnet->ioaddr );
> >> +     if ( virtnet->ioaddr ) {
> >> +             vp_reset ( virtnet->ioaddr );
> >> +     } else {
> >> +             vpm_reset ( &virtnet->vdev );
> >> +     }
> >>
> >>       /* Virtqueues can be freed now that NIC is reset */
> >> +     for ( i = 0 ; i < QUEUE_NB ; i++ ) {
> >> +             virtio_pci_unmap_capability ( &virtnet->virtqueue[i].notification );
> >> +     }
> >> +
> >>       free ( virtnet->virtqueue );
> >>       virtnet->virtqueue = NULL;
> >>
> >> @@ -302,10 +388,14 @@ static void virtnet_poll ( struct net_device *netdev ) {
> >>
> >>       /* Acknowledge interrupt.  This is necessary for UNDI operation and
> >>        * interrupts that are raised despite VRING_AVAIL_F_NO_INTERRUPT being
> >> -      * set (that flag is just a hint and the hypervisor not not have to
> >> +      * set (that flag is just a hint and the hypervisor does not have to
> >>        * honor it).
> >>        */
> >> -     vp_get_isr ( virtnet->ioaddr );
> >> +     if ( virtnet->ioaddr ) {
> >> +             vp_get_isr ( virtnet->ioaddr );
> >> +     } else {
> >> +             vpm_get_isr ( &virtnet->vdev );
> >> +     }
> >>
> >>       virtnet_process_tx_packets ( netdev );
> >>       virtnet_process_rx_packets ( netdev );
> >> @@ -338,13 +428,12 @@ static struct net_device_operations virtnet_operations = {
> >>  };
> >>
> >>  /**
> >> - * Probe PCI device
> >> + * Probe PCI device, legacy virtio 0.9.5
> >>   *
> >>   * @v pci    PCI device
> >> - * @v id     PCI ID
> >>   * @ret rc   Return status code
> >>   */
> >> -static int virtnet_probe ( struct pci_device *pci ) {
> >> +static int virtnet_probe_legacy ( struct pci_device *pci ) {
> >>       unsigned long ioaddr = pci->ioaddr;
> >>       struct net_device *netdev;
> >>       struct virtnet_nic *virtnet;
> >> @@ -395,12 +484,152 @@ static int virtnet_probe ( struct pci_device *pci ) {
> >>  }
> >>
> >>  /**
> >> + * Probe PCI device, modern virtio 1.0
> >> + *
> >> + * @v pci    PCI device
> >> + * @v found_dev      Set to non-zero if modern device was found (probe may still fail)
> >> + * @ret rc   Return status code
> >> + */
> >> +static int virtnet_probe_modern ( struct pci_device *pci, int *found_dev ) {
> >> +     struct net_device *netdev;
> >> +     struct virtnet_nic *virtnet;
> >> +     u64 features;
> >> +     int rc, common, isr, notify, config, device;
> >> +
> >> +     common = virtio_pci_find_capability ( pci, VIRTIO_PCI_CAP_COMMON_CFG );
> >> +     if ( ! common ) {
> >> +             DBG ( "Common virtio capability not found!\n" );
> >> +             return -ENODEV;
> >> +     }
> >> +     *found_dev = 1;
> >> +
> >> +     isr = virtio_pci_find_capability ( pci, VIRTIO_PCI_CAP_ISR_CFG );
> >> +     notify = virtio_pci_find_capability ( pci, VIRTIO_PCI_CAP_NOTIFY_CFG );
> >> +     config = virtio_pci_find_capability ( pci, VIRTIO_PCI_CAP_PCI_CFG );
> >> +     if ( ! isr || ! notify || ! config ) {
> >> +             DBG ( "Missing virtio capabilities %i/%i/%i/%i\n",
> >> +                   common, isr, notify, config );
> >> +             return -EINVAL;
> >> +     }
> >> +     device = virtio_pci_find_capability ( pci, VIRTIO_PCI_CAP_DEVICE_CFG );
> >> +
> >> +     /* Allocate and hook up net device */
> >> +     netdev = alloc_etherdev ( sizeof ( *virtnet ) );
> >> +     if ( ! netdev )
> >> +             return -ENOMEM;
> >> +     netdev_init ( netdev, &virtnet_operations );
> >> +     virtnet = netdev->priv;
> >> +
> >> +     pci_set_drvdata ( pci, netdev );
> >> +     netdev->dev = &pci->dev;
> >> +
> >> +     DBGC ( virtnet, "VIRTIO-NET modern %p busaddr=%s irq=%d\n",
> >> +            virtnet, pci->dev.name, pci->irq );
> >> +
> >> +     virtnet->vdev.pci = pci;
> >> +     rc = virtio_pci_map_capability ( pci, common,
> >> +             sizeof ( struct virtio_pci_common_cfg ), 4,
> >> +             0, sizeof ( struct virtio_pci_common_cfg ),
> >> +             &virtnet->vdev.common );
> >> +     if ( rc )
> >> +             goto err_map_common;
> >> +
> >> +     rc = virtio_pci_map_capability ( pci, isr, sizeof ( u8 ), 1,
> >> +             0, 1,
> >> +             &virtnet->vdev.isr );
> >> +     if ( rc )
> >> +             goto err_map_isr;
> >> +
> >> +     /* Read notify_off_multiplier from config space. */
> >> +     pci_read_config_dword ( pci,
> >> +             notify + offsetof ( struct virtio_pci_notify_cap,
> >> +             notify_off_multiplier ),
> >> +             &virtnet->vdev.notify_offset_multiplier );
> >> +
> >> +     virtnet->vdev.notify_cap_pos = notify;
> >> +     virtnet->vdev.cfg_cap_pos = config;
> >
> >
> > move this chunk to generic code, where it's used?
> 
> Yes, makes sense, will do.
> 
> >> +
> >> +     /* Map the device capability */
> >> +     if ( device ) {
> >> +             rc = virtio_pci_map_capability ( pci, device,
> >> +                     0, 4, 0, sizeof ( struct virtio_net_config ),
> >> +                     &virtnet->vdev.device );
> >> +             if ( rc )
> >> +                     goto err_map_device;
> >> +     }
> >> +
> >> +     /* Enable PCI bus master and reset NIC */
> >> +     adjust_pci_device ( pci );
> >> +
> >> +     /* Reset the device and set initial status bits */
> >> +     vpm_reset ( &virtnet->vdev );
> >> +     vpm_add_status ( &virtnet->vdev, VIRTIO_CONFIG_S_ACKNOWLEDGE );
> >> +     vpm_add_status ( &virtnet->vdev, VIRTIO_CONFIG_S_DRIVER );
> >> +
> >> +     /* Load MAC address */
> >> +     if ( device ) {
> >
> > If device config is not accessible, I think
> > you should not negotiate mac. Correct?
> 
> Right, device would be 0 in such case so we will skip the MAC code.
> 
> >> +             features = vpm_get_features ( &virtnet->vdev );
> >> +             if ( features & ( 1ULL << VIRTIO_NET_F_MAC ) ) {
> >> +                     vpm_get ( &virtnet->vdev,
> >> +                               offsetof ( struct virtio_net_config, mac ),
> >> +                               netdev->hw_addr, ETH_ALEN );
> >
> > do things work without a MAC? If not - fail early?
> 
> I thought this was handled by register_netdev but it looks like it's
> not. I'll add an explicit MAC validity check.
> 
> >> +                     DBGC ( virtnet, "VIRTIO-NET %p mac=%s\n", virtnet,
> >> +                            eth_ntoa ( netdev->hw_addr ) );
> >> +             }
> >> +     }
> >> +
> >> +     /* Register network device */
> >> +     if ( ( rc = register_netdev ( netdev ) ) != 0 )
> >> +             goto err_register_netdev;
> >> +
> >> +     /* Mark link as up, control virtqueue is not used */
> >> +     netdev_link_up ( netdev );
> >> +
> >> +     return 0;
> >> +
> >> +     unregister_netdev ( netdev );
> >> + err_register_netdev:
> >> +     vpm_reset ( &virtnet->vdev );
> >> +     netdev_nullify ( netdev );
> >> +     netdev_put ( netdev );
> >> +
> >> +     virtio_pci_unmap_capability ( &virtnet->vdev.device );
> >> +err_map_device:
> >> +     virtio_pci_unmap_capability ( &virtnet->vdev.isr );
> >> +err_map_isr:
> >> +     virtio_pci_unmap_capability ( &virtnet->vdev.common );
> >> +err_map_common:
> >> +     return rc;
> >> +}
> >> +
> >> +/**
> >> + * Probe PCI device
> >> + *
> >> + * @v pci    PCI device
> >> + * @ret rc   Return status code
> >> + */
> >> +static int virtnet_probe ( struct pci_device *pci ) {
> >> +     int found_modern = 0;
> >> +     int rc = virtnet_probe_modern ( pci, &found_modern );
> >> +     if ( ! found_modern ) {
> >> +             /* fall back to the legacy probe */
> >
> > You also must not do this for devices with modern device id.
> 
> Oops, thanks, will fix.
> 
> >> +             rc = virtnet_probe_legacy ( pci );
> >> +     }
> >> +     return rc;
> >> +}
> >> +
> >> +/**
> >>   * Remove device
> >>   *
> >>   * @v pci    PCI device
> >>   */
> >>  static void virtnet_remove ( struct pci_device *pci ) {
> >>       struct net_device *netdev = pci_get_drvdata ( pci );
> >> +     struct virtnet_nic *virtnet = netdev->priv;
> >> +
> >> +     virtio_pci_unmap_capability ( &virtnet->vdev.device );
> >
> >
> > this will unmap even if device was not set.
> 
> That's fine, uninitialized regions have flags=0 so
> virtio_pci_unmap_capability will be a no-op.
> 
> >> +     virtio_pci_unmap_capability ( &virtnet->vdev.isr );
> >> +     virtio_pci_unmap_capability ( &virtnet->vdev.common );
> >>
> >>       unregister_netdev ( netdev );
> >>       netdev_nullify ( netdev );
> >> @@ -409,6 +638,7 @@ static void virtnet_remove ( struct pci_device *pci ) {
> >>
> >>  static struct pci_device_id virtnet_nics[] = {
> >>  PCI_ROM(0x1af4, 0x1000, "virtio-net", "Virtio Network Interface", 0),
> >> +PCI_ROM(0x1af4, 0x1041, "virtio-net", "Virtio Network Interface 1.0", 0),
> >>  };
> >>
> >>  struct pci_driver virtnet_driver __pci_driver = {
> >> --
> >> 2.5.0



More information about the ipxe-devel mailing list