On 7/13/20 4:28 PM, Bastien Orivel wrote:

This is only used in the ESX driver where, when set to "static", it will ignore all the checks libvirt does about the origin of the MAC address (whether or not it's in a VMWare OUI) and forward the original one to the ESX server telling it not to check it either.

This allows keeping a deterministic MAC address which can be useful for licensed software which might dislike changes.

Signed-off-by: Bastien Orivel <bastien.orivel@diateam.net> --- NEWS.rst | 6 ++++ docs/schemas/domaincommon.rng | 8 +++++ src/conf/domain_conf.c | 26 ++++++++++++++++- src/conf/domain_conf.h | 11 +++++++ src/vmx/vmx.c | 8 +++-- .../network-interface-mac-type.xml | 29 +++++++++++++++++++ tests/genericxml2xmltest.c | 2 ++ 7 files changed, 86 insertions(+), 4 deletions(-) create mode 100644 tests/genericxml2xmlindata/network-interface-mac-type.xml

diff --git a/NEWS.rst b/NEWS.rst index 1928220854..2c6c628c61 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -18,6 +18,12 @@ v6.6.0 (unreleased) Libvirt allows configuring ACPI Heterogeneous Memory Attribute Table to hint software running inside the guest on optimization.

+ * esx: Add a ``type`` attribute for mac addresses. + + This attribute allows (when set to ``static``) ignoring VMWare checks of the + MAC addresses that would generate a new one if they were in its OUI + (00:0c:29). + * **Improvements**

Now that I'm re-reading my comment to v1 I realize that I might have misled you. What I meant was this NEWS.rst change must go into a separate patch. The reason is, while this change happens in 6.6.0 if cherry-picked to say 6.5.0 the corresponding NEWS.rst section doesn't exist and hence it wouldn't be a clean cherry-pick. The documentation of new XML attribute can of course go with the code that's introducing it (in fact, it should).

* esx: Change the NIC limit for recent virtualHW versions diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 4b4aa60c66..a810f569c6 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3179,6 +3179,14 @@ <attribute name="address"> <ref name="uniMacAddr"/> </attribute> + <optional> + <attribute name="type"> + <choice> + <value>generated</value> + <value>static</value> + </choice> + </attribute> + </optional> <empty/> </element> </optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d14485f18d..93e203de23 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -611,6 +611,13 @@ VIR_ENUM_IMPL(virDomainChrDeviceState, "disconnected", );

+VIR_ENUM_IMPL(virDomainNetMacType, + VIR_DOMAIN_NET_MAC_TYPE_LAST, + "", + "generated", + "static", +); + VIR_ENUM_IMPL(virDomainChrSerialTarget, VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_LAST, "none", @@ -11904,6 +11911,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, virDomainChrSourceReconnectDef reconnect = {0}; int rv, val; g_autofree char *macaddr = NULL; + g_autofree char *macaddr_type = NULL; g_autofree char *type = NULL; g_autofree char *network = NULL; g_autofree char *portgroup = NULL; @@ -11984,6 +11992,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, } if (!macaddr && virXMLNodeNameEqual(cur, "mac")) { macaddr = virXMLPropString(cur, "address"); + macaddr_type = virXMLPropString(cur, "type"); } else if (!network && def->type == VIR_DOMAIN_NET_TYPE_NETWORK && virXMLNodeNameEqual(cur, "source")) { @@ -12173,6 +12182,18 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, def->mac_generated = true; }

+ if (macaddr_type) { + virDomainNetMacType tmp;

This should be type of int. The reason is that upon error TypeFromString() will return a negative value and we are not guaranteed that typecast to the enum keeps the value negative.

+ if ((tmp = virDomainNetMacTypeTypeFromString(macaddr_type)) <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("invalid mac address check value: '%s'. Valid " + "values are \"generated\" and \"static\"."), + macaddr_type); + goto error; + } + def->mac_type = tmp; + } + if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, flags | VIR_DOMAIN_DEF_PARSE_ALLOW_BOOT | VIR_DOMAIN_DEF_PARSE_ALLOW_ROM) < 0) { @@ -26468,8 +26489,11 @@ virDomainNetDefFormat(virBufferPtr buf, virBufferAddLit(buf, ">\n");

virBufferAdjustIndent(buf, 2); - virBufferAsprintf(buf, "<mac address='%s'/>\n", + virBufferAsprintf(buf, "<mac address='%s'", virMacAddrFormat(&def->mac, macstr)); + if (def->mac_type) + virBufferAsprintf(buf, " type='%s'", virDomainNetMacTypeTypeToString(def->mac_type)); + virBufferAddLit(buf, "/>\n");

if (publicActual) { /* when there is a virDomainActualNetDef, and we haven't been diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 6a737591e2..7b754f959d 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -921,6 +921,15 @@ typedef enum { VIR_DOMAIN_NET_VIRTIO_TX_MODE_LAST } virDomainNetVirtioTxModeType;

+/* whether a mac address should be marked as generated in the esx driver or not*/ +typedef enum { + VIR_DOMAIN_NET_MAC_TYPE_DEFAULT, /* generated */

As mentioned on the IRC, this should be: VIR_DOMAIN_NET_MAC_TYPE_DEFAULT = 0 to make it explicit that value zero means default (and that when the corresponding virDomainNetDef structure is allocated the _DEFAULT is 'put in').

+ VIR_DOMAIN_NET_MAC_TYPE_GENERATED, + VIR_DOMAIN_NET_MAC_TYPE_STATIC, + + VIR_DOMAIN_NET_MAC_TYPE_LAST +} virDomainNetMacType; + /* the type of teaming device */ typedef enum { VIR_DOMAIN_NET_TEAMING_TYPE_NONE, @@ -972,6 +981,7 @@ struct _virDomainNetDef { virDomainNetType type; virMacAddr mac; bool mac_generated; /* true if mac was *just now* auto-generated by libvirt */ + virDomainNetMacType mac_type; int model; /* virDomainNetModelType */ char *modelstr; union { @@ -3556,6 +3566,7 @@ VIR_ENUM_DECL(virDomainFSCacheMode); VIR_ENUM_DECL(virDomainNet); VIR_ENUM_DECL(virDomainNetBackend); VIR_ENUM_DECL(virDomainNetVirtioTxMode); +VIR_ENUM_DECL(virDomainNetMacType); VIR_ENUM_DECL(virDomainNetTeaming); VIR_ENUM_DECL(virDomainNetInterfaceLinkState); VIR_ENUM_DECL(virDomainNetModel); diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index d4d66f6768..b307072869 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -3829,19 +3829,21 @@ virVMXFormatEthernet(virDomainNetDefPtr def, int controller, prefix = (def->mac.addr[0] << 16) | (def->mac.addr[1] << 8) | def->mac.addr[2]; suffix = (def->mac.addr[3] << 16) | (def->mac.addr[4] << 8) | def->mac.addr[5];

- if (prefix == 0x000c29) { + bool staticMac = def->mac_type == VIR_DOMAIN_NET_MAC_TYPE_STATIC;

We don't really like variables declared in the middle of a block.

+ + if (prefix == 0x000c29 && !staticMac) { virBufferAsprintf(buffer, "ethernet%d.addressType = \"generated\"\n", controller); virBufferAsprintf(buffer, "ethernet%d.generatedAddress = \"%s\"\n", controller, mac_string); virBufferAsprintf(buffer, "ethernet%d.generatedAddressOffset = \"0\"\n", controller); - } else if (prefix == 0x005056 && suffix <= 0x3fffff) { + } else if (prefix == 0x005056 && suffix <= 0x3fffff && !staticMac) { virBufferAsprintf(buffer, "ethernet%d.addressType = \"static\"\n", controller); virBufferAsprintf(buffer, "ethernet%d.address = \"%s\"\n", controller, mac_string); - } else if (prefix == 0x005056 && suffix >= 0x800000 && suffix <= 0xbfffff) { + } else if (prefix == 0x005056 && suffix >= 0x800000 && suffix <= 0xbfffff && !staticMac) { virBufferAsprintf(buffer, "ethernet%d.addressType = \"vpx\"\n", controller); virBufferAsprintf(buffer, "ethernet%d.generatedAddress = \"%s\"\n", diff --git a/tests/genericxml2xmlindata/network-interface-mac-type.xml b/tests/genericxml2xmlindata/network-interface-mac-type.xml new file mode 100644 index 0000000000..2b690629bb --- /dev/null +++ b/tests/genericxml2xmlindata/network-interface-mac-type.xml @@ -0,0 +1,29 @@ +<domain type='qemu'> + <name>QEMUGuest1</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 arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <interface type='bridge'> + <mac address='aa:bb:cc:dd:ee:ff'/> + <source bridge='br0'/> + </interface> + <interface type='bridge'> + <mac address='aa:bb:cc:dd:ee:fe' type='static'/> + <source bridge='br1'/> + </interface> + <interface type='bridge'> + <mac address='aa:bb:cc:dd:ee:fd' type='generated'/> + <source bridge='br2'/> + </interface> + </devices> +</domain> diff --git a/tests/genericxml2xmltest.c b/tests/genericxml2xmltest.c index 8b9b0bafb6..ea6585c901 100644 --- a/tests/genericxml2xmltest.c +++ b/tests/genericxml2xmltest.c @@ -183,6 +183,8 @@ mymain(void) DO_TEST("cpu-cache-passthrough"); DO_TEST("cpu-cache-disable");

+ DO_TEST("network-interface-mac-type"); + DO_TEST_DIFFERENT("chardev-tcp"); DO_TEST_FULL("chardev-tcp-missing-host", 0, false, TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_PARSE);

Just realized that we can move this to xml2vmxtest.c and xml2vmxdata. That enables us to demonstrate that if a MAC address begins with the right prefix (00:0c:29) but has type='static' the expected config is generated. Michal

On 7/13/20 4:28 PM, Bastien Orivel wrote:

Signed-off-by: Bastien Orivel <bastien.orivel@diateam.net> --- docs/drvesx.html.in | 7 +++++++ 1 file changed, 7 insertions(+)

diff --git a/docs/drvesx.html.in b/docs/drvesx.html.in index ac7bc645d1..70b7066861 100644 --- a/docs/drvesx.html.in +++ b/docs/drvesx.html.in @@ -427,6 +427,13 @@ error: invalid argument in libvirt was built without the 'esx' driver <pre> ethernet0.checkMACAddress = "false" </pre> + <p> + <span class="since">Since 6.6.0</span>, one can force libvirt to keep the + provided MAC address when it's in the reserved VMware range by adding a + <code>type="static"</code> attribute to the <code><mac/></code> element. + Note that this attribute is useless if the provided MAC address is outisde of + the reserved VMWare ranges. + </p>

<h3><a id="hardware">Available hardware</a></h3>

As I say in 2/2, this should be NEWS.rst change :-) Michal