Re: [Qemu-devel] [PATCH] net: Allow hubports to connect to other netdevs

[Thomas Huth]

On 10.01.2018 15:47, Paolo Bonzini wrote:
> On 10/01/2018 15:32, Thomas Huth wrote:
>> QEMU can emulate hubs to connect NICs and netdevs. This is currently
>> primarily used for the mis-named 'vlan' feature of the networking
>> subsystem. Now the 'vlan' feature has been marked as deprecated, since
>> its name is rather confusing and the users often rather mis-configure
>> their network when trying to use it. But while the 'vlan' parameter
>> should be removed at one point in time, the basic idea of emulating
>> a hub in QEMU is still good: It's useful for bundling up the output of
>> multiple NICs into one single l2tp netdev for example.
>>
>> Now to be able to use the hubport feature without 'vlan's, there is one
>> missing piece: The possibility to connect a hubport to a netdev, too.
>> This patch adds this possibility by introducing a new "netdev=..."
>> parameter to the hubports.
>>
>> To bundle up the output of multiple NICs into one socket netdev, you can
>> now run QEMU with these parameters for example:
>>
>> qemu-system-ppc64 ... -netdev socket,id=s1,connect=:11122 \
>>     -netdev hubport,hubid=1,id=h1,netdev=s1 \
>>     -netdev hubport,hubid=1,id=h2 -device e1000,netdev=h2 \
>>     -netdev hubport,hubid=1,id=h3 -device virtio-net-pci,netdev=h3
>>
>> For using the socket netdev, you have got to start another QEMU as the
>> receiving side first, for example with network dumping enabled:
>>
>> qemu-system-x86_64 -M isapc -netdev socket,id=s0,listen=:11122 \
>>     -device ne2k_isa,netdev=s0 \
>>     -object filter-dump,id=f1,netdev=s0,file=/tmp/dump.dat
>>
>> After the ppc64 guest tried to boot from both NICs, you can see in the
>> dump file (using Wireshark, for example), that the output of both NICs
>> (the e1000 and the virtio-net-pci) has been successfully transfered
>> via the socket netdev in this case.
>>
>> Suggested-by: Paolo Bonzini <address@hidden>
>> Signed-off-by: Thomas Huth <address@hidden>
>> ---
>>  See also the original discussion here for some more information:
>>  https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg05650.html
>>
>>  net/hub.c       | 23 ++++++++++++++++++++++-
>>  qapi/net.json   |  4 +++-
>>  qemu-options.hx |  8 +++++---
>>  3 files changed, 30 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/hub.c b/net/hub.c
>> index 14b4eec..0638729 100644
>> --- a/net/hub.c
>> +++ b/net/hub.c
>> @@ -13,6 +13,7 @@
>>   */
>>  
>>  #include "qemu/osdep.h"
>> +#include "qapi/error.h"
>>  #include "monitor/monitor.h"
>>  #include "net/net.h"
>>  #include "clients.h"
>> @@ -286,12 +287,32 @@ int net_init_hubport(const Netdev *netdev, const char 
>> *name,
>>                       NetClientState *peer, Error **errp)
>>  {
>>      const NetdevHubPortOptions *hubport;
>> +    NetClientState *hubncs;
>>  
>>      assert(netdev->type == NET_CLIENT_DRIVER_HUBPORT);
>>      assert(!peer);
>>      hubport = &netdev->u.hubport;
>>  
>> -    net_hub_add_port(hubport->hubid, name);
>> +    hubncs = net_hub_add_port(hubport->hubid, name);
>> +    if (!hubncs) {
>> +        error_setg(errp, "failed to add port to hub %i with id '%s'",
>> +                   hubport->hubid, name);
>> +        return -1;
>> +    }
>> +
>> +    if (hubport->has_netdev) {
>> +        NetClientState *hubpeer;
>> +
>> +        hubpeer = qemu_find_netdev(hubport->netdev);
>> +        if (!hubpeer) {
>> +            error_setg(errp, "netdev '%s' not found", hubport->netdev);
>> +            return -1;
>> +        }
>> +        assert(!hubncs->peer && !hubpeer->peer);
>> +        hubncs->peer = hubpeer;
>> +        hubpeer->peer = hubncs;
>> +    }
>> +
>>      return 0;
>>  }
>>  
>> diff --git a/qapi/net.json b/qapi/net.json
>> index 4beff5d..e41e046 100644
>> --- a/qapi/net.json
>> +++ b/qapi/net.json
>> @@ -410,12 +410,14 @@
>>  # Connect two or more net clients through a software hub.
>>  #
>>  # @hubid: hub identifier number
>> +# @netdev: used to connect hub to a netdev instead of a device (since 2.12)
>>  #
>>  # Since: 1.2
>>  ##
>>  { 'struct': 'NetdevHubPortOptions',
>>    'data': {
>> -    'hubid':     'int32' } }
>> +    'hubid':     'int32',
>> +    '*netdev':    'str' } }
>>  
>>  ##
>>  # @NetdevNetmapOptions:
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index 678181c..9ec4af7 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -2017,7 +2017,7 @@ DEF("netdev", HAS_ARG, QEMU_OPTION_netdev,
>>  #endif
>>      "-netdev vhost-user,id=str,chardev=dev[,vhostforce=on|off]\n"
>>      "                configure a vhost-user network, backed by a chardev 
>> 'dev'\n"
>> -    "-netdev hubport,id=str,hubid=n\n"
>> +    "-netdev hubport,id=str,hubid=n[,netdev=nd]\n"
>>      "                configure a hub port on QEMU VLAN 'n'\n", 
>> QEMU_ARCH_ALL)
>>  DEF("net", HAS_ARG, QEMU_OPTION_net,
>>      "-net 
>> nic[,vlan=n][,netdev=nd][,macaddr=mac][,model=type][,name=str][,addr=str][,vectors=v]\n"
>> @@ -2445,13 +2445,15 @@ vde_switch -F -sock /tmp/myswitch
>>  qemu-system-i386 linux.img -net nic -net vde,sock=/tmp/myswitch
>>  @end example
>>  
>> address@hidden -netdev hubport,address@hidden,address@hidden
>> address@hidden -netdev hubport,address@hidden,address@hidden,address@hidden
>>  
>>  Create a hub port on QEMU "vlan" @var{hubid}.
>>  
>>  The hubport netdev lets you connect a NIC to a QEMU "vlan" instead of a 
>> single
>>  netdev.  @code{-net} and @code{-device} with parameter @option{vlan} create 
>> the
>> -required hub automatically.
>> +required hub automatically. Alternatively, you can also connect the hubport
>> +to another netdev with ID @var{nd} by using the @address@hidden
>> +option.
>>  
>>  @item -netdev vhost-user,address@hidden,vhostforce=on|off][,queues=n]
> 
> Thank you very much!
> 
> What is the next step for deprecating vlans?  I guess
> 
> 1) removing the vlan property from DEFINE_NIC_PROPERTIES
> 
> 2) removing the vlan property from -net, defaulting to vlan 0

Yes.

> 3) only allow exactly two -net options, one for the backend (creating a
> netdev) and one from the front-end (filling in nd_table)

I see multiple options here:

a) Only allow one pair of -net options. That's likely bad if there is a
board with more than one embedded NIC, since "-net" is the only way to
configure them right now.

b) Keep the current way (just without the 'vlan' parameter), so that all
-net front- and back-ends are always connected to "vlan 0". We'd stay as
compatible as possible this way, but actually this is also the way that
regularly confuses the users - they do not expect that everything ends
up on one hub!

c) Introduce a slightly new behaviour, so that if you specify -net
nic,id=a1 -net user,id=a2 -net nic,id=b1 -net user,id=b2 then QEMU
connects a1 with a2, and b1 with b2 directly, without a hub in between.
That's likely the behaviour that most users expect, but it's also a
slight change in behavior compared to the previous QEMU versions.

d) Finally get rid of the ugly "-net" parameter (or maybe rather limit
it to option a) and introduce a new "-n" convenience parameter instead
which can be used to configure the default/embedded NICs. With that new
convenience parameter, you don't have to specify it twice anymore like
in "-net nic -net tap", but you could simply specify "-n tap" and QEMU
then configures a tap interface for the default NIC of the current
machine for you. (I'd drop the possibility to change the NIC model here,
but in case it is still required, I'd go for something like "-n
tap,model=e1000" or so)

Opinions?

 Thomas