Joao Martins wrote:
> 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@xxxxxxxxxx>
>>> ---
>>>
>>> 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?
Sure, no problem.
>
>> 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.
Right. I guess the secret is out that I didn't even compile-test my suggestion :-).
>
>> 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?
Nod. PV vs PV+emulated NIC is a Xen oddity that has caused lots of confusion
over the years, so I think it is hard to be too verbose here.
>
> "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.
Feel free to rearrange that logic if you can think of something more clever. My
main review note about your patch was the need for a comment describing the Xen
behavior.
Regards,
Jim
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
