Page MenuHomeFreeBSD

bhyve: move common code to net_utils.c
ClosedPublic

Authored by vmaffione on Jun 12 2019, 8:19 PM.

Details

Summary

Both virtio_net and e82545 network frontends have code to validate and generate
MAC addresses. These functionalities are replicated in the two files, so we move
them in a separate compilation unit.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

vmaffione created this revision.Jun 12 2019, 8:19 PM
vmaffione updated this revision to Diff 58572.Jun 12 2019, 8:22 PM

I am pulling Kyle Evans in on this as he just did a bunch of cleanup on mac generation code and there may already be existing code to reused rather than have yet another mac generator.

usr.sbin/bhyve/net_utils.c
3 ↗(On Diff #58572)

This I can understand being attributed to NetApp as you took this code from pci_e82545.c.

usr.sbin/bhyve/net_utils.h
3 ↗(On Diff #58572)

How is this file attributed to NetApp? I think this is a new file you created, as these functions did not exist before as external.

bryanv accepted this revision.Jun 13 2019, 3:22 PM
This revision is now accepted and ready to land.Jun 13 2019, 3:22 PM
vmaffione updated this revision to Diff 58588.Jun 13 2019, 3:29 PM

Updated attribution.

This revision now requires review to proceed.Jun 13 2019, 3:29 PM
vmaffione marked 2 inline comments as done.Jun 13 2019, 3:29 PM
imp added inline comments.Jun 13 2019, 3:43 PM
usr.sbin/bhyve/net_utils.c
81 ↗(On Diff #58588)

bhyve already has a set of MAC addresses assigned to it.

/* Organizationally Unique Identifier assigned by IEEE 14 Nov 2013 */
#define OUI_FREEBSD_BASE 0x589cfc000000
#define OUI_FREEBSD(nic) (OUI_FREEBSD_BASE | (nic))
...
/* Allocate 20 bits to bhyve */
#define OUI_FREEBSD_BHYVE_LOW OUI_FREEBSD(0x000001)
#define OUI_FREEBSD_BHYVE_HIGH OUI_FREEBSD(0x0fffff)
...

(from net/ieee-oui.h)

this isn't in that range. Is that relevant? Or perhaps a change for another day?

usr.sbin/bhyve/net_utils.h
3 ↗(On Diff #58588)

You can omit this phrase (here and elsewhere) if you choose. The project template no longer has it, though it used to.

grehan added a subscriber: grehan.Jun 13 2019, 3:50 PM
grehan added inline comments.
usr.sbin/bhyve/net_utils.c
81 ↗(On Diff #58588)

The issue with changing it (and why it wasn't done earlier) is that Linux guests would hang on boot when they see a different MAC address, which implied a different interface than the one configured. Maybe things are better these days with systemd.

imp added inline comments.Jun 13 2019, 4:02 PM
usr.sbin/bhyve/net_utils.c
81 ↗(On Diff #58588)

Makes sense... I'm cool with this just being code motion too...

vmaffione updated this revision to Diff 58590.Jun 13 2019, 4:25 PM

Removed "All rights reserved".

vmaffione marked 4 inline comments as done.Jun 13 2019, 4:28 PM
vmaffione added inline comments.
usr.sbin/bhyve/net_utils.c
81 ↗(On Diff #58588)

+1

kevans accepted this revision.Jun 13 2019, 4:39 PM

I am pulling Kyle Evans in on this as he just did a bunch of cleanup on mac generation code and there may already be existing code to reused rather than have yet another mac generator.

We have this facility (in kernel only at the moment), but I think it would ultimately be wrong for bhyve, judging by the description:

followed by an MD5 of the PCI slot/func number and dev name

One could theoretically take advantage of this to have a stable MAC across hosts for cases like, e.g., migration, no? Although it's not clear that bhyve folk would want to maintain this kind of promise, I could see it being somewhat handy.

This revision is now accepted and ready to land.Jun 13 2019, 4:39 PM
This revision was automatically updated to reflect the committed changes.

I am pulling Kyle Evans in on this as he just did a bunch of cleanup on mac generation code and there may already be existing code to reused rather than have yet another mac generator.

...

One could theoretically take advantage of this to have a stable MAC across hosts for cases like, e.g., migration, no? Although it's not clear that bhyve folk would want to maintain this kind of promise, I could see it being somewhat handy.

For the purpose of migration you take the mac with you, that is part of the guest state.

usr.sbin/bhyve/net_utils.c
3 ↗(On Diff #58590)

The All rights reserved clause needs to go back here, you can not remove someone else's All rights reserved

usr.sbin/bhyve/net_utils.h
3 ↗(On Diff #58588)

The elsewhere statement was missleading here, I think Warner meant for other places you may write original work, you can not omit this when copying someone else's copyrighted work and copyright.

vmaffione marked an inline comment as done.Jun 13 2019, 5:44 PM
vmaffione added inline comments.
usr.sbin/bhyve/net_utils.c
3 ↗(On Diff #58590)

Ok, I'll restore that on the next commit.