All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC] libxl hotplug / unplug emulated devices
@ 2016-05-20 16:34 Wei Liu
  2016-05-20 16:38 ` Ian Jackson
  0 siblings, 1 reply; 13+ messages in thread
From: Wei Liu @ 2016-05-20 16:34 UTC (permalink / raw)
  To: Xen-devel; +Cc: Anthony PERARD, Ian Jackson, Wei Liu, George Dunlap

Hi all

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.

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.

I think the ability to plug in emulated devices is nice to have -- how
important this is is up for debate.  As far as I can tell, at
least for nic, plugging in an emulated nic never worked in libxl. I
can't speak for xend. I also don't see many complains, so this is
probably not that important after all. On the other hand, it might be a
prerequisite for HVM usb passthrough?

The other angle is that we only support plugging in pv devices. Then
the rest of this email is moot. I will fix the network attach command
and we call it a day. :-)

To plug in emulated devices, calling out to QEMU is probably the easy
part. Also currently libxl__device_XXX_add always populates xenstore
entries, which we might not need or want. This is not too hard to
change, either.

The harder bit is to decide what should libxl do when disk and nic are
involved. That is, device that has the ability to use either emulated
path and pv path (which I think there are only disk and nic at the
moment).

Guest has ability to unplug emulated devices and switch to pv
devices. But the switch can only happen when the guest is booting. So
it would be ok to provide both emulated and pv disk / nic during guest
start up but not hotplug.

What if the user indeed wants to hotplug an emulated disk?

1. Add an extra field in diskspec to indicate intention (pv, emulated,
   both), defaults to "pv" when hotplug, "both" when creating domain.
   
2. Strictly follow the disk spec when doing hotplug, if it is emulated
   device identifier (hda, sda etc), add only emulated device, if it
   is pv identifier (xvdx etc), add only pv device. Refuse to proceed
   if nothing is specified. Note this might subtly change the device
   the guest sees, but there is no danger of data corruption.

3. Query QEMU if unplug has happened and combine this with option 1 to
   make libxl "smarter". For example, if unplug has happened, the
   intention flag defaults to pv, otherwise it defaults to both. This
   requires modifying QEMU.

Follow the same principle for nic.

Do we expect more devices to be able to switch from emulation path to
pv path?

I can take a stab at this if we make a decision on what to do.

Wei.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC] libxl hotplug / unplug emulated devices
  2016-05-20 16:34 [RFC] libxl hotplug / unplug emulated devices Wei Liu
@ 2016-05-20 16:38 ` Ian Jackson
  2016-05-20 16:42   ` Wei Liu
  2016-05-20 17:45   ` Wei Liu
  0 siblings, 2 replies; 13+ messages in thread
From: Ian Jackson @ 2016-05-20 16:38 UTC (permalink / raw)
  To: Wei Liu; +Cc: Anthony PERARD, Xen-devel, George Dunlap

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.

Ian.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC] libxl hotplug / unplug emulated devices
  2016-05-20 16:38 ` Ian Jackson
@ 2016-05-20 16:42   ` Wei Liu
  2016-05-20 16:45     ` Andrew Cooper
  2016-05-20 17:45   ` Wei Liu
  1 sibling, 1 reply; 13+ messages in thread
From: Wei Liu @ 2016-05-20 16:42 UTC (permalink / raw)
  To: Ian Jackson; +Cc: Anthony PERARD, Xen-devel, Wei Liu, George Dunlap

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 ?

Suppose you have a Windows guest doesn't have PV driver? Or any other
OSes that have PCI drivers with hotplug support but not Xen drivers?

For the second question, yes, more or less the same if you're talking
about libxl side implementation. It's going to call some QMP commands.

> 
> > 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.
> 

I certainly am fine with this.

Wei.

> Ian.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC] libxl hotplug / unplug emulated devices
  2016-05-20 16:42   ` Wei Liu
@ 2016-05-20 16:45     ` Andrew Cooper
  2016-05-20 16:49       ` Wei Liu
  0 siblings, 1 reply; 13+ messages in thread
From: Andrew Cooper @ 2016-05-20 16:45 UTC (permalink / raw)
  To: Wei Liu, Ian Jackson; +Cc: Anthony PERARD, Xen-devel, George Dunlap

On 20/05/16 17:42, 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 ?
> Suppose you have a Windows guest doesn't have PV driver? Or any other
> OSes that have PCI drivers with hotplug support but not Xen drivers?

On qemu-trad, none of the devices support hotplug, so the option
shouldn't be available in xl.

I believe qemu-upstream does offer hotplug devices, but it still has to
create empty PCIe slots at boot time to hotplug into later, along with
appropriate ACPI tables.


>
> For the second question, yes, more or less the same if you're talking
> about libxl side implementation. It's going to call some QMP commands.
>
>>> 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.
>>
> I certainly am fine with this.

+1.

~Andrew

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC] libxl hotplug / unplug emulated devices
  2016-05-20 16:45     ` Andrew Cooper
@ 2016-05-20 16:49       ` Wei Liu
  0 siblings, 0 replies; 13+ messages in thread
From: Wei Liu @ 2016-05-20 16:49 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Anthony PERARD, Ian Jackson, Wei Liu, George Dunlap, Xen-devel

On Fri, May 20, 2016 at 05:45:53PM +0100, Andrew Cooper wrote:
> On 20/05/16 17:42, 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 ?
> > Suppose you have a Windows guest doesn't have PV driver? Or any other
> > OSes that have PCI drivers with hotplug support but not Xen drivers?
> 
> On qemu-trad, none of the devices support hotplug, so the option
> shouldn't be available in xl.
> 
> I believe qemu-upstream does offer hotplug devices, but it still has to
> create empty PCIe slots at boot time to hotplug into later, along with
> appropriate ACPI tables.
> 

I don't think it requires that much setup TBH. I tried to issue two QMP
commands and a nic shows up in the guest. Admittedly I only played with
it to see if it actually works so I could be missing a lot of things.

Wei.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC] libxl hotplug / unplug emulated devices
  2016-05-20 16:38 ` Ian Jackson
  2016-05-20 16:42   ` Wei Liu
@ 2016-05-20 17:45   ` Wei Liu
  2016-05-20 17:58     ` Ian Jackson
                       ` (2 more replies)
  1 sibling, 3 replies; 13+ messages in thread
From: Wei Liu @ 2016-05-20 17:45 UTC (permalink / raw)
  To: Ian Jackson; +Cc: Anthony PERARD, Xen-devel, Wei Liu, George Dunlap

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;
             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);
 }
 
-- 
2.1.4

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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [RFC] libxl hotplug / unplug emulated devices
  2016-05-20 17:45   ` Wei Liu
@ 2016-05-20 17:58     ` Ian Jackson
  2016-05-20 18:04       ` Wei Liu
  2016-05-23  9:32     ` George Dunlap
  2016-05-23 10:45     ` Wei Liu
  2 siblings, 1 reply; 13+ messages in thread
From: Ian Jackson @ 2016-05-20 17:58 UTC (permalink / raw)
  To: Wei Liu; +Cc: Anthony PERARD, Xen-devel, George Dunlap

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.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC] libxl hotplug / unplug emulated devices
  2016-05-20 17:58     ` Ian Jackson
@ 2016-05-20 18:04       ` Wei Liu
  2016-05-20 18:05         ` Wei Liu
  2016-05-20 18:05         ` Ian Jackson
  0 siblings, 2 replies; 13+ messages in thread
From: Wei Liu @ 2016-05-20 18:04 UTC (permalink / raw)
  To: Ian Jackson; +Cc: Anthony PERARD, Xen-devel, Wei Liu, George Dunlap

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.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC] libxl hotplug / unplug emulated devices
  2016-05-20 18:04       ` Wei Liu
@ 2016-05-20 18:05         ` Wei Liu
  2016-05-20 18:05         ` Ian Jackson
  1 sibling, 0 replies; 13+ messages in thread
From: Wei Liu @ 2016-05-20 18:05 UTC (permalink / raw)
  To: Ian Jackson; +Cc: Anthony PERARD, Xen-devel, Wei Liu, George Dunlap

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.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC] libxl hotplug / unplug emulated devices
  2016-05-20 18:04       ` Wei Liu
  2016-05-20 18:05         ` Wei Liu
@ 2016-05-20 18:05         ` Ian Jackson
  1 sibling, 0 replies; 13+ messages in thread
From: Ian Jackson @ 2016-05-20 18:05 UTC (permalink / raw)
  To: Wei Liu; +Cc: Anthony PERARD, Xen-devel, Ian Jackson, George Dunlap

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.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC] libxl hotplug / unplug emulated devices
  2016-05-20 17:45   ` Wei Liu
  2016-05-20 17:58     ` Ian Jackson
@ 2016-05-23  9:32     ` George Dunlap
  2016-05-23 10:28       ` Wei Liu
  2016-05-23 10:45     ` Wei Liu
  2 siblings, 1 reply; 13+ messages in thread
From: George Dunlap @ 2016-05-23  9:32 UTC (permalink / raw)
  To: Wei Liu, Ian Jackson; +Cc: Anthony PERARD, Xen-devel, George Dunlap

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


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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC] libxl hotplug / unplug emulated devices
  2016-05-23  9:32     ` George Dunlap
@ 2016-05-23 10:28       ` Wei Liu
  0 siblings, 0 replies; 13+ messages in thread
From: Wei Liu @ 2016-05-23 10:28 UTC (permalink / raw)
  To: George Dunlap
  Cc: Anthony PERARD, Ian Jackson, Wei Liu, George Dunlap, Xen-devel

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.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC] libxl hotplug / unplug emulated devices
  2016-05-20 17:45   ` Wei Liu
  2016-05-20 17:58     ` Ian Jackson
  2016-05-23  9:32     ` George Dunlap
@ 2016-05-23 10:45     ` Wei Liu
  2 siblings, 0 replies; 13+ messages in thread
From: Wei Liu @ 2016-05-23 10:45 UTC (permalink / raw)
  To: Ian Jackson; +Cc: Anthony PERARD, Xen-devel, Wei Liu, George Dunlap

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.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2016-05-23 10:45 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-20 16:34 [RFC] libxl hotplug / unplug emulated devices Wei Liu
2016-05-20 16:38 ` Ian Jackson
2016-05-20 16:42   ` Wei Liu
2016-05-20 16:45     ` Andrew Cooper
2016-05-20 16:49       ` Wei Liu
2016-05-20 17:45   ` Wei Liu
2016-05-20 17:58     ` Ian Jackson
2016-05-20 18:04       ` Wei Liu
2016-05-20 18:05         ` Wei Liu
2016-05-20 18:05         ` Ian Jackson
2016-05-23  9:32     ` George Dunlap
2016-05-23 10:28       ` Wei Liu
2016-05-23 10:45     ` Wei Liu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.