From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jim Fehlig Subject: Re: [libvirt] Setting devid for emulated NICs (Xen 4.3.1 / libvirt 1.2.0) using libxl driver Date: Wed, 18 Dec 2013 17:44:10 -0700 Message-ID: <52B2415A.3030903@suse.com> References: <52B07D09.5060008@canonical.com> <1387299534.1025.19.camel@dagon.hellion.org.uk> <52B08AA9.8010809@canonical.com> <1387369646.27441.129.camel@kazak.uk.xensource.com> <52B19F4E.8010601@canonical.com> <1387373284.28680.18.camel@kazak.uk.xensource.com> <52B1B842.4090306@canonical.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <52B1B842.4090306@canonical.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Stefan Bader Cc: libvir-list@redhat.com, Ian Campbell , Xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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 >>>>>> 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 >>>>>> Acked-by: Ian Campbell >>>>>> Committed-by: Ian Campbell >>>>>> >>>>>> 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