Re: [libvirt] [PATCH RFC] libxl: reverse defaults on HVM net device attach

On 12/13/2016 03:24 AM, Jim Fehlig wrote: > On 12/09/2016 04:35 AM, Joao Martins wrote: >> libvirt libxl picks its own default with respect to the default NIC >> to use. libxlMakeNic is the one responsible for this and on boot it >> picks LIBXL_NIC_TYPE_VIF_IOEMU such that it accomodates both PV and >> emulated one. The good behaving guest at boot will then select the pv >> and unplug the emulated device. >> >> Now, on HVM when attaching an interface it will pick the same default >> that is LIBXL_NIC_TYPE_VIF_IOEMU which as a result will fail the attach >> (see xen commit 32e9d0f ("libxl: nic type defaults to vif in hotplug for >> hvm guest"). Xen doesn't yet support the hotplug of emulated devices, >> but we don't want to rule out that case either, which might get support >> in the future. Hence we simply reverse the defaults when we are >> attaching the interface which allows libvirt to prefer the PV nic first >> without adding "model='netfront'" following the same pattern as above >> commit. Also to avoid ruling out the emulated one we set to >> LIBXL_NIC_TYPE_IOEMU when setting a model type that is not 'netfront'. >> >> Signed-off-by: Joao Martins <joao.m.martins(a)oracle.com&gt; >> --- >> >> This allows Openstack to attach network interfaces, which currently >> is broken on libxl if it doesn't set model 'netfront'. I am not sure >> whether this is the best way (hence RFC) or if users should really be >> setting the model='netfront' when attaching devices. But sounds to me >> that it would be better to have by default the supported, and if users >> want emulated attach to specify a nic model. Thoughts? > Agreed. I think it is best to mimic the behavior of xl/libxl. Cool, Thanks for the feedback. >> --- >> src/libxl/libxl_conf.c | 9 ++++++--- >> src/libxl/libxl_conf.h | 3 ++- >> src/libxl/libxl_driver.c | 2 +- >> 3 files changed, 9 insertions(+), 5 deletions(-) >> >> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c >> index dcf8e7e..50aa958 100644 >> --- a/src/libxl/libxl_conf.c >> +++ b/src/libxl/libxl_conf.c >> @@ -881,9 +881,10 @@ libxlMakeDiskList(virDomainDefPtr def, libxl_domain_config *d_config) >> int >> libxlMakeNic(virDomainDefPtr def, >> virDomainNetDefPtr l_nic, >> - libxl_device_nic *x_nic) >> + libxl_device_nic *x_nic, >> + bool attach) >> { >> - bool ioemu_nic = def->os.type == VIR_DOMAIN_OSTYPE_HVM; >> + bool ioemu_nic = def->os.type == VIR_DOMAIN_OSTYPE_HVM && !attach; >> virDomainNetType actual_type = virDomainNetGetActualType(l_nic); >> virNetworkPtr network = NULL; >> virConnectPtr conn = NULL; >> @@ -917,6 +918,8 @@ libxlMakeNic(virDomainDefPtr def, >> goto cleanup; >> if (STREQ(l_nic->model, "netfront")) >> x_nic->nictype = LIBXL_NIC_TYPE_VIF; >> + else >> + x_nic->nictype = LIBXL_NIC_TYPE_VIF_IOEMU; >> } >> >> if (VIR_STRDUP(x_nic->ifname, l_nic->ifname) < 0) > This logic has gotten a bit confusing for the casual reader not familiar with > the Xen behavior. It is late here and my eyes are getting heavy, but what do you > think about replacing this hunk with something like the below? Feel free to > suggest something more clear and succinct, but at minimum I think a comment > describing the Xen behavior is warranted. The patch looks good, just two minor comments below. I can submit another version, and as I am taking your changes - may I add your SoB tag?

> Regards, > Jim > > diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c > index dcf8e7e..6650b21 100644 > --- a/src/libxl/libxl_conf.c > +++ b/src/libxl/libxl_conf.c > @@ -881,7 +881,8 @@ libxlMakeDiskList(virDomainDefPtr def, libxl_domain_config > *d_config) > int > libxlMakeNic(virDomainDefPtr def, > virDomainNetDefPtr l_nic, > - libxl_device_nic *x_nic) > + libxl_device_nic *x_nic, > + bool attach) > { > bool ioemu_nic = def->os.type == VIR_DOMAIN_OSTYPE_HVM; ^^^^^^^^^ This is no longer needed after this chunk.

> virDomainNetType actual_type = virDomainNetGetActualType(l_nic); > @@ -907,16 +908,33 @@ libxlMakeNic(virDomainDefPtr def, > > virMacAddrGetRaw(&l_nic->mac, x_nic->mac); > > - if (ioemu_nic) > - x_nic->nictype = LIBXL_NIC_TYPE_VIF_IOEMU; > - else > - x_nic->nictype = LIBXL_NIC_TYPE_VIF; > - > + /* > + * The nictype field of libxl_device_nic structure tells Xen which type of > + * NIC device to create for the domain. LIBXL_NIC_TYPE_VIF specifies a > + * PV NIC. LIBXL_NIC_TYPE_VIF_IOEMU specifies a PV and emulated NIC, > + * allowing the domain to choose which NIC to use and unplug the unused > + * one. LIBXL_NIC_TYPE_VIF_IOEMU is only valid for HVM domains. Further, > + * if hotplugging the NIC, emulated NICs are currently not supported. > + */ Probably in addition would it worth mentioning we mimic xl/libxl behavior, like below comment?

"Alternatively one could set LIBXL_NIC_TYPE_UNKNOWN and let libxl decide, but its behaviour might not be consistent across all supported xen versions. The other nictype values are well established already, hence we manually select our own default and mimic xl/libxl behaviour since xen commit 32e9d0f." > if (l_nic->model) { > + if (def->os.type == VIR_DOMAIN_OSTYPE_XEN && > + STRNEQ(l_nic->model, "netfront")) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("only model 'netfront' is supported for " > + "Xen PV domains")); > + return -1; > + } > if (VIR_STRDUP(x_nic->model, l_nic->model) < 0) > goto cleanup; > if (STREQ(l_nic->model, "netfront")) > x_nic->nictype = LIBXL_NIC_TYPE_VIF; > + else > + x_nic->nictype = LIBXL_NIC_TYPE_VIF_IOEMU; > + } else { > + if (def->os.type == VIR_DOMAIN_OSTYPE_HVM && !attach) > + x_nic->nictype = LIBXL_NIC_TYPE_VIF_IOEMU; > + else > + x_nic->nictype = LIBXL_NIC_TYPE_VIF; > } > > if (VIR_STRDUP(x_nic->ifname, l_nic->ifname) < 0) > Other than that, Looks good. One suggestion would be to group the tails of the if/else, but the condition would have some redundancy in the predicates, hence this one looks clearer.