Re: [Xen-devel] [libvirt] Setting devid for emulated NICs (Xen 4.3.1 / libvirt 1.2.0) using libxl driver

[Jim Fehlig <jfehlig@xxxxxxxx>]

Stefan Bader wrote:
> On 18.12.2013 14:28, Ian Campbell wrote:
>   
>> On Wed, 2013-12-18 at 14:12 +0100, Stefan Bader wrote:
>>     
>>> On 18.12.2013 13:27, Ian Campbell wrote:
>>>       
>>>> On Tue, 2013-12-17 at 18:32 +0100, Stefan Bader wrote:
>>>>         
>>>>>> Might this libxl fix be relevant:
>>>>>>         commit 5420f26507fc5c9853eb1076401a8658d72669da
>>>>>>         Author: Jim Fehlig <jfehlig@xxxxxxxx>
>>>>>>         Date:   Fri Jan 11 12:22:26 2013 +0000
>>>>>>         
>>>>>>             libxl: Set vfb and vkb devid if not done so by the caller
>>>>>>             
>>>>>>             Other devices set a sensible devid if the caller has not 
>>>>>> done so.
>>>>>>             Do the same for vfb and vkb.  While at it, factor out the 
>>>>>> common code
>>>>>>             used to determine a sensible devid, so it can be used by 
>>>>>> other
>>>>>>             libxl__device_*_add functions.
>>>>>>             
>>>>>>             Signed-off-by: Jim Fehlig <jfehlig@xxxxxxxx>
>>>>>>             Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
>>>>>>             Committed-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
>>>>>>         
>>>>>> and a follow up in dfeccbeaa. Although the comment implies that nic's
>>>>>> were already correctly assigning a devid if the caller specified -1, so
>>>>>> I don't know why it doesn't work for you :-(
>>>>>>             
>>>>> Ok, yes, the commit above indeed changes libxl__device_nic_add to call
>>>>> libxl__device_nextid for the devid... Just how is this actually called.
>>>>> Maybe not sufficient but "git grep libxl__device_nic_add" in the xen code 
>>>>> only
>>>>> shows the definition and a declaration in libxl_internal.h to me...
>>>>>           
>>>> I have a feeling a macro might be involved...
>>>>
>>>> Here we go, look for DEFINE_DEVICE_REMOVE in libxl.c. We should really
>>>> add the eventual function names in comments to provide grep fodder....
>>>>         
>>> Oh duh, yeah. So in DEFINE_DEVICE_ADD a libxl_device_nic_add is created 
>>> which
>>> calls to libxl__device_nic_add. When I look for the single _ version I find 
>>> a
>>> call from xl_cmdimpl.c and its public declaration in libxl.h.
>>> So I guess the bug is that libvirt in the libxl driver never seems to do so
>>>       
>> The macro creates libxl__add_nics which adds the nics from the
>> libxl_domain_config->nics array. I don't think libvirt needs to call
>> libxl_device_nic_add manually unless it is hotplugging a new nic at
>> runtime.
>>
>>     
>
> Hm, so I think this is the path:
>
> libxl_domain_create_new
> -> do_domain_create
>    -> initiate_domain_create
>       -> libxl__bootloader_run (HVM domain, skipping bootloader)
>       <- domcreate_bootloader_done
>          -> domcreate_rebuild_done
>             <- domcreate_launch_dm
>                -> libxl__spawn_local_dm
>          <- domcreate_devmodel_started
>
> In libxl__spawn_local_dm, there is the following loop:
>
>     for (i = 0; i < d_config->num_nics; i++) {
>         /* We have to init the nic here, because we still haven't
>          * called libxl_device_nic_add at this point, but qemu needs
>          * the nic information to be complete.
>          */
>         ret = libxl__device_nic_setdefault(gc, &d_config->nics[i], domid);
>         if (ret)
>             goto error_out;
>     }
>
> So I think when starting the dm, the devid just is not set as setdefault does
> not seem to do so. I would be done in the later domcreate_devmodel_started
> callback but that is too late for the generated qemu arguments.
>   

Sorry for jumping in late...

I stumbled across this problem just before openSUSE13.1 released and did
a quick fix in libvirt

https://build.opensuse.org/package/view_file/Virtualization:openSUSE13.1/libvirt/libxl-hvm-nic.patch?expand=1

I removed setting the NIC devid in the libxl driver a while back to be
consistent with other devices

http://libvirt.org/git/?p=libvirt.git;a=commit;h=ba64b97134a6129a48684f22f31be92c3b6eef96

The quick fix was to essentially revert the above commit until I could
investigate further.  Thank you for now having done that investigation
:).  Can the devid assignment logic be moved from
libxl__device_nic_add() to libxl__device_nic_setdefault()?

Regards,
Jim


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel