[libvirt] [PATCH 3/3] qemu: add usb-net support

[Laine Stump]

On 01/03/2013 02:13 AM, Guannan Ren wrote:
> Libvirt XML sample:
>   <devices>
>     <interface type='user'>
>       <mac address='52:54:00:32:6a:91'/>
>       <model type='usb-net'/>
>       <alias name='net1'/>
>       <address type='usb' bus='0' port='1'/>
>     </interface>
>   </devices>
>
> qemu commandline:
>
> qemu ${other_vm_args}
>     -netdev user,id=hostnet1 \
>     -device usb-net,netdev=hostnet1,id=net1,\
>       mac=52:54:00:32:6a:91,bus=usb.0,port=1
> ---
>  docs/formatdomain.html.in |  4 +++-
>  src/conf/domain_conf.c    | 18 +++++++++++++-----
>  src/qemu/qemu_command.c   |  8 +++++++-
>  3 files changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 45d7593..fbd77f2 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -2952,7 +2952,9 @@ qemu-kvm -net nic,model=? /dev/null
>  
>      <p>
>        Typical values for QEMU and KVM include:
> -      ne2k_isa i82551 i82557b i82559er ne2k_pci pcnet rtl8139 e1000 virtio
> +      ne2k_isa i82551 i82557b i82559er ne2k_pci pcnet rtl8139 e1000 virtio.
> +      Besides above, there is a model of usb-net which is a QEMU USB Network interface

  ne2k_isa i82551 i82557b i82559er ne2k_pci pcnet rtl8139 e1000 virtio XXXX.
  (All of these models are PCI devices, except "XXXX" which emulates a
  $manufacturers-name XXXX usb ethernet device.

(A bit off topice, but it's actually kind of starting to bother me that
we just blindly pass-through all the other netdev models rather than
validating them...)

> +      <span class="since">Since 1.0.2

   ) 

  [so that it's obvious the "since 1.0.2" applies only to the usb device]

>  (QEMU and KVM only)</span>
>      </p>
>  
>      <h5><a name="elementsDriverBackendOptions">Setting NIC driver-specific options</a></h5>


You haven't mentioned anything about the (new) possibility of an
<interface> having <address type='usb' .../> (of course there is no
specific place where it's mentioned that it can have an <address
type='pci' .../> either, so I'm not sure where is the best place to put
that).


> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 70c1ee7..94cd059 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5131,14 +5131,15 @@ virDomainNetDefParseXML(virCapsPtr caps,
>              goto error;
>      }
>  
> -    /* XXX what about ISA/USB based NIC models - once we support
> +    /* XXX what about ISA based NIC models - once we support
>       * them we should make sure address type is correct */
>      if (def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
>          def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO &&
>          def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390 &&
> -        def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
> +        def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
> +        def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB) {
>          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                       _("Network interfaces must use 'pci' address type"));
> +                       _("Network interfaces have incorrect address type"));


>          goto error;
>      }
>  
> @@ -5301,9 +5302,16 @@ virDomainNetDefParseXML(virCapsPtr caps,
>              virReportError(VIR_ERR_INVALID_ARG, "%s",
>                             _("Model name contains invalid characters"));
>              goto error;
> +        } else if (STREQ(model, "usb-net") &&
> +                   def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
> +                   def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("Interface of usb-net model requires address of usb type"));

This is technically correct, but the existing check of detail
consistency for virtio devices is done down below, after def->model is
already set, rather than combining it into this basic sanity check. That
would allow more sanity checks related to this type of device to be
added later, without obscuring the character set sanity checking that's
being done here.


> +            goto error;
> +        } else {
> +            def->model = model;
> +            model = NULL;
>          }
> -        def->model = model;
> -        model = NULL;
>      }
>  
>      if (def->model && STREQ(def->model, "virtio")) {
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 8a3de09..b3ce76a 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1558,7 +1558,8 @@ qemuAssignDevicePCISlots(virDomainDefPtr def,
>           * instead of here.
>           */
>          if ((def->nets[i]->type == VIR_DOMAIN_NET_TYPE_HOSTDEV) ||
> -            (def->nets[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)) {
> +            (def->nets[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) ||
> +            (STREQ_NULLABLE(def->nets[i]->model, "usb-net"))) {

Hmm. It would be nicer to be able to just check for info.type == PCI,
but of course on the initial run that isn't set yet. I guess at some
place this type of device (whatever we end up calling it) needs to be
determined to be a USB device and we need to assign it an address. How
is this done for hostdevs? (I haven't looked, I've only done things with
PCI hostdevs)


>              continue;
>          }
>          if (qemuDomainPCIAddressSetNextAddr(addrs, &def->nets[i]->info) < 0)
> @@ -3180,6 +3181,11 @@ qemuBuildNicDevStr(virDomainNetDefPtr net,
>              nic = "virtio-net-pci";
>          }
>          usingVirtio = true;
> +    } else if (STREQ(net->model, "usb-net") &&
> +               !qemuCapsGet(caps, QEMU_CAPS_DEVICE_USB_NET)) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("usb-net is not supported in this QEMU binary"));
> +        goto error;
>      } else {
>          nic = net->model;
>      }