[libvirt] [PATCH v3 2/2] bhyve: add e1000 nic support
Laine Stump
laine at laine.org
Mon Nov 14 17:43:14 UTC 2016
On 11/07/2016 09:20 AM, Roman Bogorodskiy wrote:
> Recently e1000 NIC support was added to bhyve; implement that in
> the bhyve driver:
>
> - Add capability check by analyzing output of the 'bhyve -s 0,e1000'
> command
> - Modify bhyveBuildNetArgStr() to support e1000 and also pass
> virConnectPtr so it could call bhyveDriverGetCaps() to check if this
> NIC is supported
> - Modify command parsing code to add support for e1000 and adjust tests
> - Add net-e1000 test
> ---
> src/bhyve/bhyve_capabilities.c | 27 ++++++++
> src/bhyve/bhyve_capabilities.h | 1 +
> src/bhyve/bhyve_command.c | 74 ++++++++++++++--------
> src/bhyve/bhyve_parse_command.c | 9 ++-
> tests/bhyveargv2xmldata/bhyveargv2xml-e1000.args | 8 +++
> tests/bhyveargv2xmldata/bhyveargv2xml-e1000.xml | 28 ++++++++
> .../bhyveargv2xmldata/bhyveargv2xml-virtio-net.xml | 2 +
> .../bhyveargv2xml-virtio-net4.xml | 1 +
> tests/bhyveargv2xmltest.c | 1 +
> .../bhyvexml2argvdata/bhyvexml2argv-net-e1000.args | 9 +++
> .../bhyvexml2argv-net-e1000.ldargs | 3 +
> .../bhyvexml2argvdata/bhyvexml2argv-net-e1000.xml | 22 +++++++
> tests/bhyvexml2argvtest.c | 7 +-
> 13 files changed, 162 insertions(+), 30 deletions(-)
> create mode 100644 tests/bhyveargv2xmldata/bhyveargv2xml-e1000.args
> create mode 100644 tests/bhyveargv2xmldata/bhyveargv2xml-e1000.xml
> create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-net-e1000.args
> create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-net-e1000.ldargs
> create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-net-e1000.xml
>
> diff --git a/src/bhyve/bhyve_capabilities.c b/src/bhyve/bhyve_capabilities.c
> index be68e51..3012788 100644
> --- a/src/bhyve/bhyve_capabilities.c
> +++ b/src/bhyve/bhyve_capabilities.c
> @@ -192,6 +192,30 @@ bhyveProbeCapsRTC_UTC(unsigned int *caps, char *binary)
> return ret;
> }
>
> +static int
> +bhyveProbeCapsNetE1000(unsigned int *caps, char *binary)
> +{
> + char *error;
> + virCommandPtr cmd = NULL;
> + int ret = 0, exit;
Again, more consistent with the rest of the code to initialize ret = -1...
> +
> + cmd = virCommandNew(binary);
> + virCommandAddArgList(cmd, "-s", "0,e1000", NULL);
> + virCommandSetErrorBuffer(cmd, &error);
> + if (virCommandRun(cmd, &exit) < 0) {
> + ret = -1;
...remove this line...
> + goto out;
> + }
> +
> + if (strstr(error, "pci slot 0:0: unknown device \"e1000\"") == 0)
If you're going to check the return of strstr(), it's more "pure" to
check "== NULL", since the return is a pointer. Or even better (and what
libvirt does in most cases) change the condition into:
if (!strstr(errror, .......))
> + *caps |= BHYVE_CAP_NET_E1000;
> +
> + out:
... and change the above label to "cleanup:" with a "ret = 0;" just
above it.
> + VIR_FREE(error);
> + virCommandFree(cmd);
> + return ret;
> +}
> +
> int
> virBhyveProbeCaps(unsigned int *caps)
> {
> @@ -207,6 +231,9 @@ virBhyveProbeCaps(unsigned int *caps)
> if ((ret = bhyveProbeCapsRTC_UTC(caps, binary)))
> goto out;
>
> + if ((ret = bhyveProbeCapsNetE1000(caps, binary)))
> + goto out;
Following the advice from the previous patch, this will be:
if (bhyveProbeCapsNetE1000(caps, binary) < 0)
goto cleanup;
> +
> out:
> VIR_FREE(binary);
> return ret;
> diff --git a/src/bhyve/bhyve_capabilities.h b/src/bhyve/bhyve_capabilities.h
> index 8e7646d..d6c7bb0 100644
> --- a/src/bhyve/bhyve_capabilities.h
> +++ b/src/bhyve/bhyve_capabilities.h
> @@ -38,6 +38,7 @@ typedef enum {
>
> typedef enum {
> BHYVE_CAP_RTC_UTC = 1,
> + BHYVE_CAP_NET_E1000 = 2,
Since these are bits in an int, and not values, you should make that
painfully obvious like this:
BHYVE_CAP_RTC_UTC = 1 << 0;
BHYVE_CAP_NET_E1000 = 1 << 1;
(of course, if the future maps out like I predict, they'll eventually
end up being regular incremental values anyway, so they can be used as
arguments to virBitmap functions - that will be necessary as soon as you
encounter "capability #33".)
> } virBhyveCapsFlags;
>
> int virBhyveProbeGrubCaps(virBhyveGrubCapsFlags *caps);
> diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
> index 022b03b..cfb8b45 100644
> --- a/src/bhyve/bhyve_command.c
> +++ b/src/bhyve/bhyve_command.c
> @@ -44,7 +44,8 @@
> VIR_LOG_INIT("bhyve.bhyve_command");
>
> static int
> -bhyveBuildNetArgStr(const virDomainDef *def,
> +bhyveBuildNetArgStr(virConnectPtr conn,
qemu functions tend to pass around a virQEMUCapsPtr or driver ptr rather
than a conn...[*]
> + const virDomainDef *def,
> virDomainNetDefPtr net,
> virCommandPtr cmd,
> bool dryRun)
> @@ -52,26 +53,46 @@ bhyveBuildNetArgStr(const virDomainDef *def,
> char macaddr[VIR_MAC_STRING_BUFLEN];
> char *realifname = NULL;
> char *brname = NULL;
> + char *nic_model = NULL;
> + int ret = -1;
> virDomainNetType actualType = virDomainNetGetActualType(net);
>
> + if (STREQ(net->model, "virtio")) {
> + if (VIR_STRDUP(nic_model, "virtio-net") < 0)
> + return -1;
> + } else if (STREQ(net->model, "e1000")) {
(Someday, "someone" needs to change net->model into an enum. It will
involve changing things in all the hypervisor drivers though, which
raises the likelyhood of "someone" breaking a hypervisor driver they
aren't equipped to test :-/)
> + if ((bhyveDriverGetCaps(conn) & BHYVE_CAP_NET_E1000) != 0) {
[*]... and as a matter of fact, since bhyveDriverGetCaps execs two
external binaries each time it is called, I think the capabilities
should be probed at a higher level in the call chain so it's only done
once for each guest that is started.
> + if (VIR_STRDUP(nic_model, "e1000") < 0)
> + return -1;
> + } else {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("NIC model 'e1000' is not supported "
> + "by given bhyve binary"));
> + return -1;
> + }
> + } else {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("NIC model '%s' is not supported"),
> + net->model);
> + return -1;
> + }
> +
> if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) {
> if (VIR_STRDUP(brname, virDomainNetGetActualBridgeName(net)) < 0)
Pre-existing condition, but you're not checking that
virDomainNetGetActualBridgeName() is returning non-NULL.
In the qemu and LXC drivers, that's checked while setting up interfaces,
and a missing bridge name is reported. (It should have been validated
during parse anyway, but....)
Also, after hearing that you're building the network driver for FreeBSD,
I'm surprised that this doesn't support at least the tap-based versions
of VIR_DOMAIN_NET_TYPE_NETWORK (but again, that is unrelated to the
current patch).
> - return -1;
> + goto out;
> } else {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("Network type %d is not supported"),
> virDomainNetGetActualType(net));
> - return -1;
> + goto out;
> }
>
> if (!net->ifname ||
> STRPREFIX(net->ifname, VIR_NET_GENERATED_PREFIX) ||
> strchr(net->ifname, '%')) {
> VIR_FREE(net->ifname);
> - if (VIR_STRDUP(net->ifname, VIR_NET_GENERATED_PREFIX "%d") < 0) {
> - VIR_FREE(brname);
> - return -1;
> - }
> + if (VIR_STRDUP(net->ifname, VIR_NET_GENERATED_PREFIX "%d") < 0)
> + goto out;
> }
>
> if (!dryRun) {
> @@ -79,44 +100,41 @@ bhyveBuildNetArgStr(const virDomainDef *def,
> def->uuid, NULL, NULL, 0,
> virDomainNetGetActualVirtPortProfile(net),
> virDomainNetGetActualVlan(net),
> - VIR_NETDEV_TAP_CREATE_IFUP | VIR_NETDEV_TAP_CREATE_PERSIST) < 0) {
> - VIR_FREE(net->ifname);
> - VIR_FREE(brname);
> - return -1;
> - }
> + VIR_NETDEV_TAP_CREATE_IFUP | VIR_NETDEV_TAP_CREATE_PERSIST) < 0)
Since the condition is multiple lines, don't remove the braces - the
hacking doc says you should use braces if either the body *or* the
condition are multiple lines.
> + goto out;
>
> realifname = virNetDevTapGetRealDeviceName(net->ifname);
>
> - if (realifname == NULL) {
> - VIR_FREE(net->ifname);
> - VIR_FREE(brname);
> - return -1;
> - }
> + if (realifname == NULL)
> + goto out;
We often/usually used "if (!realifname)" when checking for NULL pointers.
>
> VIR_DEBUG("%s -> %s", net->ifname, realifname);
> /* hack on top of other hack: we need to set
> * interface to 'UP' again after re-opening to find its
> * name
> */
> - if (virNetDevSetOnline(net->ifname, true) != 0) {
> - VIR_FREE(realifname);
> - VIR_FREE(net->ifname);
> - VIR_FREE(brname);
> - return -1;
> - }
> + if (virNetDevSetOnline(net->ifname, true) != 0)
> + goto out;
After several of these... Not necessary this time, but for future
reference (or if you want extra Brownie points this time :-)) - we've
found it much easier to review patches adding new functionality if other
necessary reorganization (e.g. changing all the "return -1" into "goto
cleanup" and moving all resource-freeing down to cleanup:) is put in a
separate prerequisite patch. Then the new functionality is just a simple
addition rather than a re-org + addition.
> } else {
> if (VIR_STRDUP(realifname, "tap0") < 0)
> - return -1;
> + goto out;
> }
>
>
> virCommandAddArg(cmd, "-s");
> - virCommandAddArgFormat(cmd, "%d:0,virtio-net,%s,mac=%s",
> - net->info.addr.pci.slot,
> + virCommandAddArgFormat(cmd, "%d:0,%s,%s,mac=%s",
> + net->info.addr.pci.slot, nic_model,
> realifname, virMacAddrFormat(&net->mac, macaddr));
> +
> + ret = 0;
> + out:
Change this label to "cleanup:".
> + VIR_FREE(net->ifname);
> + VIR_FREE(realifname);
> + VIR_FREE(brname);
> VIR_FREE(realifname);
> + VIR_FREE(nic_model);
>
> - return 0;
> + return ret;
> }
>
> static int
> @@ -284,7 +302,7 @@ virBhyveProcessBuildBhyveCmd(virConnectPtr conn,
> /* Devices */
> for (i = 0; i < def->nnets; i++) {
> virDomainNetDefPtr net = def->nets[i];
> - if (bhyveBuildNetArgStr(def, net, cmd, dryRun) < 0)
> + if (bhyveBuildNetArgStr(conn, def, net, cmd, dryRun) < 0)
Again, rather than passing down conn here, the capabilities should have
been probed either here or higher, and then just the capabilities sent
down to lower levels.
> goto error;
> }
> for (i = 0; i < def->ndisks; i++) {
> diff --git a/src/bhyve/bhyve_parse_command.c b/src/bhyve/bhyve_parse_command.c
> index 0ae7a83..deb3d96 100644
> --- a/src/bhyve/bhyve_parse_command.c
> +++ b/src/bhyve/bhyve_parse_command.c
> @@ -496,6 +496,7 @@ bhyveParsePCINet(virDomainDefPtr def,
> unsigned pcislot,
> unsigned pcibus,
> unsigned function,
> + const char *model,
> const char *config)
> {
> /* -s slot,virtio-net,tapN[,mac=xx:xx:xx:xx:xx:xx] */
> @@ -510,6 +511,8 @@ bhyveParsePCINet(virDomainDefPtr def,
> /* Let's just assume it is VIR_DOMAIN_NET_TYPE_ETHERNET, it could also be
> * a bridge, but this is the most generic option. */
(Preexisting, but...) That's an odd choice, since the bhyve driver only
supports VIR_DOMAIN_NET_TYPE_BRIDGE. So you're guaranteeing that the
config you generate will be unusable.
> net->type = VIR_DOMAIN_NET_TYPE_ETHERNET;
> + if (VIR_STRDUP(net->model, model) < 0)
> + goto error;
>
> net->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
> net->info.addr.pci.slot = pcislot;
> @@ -617,7 +620,11 @@ bhyveParseBhyvePCIArg(virDomainDefPtr def,
> nahcidisk,
> conf);
> else if (STREQ(emulation, "virtio-net"))
> - bhyveParsePCINet(def, xmlopt, caps, pcislot, bus, function, conf);
> + bhyveParsePCINet(def, xmlopt, caps, pcislot, bus, function,
> + "virtio", conf);
> + else if (STREQ(emulation, "e1000"))
> + bhyveParsePCINet(def, xmlopt, caps, pcislot, bus, function,
> + "e1000", conf);
>
> VIR_FREE(emulation);
> VIR_FREE(slotdef);
> diff --git a/tests/bhyveargv2xmldata/bhyveargv2xml-e1000.args b/tests/bhyveargv2xmldata/bhyveargv2xml-e1000.args
> new file mode 100644
> index 0000000..aa568fe
> --- /dev/null
> +++ b/tests/bhyveargv2xmldata/bhyveargv2xml-e1000.args
> @@ -0,0 +1,8 @@
> +/usr/sbin/bhyve \
> +-c 1 \
> +-m 214 \
> +-H \
> +-P \
> +-s 0:0,hostbridge \
> +-s 1:0,e1000,tap0 \
> +-s 1:1,e1000,tap1,mac=FE:ED:AD:EA:DF:15 bhyve
> diff --git a/tests/bhyveargv2xmldata/bhyveargv2xml-e1000.xml b/tests/bhyveargv2xmldata/bhyveargv2xml-e1000.xml
> new file mode 100644
> index 0000000..cd1ec5d
> --- /dev/null
> +++ b/tests/bhyveargv2xmldata/bhyveargv2xml-e1000.xml
> @@ -0,0 +1,28 @@
> +<domain type='bhyve'>
> + <name>bhyve</name>
> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> + <memory unit='KiB'>219136</memory>
> + <currentMemory unit='KiB'>219136</currentMemory>
> + <vcpu placement='static'>1</vcpu>
> + <os>
> + <type>hvm</type>
> + </os>
> + <clock offset='localtime'/>
> + <on_poweroff>destroy</on_poweroff>
> + <on_reboot>destroy</on_reboot>
> + <on_crash>destroy</on_crash>
> + <devices>
> + <interface type='ethernet'>
> + <mac address='52:54:00:00:00:00'/>
> + <target dev='tap0'/>
> + <model type='e1000'/>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/>
> + </interface>
> + <interface type='ethernet'>
> + <mac address='fe:ed:ad:ea:df:15'/>
> + <target dev='tap1'/>
> + <model type='e1000'/>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
> + </interface>
> + </devices>
> +</domain>
> diff --git a/tests/bhyveargv2xmldata/bhyveargv2xml-virtio-net.xml b/tests/bhyveargv2xmldata/bhyveargv2xml-virtio-net.xml
> index 09cc79b..d920a09 100644
> --- a/tests/bhyveargv2xmldata/bhyveargv2xml-virtio-net.xml
> +++ b/tests/bhyveargv2xmldata/bhyveargv2xml-virtio-net.xml
> @@ -15,11 +15,13 @@
> <interface type='ethernet'>
> <mac address='52:54:00:00:00:00'/>
> <target dev='tap0'/>
> + <model type='virtio'/>
> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/>
> </interface>
> <interface type='ethernet'>
> <mac address='fe:ed:ad:ea:df:15'/>
> <target dev='tap1'/>
> + <model type='virtio'/>
> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
> </interface>
> </devices>
> diff --git a/tests/bhyveargv2xmldata/bhyveargv2xml-virtio-net4.xml b/tests/bhyveargv2xmldata/bhyveargv2xml-virtio-net4.xml
> index e1bda46..6fbb3d2 100644
> --- a/tests/bhyveargv2xmldata/bhyveargv2xml-virtio-net4.xml
> +++ b/tests/bhyveargv2xmldata/bhyveargv2xml-virtio-net4.xml
> @@ -15,6 +15,7 @@
> <interface type='ethernet'>
> <mac address='00:00:00:00:00:00'/>
> <target dev='tap1'/>
> + <model type='virtio'/>
> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
> </interface>
> </devices>
> diff --git a/tests/bhyveargv2xmltest.c b/tests/bhyveargv2xmltest.c
> index 0995f69..e759e4f 100644
> --- a/tests/bhyveargv2xmltest.c
> +++ b/tests/bhyveargv2xmltest.c
> @@ -175,6 +175,7 @@ mymain(void)
> DO_TEST("ahci-hd");
> DO_TEST("virtio-blk");
> DO_TEST("virtio-net");
> + DO_TEST("e1000");
> DO_TEST_WARN("virtio-net2");
> DO_TEST_WARN("virtio-net3");
> DO_TEST_WARN("virtio-net4");
> diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-net-e1000.args b/tests/bhyvexml2argvdata/bhyvexml2argv-net-e1000.args
> new file mode 100644
> index 0000000..cd0e896
> --- /dev/null
> +++ b/tests/bhyvexml2argvdata/bhyvexml2argv-net-e1000.args
> @@ -0,0 +1,9 @@
> +/usr/sbin/bhyve \
> +-c 1 \
> +-m 214 \
> +-u \
> +-H \
> +-P \
> +-s 0:0,hostbridge \
> +-s 3:0,e1000,faketapdev,mac=52:54:00:00:00:00 \
> +-s 2:0,ahci-hd,/tmp/freebsd.img bhyve
> diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-net-e1000.ldargs b/tests/bhyvexml2argvdata/bhyvexml2argv-net-e1000.ldargs
> new file mode 100644
> index 0000000..32538b5
> --- /dev/null
> +++ b/tests/bhyvexml2argvdata/bhyvexml2argv-net-e1000.ldargs
> @@ -0,0 +1,3 @@
> +/usr/sbin/bhyveload \
> +-m 214 \
> +-d /tmp/freebsd.img bhyve
> diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-net-e1000.xml b/tests/bhyvexml2argvdata/bhyvexml2argv-net-e1000.xml
> new file mode 100644
> index 0000000..4b3148c
> --- /dev/null
> +++ b/tests/bhyvexml2argvdata/bhyvexml2argv-net-e1000.xml
> @@ -0,0 +1,22 @@
> +<domain type='bhyve'>
> + <name>bhyve</name>
> + <uuid>df3be7e7-a104-11e3-aeb0-50e5492bd3dc</uuid>
> + <memory>219136</memory>
> + <vcpu>1</vcpu>
> + <os>
> + <type>hvm</type>
> + </os>
> + <devices>
> + <disk type='file'>
> + <driver name='file' type='raw'/>
> + <source file='/tmp/freebsd.img'/>
> + <target dev='hda' bus='sata'/>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
> + </disk>
> + <interface type='bridge'>
> + <model type='e1000'/>
> + <source bridge="virbr0"/>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
> + </interface>
> + </devices>
> +</domain>
> diff --git a/tests/bhyvexml2argvtest.c b/tests/bhyvexml2argvtest.c
> index b85439b..a92b0cf 100644
> --- a/tests/bhyvexml2argvtest.c
> +++ b/tests/bhyvexml2argvtest.c
> @@ -151,7 +151,7 @@ mymain(void)
> DO_TEST_FULL(name, FLAG_EXPECT_PARSE_ERROR)
>
> driver.grubcaps = BHYVE_GRUB_CAP_CONSDEV;
> - driver.bhyvecaps = BHYVE_CAP_RTC_UTC;
> + driver.bhyvecaps = BHYVE_CAP_RTC_UTC | BHYVE_CAP_NET_E1000;
>
> DO_TEST("base");
> DO_TEST("acpiapic");
> @@ -174,11 +174,16 @@ mymain(void)
> DO_TEST("disk-cdrom-grub");
> DO_TEST("serial-grub");
> DO_TEST("localtime");
> + DO_TEST("net-e1000");
>
> driver.grubcaps = 0;
>
> DO_TEST("serial-grub-nocons");
>
> + driver.bhyvecaps &= ~BHYVE_CAP_NET_E1000;
> +
> + DO_TEST_FAILURE("net-e1000");
> +
> virObjectUnref(driver.caps);
> virObjectUnref(driver.xmlopt);
>
(I'll trust that the test cases are correct, since I don't know what
bhyve commands look like :-)
More information about the libvir-list
mailing list