| Message ID | 20160520174504.GF31272@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Wei Liu writes ("Re: [RFC] libxl hotplug / unplug emulated devices"):
> On Fri, May 20, 2016 at 05:38:44PM +0100, Ian Jackson wrote:
> > Maybe the fix should be that xl network-attach should default hotplug
> > nics to pv only.
>
> Here is a patch to do this. :-)
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Although I have a style nit:
> if (libxl__device_model_version_running(gc, domid) ==
> - LIBXL_DEVICE_MODEL_VERSION_NONE)
> + LIBXL_DEVICE_MODEL_VERSION_NONE || hotplug)
> nic->nictype = LIBXL_NIC_TYPE_VIF;
This is rather odd formatting. It makes it look like
if (version = (NONE || hotplug)) { ...
You might prefer to put another line break, before the || perhaps.
Ian.
On Fri, May 20, 2016 at 06:58:40PM +0100, Ian Jackson wrote: > Wei Liu writes ("Re: [RFC] libxl hotplug / unplug emulated devices"): > > On Fri, May 20, 2016 at 05:38:44PM +0100, Ian Jackson wrote: > > > Maybe the fix should be that xl network-attach should default hotplug > > > nics to pv only. > > > > Here is a patch to do this. :-) > > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com> > > Although I have a style nit: > > > if (libxl__device_model_version_running(gc, domid) == > > - LIBXL_DEVICE_MODEL_VERSION_NONE) > > + LIBXL_DEVICE_MODEL_VERSION_NONE || hotplug) > > nic->nictype = LIBXL_NIC_TYPE_VIF; > > This is rather odd formatting. It makes it look like > if (version = (NONE || hotplug)) { ... > Perhaps you mean if ((version == NONE) || hotplug) ? > You might prefer to put another line break, before the || perhaps. > > Ian.
On Fri, May 20, 2016 at 07:04:20PM +0100, Wei Liu wrote: > On Fri, May 20, 2016 at 06:58:40PM +0100, Ian Jackson wrote: > > Wei Liu writes ("Re: [RFC] libxl hotplug / unplug emulated devices"): > > > On Fri, May 20, 2016 at 05:38:44PM +0100, Ian Jackson wrote: > > > > Maybe the fix should be that xl network-attach should default hotplug > > > > nics to pv only. > > > > > > Here is a patch to do this. :-) > > > > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com> > > > > Although I have a style nit: > > > > > if (libxl__device_model_version_running(gc, domid) == > > > - LIBXL_DEVICE_MODEL_VERSION_NONE) > > > + LIBXL_DEVICE_MODEL_VERSION_NONE || hotplug) > > > nic->nictype = LIBXL_NIC_TYPE_VIF; > > > > This is rather odd formatting. It makes it look like > > if (version = (NONE || hotplug)) { ... > > > > Perhaps you mean > > if ((version == NONE) || hotplug) > > ? Never mind. I misread. I will fix that. > > > You might prefer to put another line break, before the || perhaps. > > > > Ian.
Wei Liu writes ("Re: [RFC] libxl hotplug / unplug emulated devices"):
> On Fri, May 20, 2016 at 06:58:40PM +0100, Ian Jackson wrote:
> > Although I have a style nit:
> >
> > > if (libxl__device_model_version_running(gc, domid) ==
> > > - LIBXL_DEVICE_MODEL_VERSION_NONE)
> > > + LIBXL_DEVICE_MODEL_VERSION_NONE || hotplug)
> > > nic->nictype = LIBXL_NIC_TYPE_VIF;
> >
> > This is rather odd formatting. It makes it look like
> > if (version = (NONE || hotplug)) { ...
>
> Perhaps you mean
> if ((version == NONE) || hotplug)
> ?
That's what you meant, and the compiler knows you meant (because of
the relative precedence of == and ||).
My point is that the formatting is misleading.
Ian.
On 20/05/16 18:45, Wei Liu wrote: > On Fri, May 20, 2016 at 05:38:44PM +0100, Ian Jackson wrote: >> Wei Liu writes ("[RFC] libxl hotplug / unplug emulated devices"): >>> Recently I got a report on xen-users@ about xl network-attach not >>> working for HVM guest. >>> >>> I try to use >>> xl network-attach jessie-hvm 'bridge=xenbr0' >>> and vif-bridge script complains that it can't add vifXX-emu to bridge. >>> >>> The underlying issue is that the vif spec provided defaults to >>> emulated nic, but libxl only populates a pv nic but doesn't call out >>> via QMP to QEMU to populate one. Note that this issue not only affects >>> nic device but essentially all device types. >> >> Is it really sensible to offer emulated nic hotplug ? That'd be >> presented to the guest as pci hotplug, I guess ? >> >>> I also experimented with block device: >>> xl block-attach jessie-hvm 'phy:/dev/DATA/disk,hdb,w' >>> and it succeed, only pv disk is populated though. >> >> That's what I would have expected. >> >> Maybe the fix should be that xl network-attach should default hotplug >> nics to pv only. >> > > Here is a patch to do this. :-) Should we update the man page as well to clarify this? -George
On Mon, May 23, 2016 at 10:32:15AM +0100, George Dunlap wrote: > On 20/05/16 18:45, Wei Liu wrote: > > On Fri, May 20, 2016 at 05:38:44PM +0100, Ian Jackson wrote: > >> Wei Liu writes ("[RFC] libxl hotplug / unplug emulated devices"): > >>> Recently I got a report on xen-users@ about xl network-attach not > >>> working for HVM guest. > >>> > >>> I try to use > >>> xl network-attach jessie-hvm 'bridge=xenbr0' > >>> and vif-bridge script complains that it can't add vifXX-emu to bridge. > >>> > >>> The underlying issue is that the vif spec provided defaults to > >>> emulated nic, but libxl only populates a pv nic but doesn't call out > >>> via QMP to QEMU to populate one. Note that this issue not only affects > >>> nic device but essentially all device types. > >> > >> Is it really sensible to offer emulated nic hotplug ? That'd be > >> presented to the guest as pci hotplug, I guess ? > >> > >>> I also experimented with block device: > >>> xl block-attach jessie-hvm 'phy:/dev/DATA/disk,hdb,w' > >>> and it succeed, only pv disk is populated though. > >> > >> That's what I would have expected. > >> > >> Maybe the fix should be that xl network-attach should default hotplug > >> nics to pv only. > >> > > > > Here is a patch to do this. :-) > > Should we update the man page as well to clarify this? Yes, I think if we only support plugging PV disk and nic we should make that clear in man page. I will prepare a separate patch for that. Wei.
On Fri, May 20, 2016 at 06:45:04PM +0100, Wei Liu wrote: > On Fri, May 20, 2016 at 05:38:44PM +0100, Ian Jackson wrote: > > Wei Liu writes ("[RFC] libxl hotplug / unplug emulated devices"): > > > Recently I got a report on xen-users@ about xl network-attach not > > > working for HVM guest. > > > > > > I try to use > > > xl network-attach jessie-hvm 'bridge=xenbr0' > > > and vif-bridge script complains that it can't add vifXX-emu to bridge. > > > > > > The underlying issue is that the vif spec provided defaults to > > > emulated nic, but libxl only populates a pv nic but doesn't call out > > > via QMP to QEMU to populate one. Note that this issue not only affects > > > nic device but essentially all device types. > > > > Is it really sensible to offer emulated nic hotplug ? That'd be > > presented to the guest as pci hotplug, I guess ? > > > > > I also experimented with block device: > > > xl block-attach jessie-hvm 'phy:/dev/DATA/disk,hdb,w' > > > and it succeed, only pv disk is populated though. > > > > That's what I would have expected. > > > > Maybe the fix should be that xl network-attach should default hotplug > > nics to pv only. > > > > Here is a patch to do this. :-) > > ---8<--- > From 0a0a0ac76f825983bb13d70e46a59cabb05ed77b Mon Sep 17 00:00:00 2001 > From: Wei Liu <wei.liu2@citrix.com> > Date: Fri, 20 May 2016 18:16:05 +0100 > Subject: [PATCH for-4.7] libxl: nic type defaults to vif in hotplug for hvm guest > > We don't support plugging in emulated nic to a HVM guest. > > The "update_json" flag is only set when doing hotplug, so use that as an > indicator in libxl__device_nic_add. The new hotplug flag to _setdefault > function should be false in all other locations. > > This then requires saving nic type in JSON file, because we don't want > the receiving end to recalculate the nic type. > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > --- > tools/libxl/libxl.c | 6 +++--- > tools/libxl/libxl_create.c | 3 ++- > tools/libxl/libxl_dm.c | 3 ++- > tools/libxl/libxl_internal.h | 3 ++- > 4 files changed, 9 insertions(+), 6 deletions(-) > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > index c39d745..62e9294 100644 > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -3319,7 +3319,7 @@ out: > /******************************************************************************/ > > int libxl__device_nic_setdefault(libxl__gc *gc, libxl_device_nic *nic, > - uint32_t domid) > + uint32_t domid, bool hotplug) > { > int rc; > > @@ -3358,7 +3358,7 @@ int libxl__device_nic_setdefault(libxl__gc *gc, libxl_device_nic *nic, > case LIBXL_DOMAIN_TYPE_HVM: > if (!nic->nictype) { > if (libxl__device_model_version_running(gc, domid) == > - LIBXL_DEVICE_MODEL_VERSION_NONE) > + LIBXL_DEVICE_MODEL_VERSION_NONE || hotplug) > nic->nictype = LIBXL_NIC_TYPE_VIF; @@ -3357,8 +3357,9 @@ int libxl__device_nic_setdefault(libxl__gc *gc, libxl_device_nic *nic, switch (libxl__domain_type(gc, domid)) { case LIBXL_DOMAIN_TYPE_HVM: if (!nic->nictype) { - if (libxl__device_model_version_running(gc, domid) == - LIBXL_DEVICE_MODEL_VERSION_NONE) + if (hotplug || + (libxl__device_model_version_running(gc, domid) == + LIBXL_DEVICE_MODEL_VERSION_NONE)) nic->nictype = LIBXL_NIC_TYPE_VIF; I changed the snippet to above and will push this patch soon. Wei.
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c index c39d745..62e9294 100644 --- a/tools/libxl/libxl.c +++ b/tools/libxl/libxl.c @@ -3319,7 +3319,7 @@ out: /******************************************************************************/ int libxl__device_nic_setdefault(libxl__gc *gc, libxl_device_nic *nic, - uint32_t domid) + uint32_t domid, bool hotplug) { int rc; @@ -3358,7 +3358,7 @@ int libxl__device_nic_setdefault(libxl__gc *gc, libxl_device_nic *nic, case LIBXL_DOMAIN_TYPE_HVM: if (!nic->nictype) { if (libxl__device_model_version_running(gc, domid) == - LIBXL_DEVICE_MODEL_VERSION_NONE) + LIBXL_DEVICE_MODEL_VERSION_NONE || hotplug) nic->nictype = LIBXL_NIC_TYPE_VIF; else nic->nictype = LIBXL_NIC_TYPE_VIF_IOEMU; @@ -3411,7 +3411,7 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t domid, libxl_device_nic_init(&nic_saved); libxl_device_nic_copy(CTX, &nic_saved, nic); - rc = libxl__device_nic_setdefault(gc, nic, domid); + rc = libxl__device_nic_setdefault(gc, nic, domid, aodev->update_json); if (rc) goto out; front = flexarray_make(gc, 16, 1); diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c index 5000bd0..7e57215 100644 --- a/tools/libxl/libxl_create.c +++ b/tools/libxl/libxl_create.c @@ -946,7 +946,8 @@ static void initiate_domain_create(libxl__egc *egc, * called libxl_device_nic_add when domcreate_launch_dm gets called, * but qemu needs the nic information to be complete. */ - ret = libxl__device_nic_setdefault(gc, &d_config->nics[i], domid); + ret = libxl__device_nic_setdefault(gc, &d_config->nics[i], domid, + false); if (ret) { LOG(ERROR, "Unable to set nic defaults for nic %d", i); goto error_out; diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c index 4aff323a..65dceee 100644 --- a/tools/libxl/libxl_dm.c +++ b/tools/libxl/libxl_dm.c @@ -1809,7 +1809,8 @@ static void spawn_stub_launch_dm(libxl__egc *egc, * called libxl_device_nic_add at this point, but qemu needs * the nic information to be complete. */ - ret = libxl__device_nic_setdefault(gc, &dm_config->nics[i], dm_domid); + ret = libxl__device_nic_setdefault(gc, &dm_config->nics[i], dm_domid, + false); if (ret) goto out; } diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h index c791418..fac5751 100644 --- a/tools/libxl/libxl_internal.h +++ b/tools/libxl/libxl_internal.h @@ -1217,7 +1217,7 @@ _hidden int libxl__device_disk_setdefault(libxl__gc *gc, libxl_device_disk *disk, uint32_t domid); _hidden int libxl__device_nic_setdefault(libxl__gc *gc, libxl_device_nic *nic, - uint32_t domid); + uint32_t domid, bool hotplug); _hidden int libxl__device_vtpm_setdefault(libxl__gc *gc, libxl_device_vtpm *vtpm); _hidden int libxl__device_vfb_setdefault(libxl__gc *gc, libxl_device_vfb *vfb); _hidden int libxl__device_vkb_setdefault(libxl__gc *gc, libxl_device_vkb *vkb); @@ -4095,6 +4095,7 @@ static inline void libxl__update_config_nic(libxl__gc *gc, libxl_device_nic *src) { dst->devid = src->devid; + dst->nictype = src->nictype; libxl_mac_copy(CTX, &dst->mac, &src->mac); }