| Message ID | 20210112181242.1570-17-bouyer@antioche.eu.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | libs/light: pass some infos to qemu | expand |
On Tue, Jan 12, 2021 at 07:12:37PM +0100, Manuel Bouyer wrote: > From: Manuel Bouyer <bouyer@netbsd.org> > > Pass bridge name to qemu as command line option > When starting qemu, set an environnement variable XEN_DOMAIN_ID, > to be used by qemu helper scripts > > Signed-off-by: Manuel Bouyer <bouyer@netbsd.org> > --- > tools/libs/light/libxl_dm.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c > index 3da83259c0..8866c3f5ad 100644 > --- a/tools/libs/light/libxl_dm.c > +++ b/tools/libs/light/libxl_dm.c > @@ -761,6 +761,8 @@ static int libxl__build_device_model_args_old(libxl__gc *gc, > int nr_set_cpus = 0; > char *s; > > + flexarray_append_pair(dm_envs, "XEN_DOMAIN_ID", GCSPRINTF("%d", domid)); > + > if (b_info->kernel) { > LOGD(ERROR, domid, "HVM direct kernel boot is not supported by " > "qemu-xen-traditional"); > @@ -1547,8 +1549,10 @@ static int libxl__build_device_model_args_new(libxl__gc *gc, > flexarray_append(dm_args, "-netdev"); > flexarray_append(dm_args, > GCSPRINTF("type=tap,id=net%d,ifname=%s," > + "br=%s," > "script=%s,downscript=%s", > nics[i].devid, ifname, > + nics[i].bridge, You have some hard tabs in there. Also looking at the manual the br= option seems to only be available for the bridge networking mode, while here Xen is using tap instead? Thanks, Roger.
On Sat, Jan 16, 2021 at 11:16:06AM +0100, Roger Pau Monné wrote: > On Tue, Jan 12, 2021 at 07:12:37PM +0100, Manuel Bouyer wrote: > > From: Manuel Bouyer <bouyer@netbsd.org> > > > > Pass bridge name to qemu as command line option > > When starting qemu, set an environnement variable XEN_DOMAIN_ID, > > to be used by qemu helper scripts > > > > Signed-off-by: Manuel Bouyer <bouyer@netbsd.org> > > --- > > tools/libs/light/libxl_dm.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c > > index 3da83259c0..8866c3f5ad 100644 > > --- a/tools/libs/light/libxl_dm.c > > +++ b/tools/libs/light/libxl_dm.c > > @@ -761,6 +761,8 @@ static int libxl__build_device_model_args_old(libxl__gc *gc, > > int nr_set_cpus = 0; > > char *s; > > > > + flexarray_append_pair(dm_envs, "XEN_DOMAIN_ID", GCSPRINTF("%d", domid)); > > + > > if (b_info->kernel) { > > LOGD(ERROR, domid, "HVM direct kernel boot is not supported by " > > "qemu-xen-traditional"); > > @@ -1547,8 +1549,10 @@ static int libxl__build_device_model_args_new(libxl__gc *gc, > > flexarray_append(dm_args, "-netdev"); > > flexarray_append(dm_args, > > GCSPRINTF("type=tap,id=net%d,ifname=%s," > > + "br=%s," > > "script=%s,downscript=%s", > > nics[i].devid, ifname, > > + nics[i].bridge, > > You have some hard tabs in there. Yes. What's the problem ? > > Also looking at the manual the br= option seems to only be available > for the bridge networking mode, while here Xen is using tap instead? Unless I missed something, the bridge networking mode is using the tap interface, to connect qemu to the bridge. And indeed, the qemu-ifup script is doing exec /sbin/brconfig $2 add $1 (the script is called with: qemu-ifup <tap if> <bridge if>) This is a problem that hit me when I converted NetBSD to qemu-xen: qemu-traditional does call the qemu-ifup script with the 2 parameters, while qemu-xen calls it only with the tap if. So the qemu-ifup script can't know to which bridge the tap interface should be attached to.
On Sat, Jan 16, 2021 at 12:25:02PM +0100, Manuel Bouyer wrote: > On Sat, Jan 16, 2021 at 11:16:06AM +0100, Roger Pau Monné wrote: > > On Tue, Jan 12, 2021 at 07:12:37PM +0100, Manuel Bouyer wrote: > > > From: Manuel Bouyer <bouyer@netbsd.org> > > > > > > Pass bridge name to qemu as command line option > > > When starting qemu, set an environnement variable XEN_DOMAIN_ID, > > > to be used by qemu helper scripts > > > > > > Signed-off-by: Manuel Bouyer <bouyer@netbsd.org> > > > --- > > > tools/libs/light/libxl_dm.c | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c > > > index 3da83259c0..8866c3f5ad 100644 > > > --- a/tools/libs/light/libxl_dm.c > > > +++ b/tools/libs/light/libxl_dm.c > > > @@ -761,6 +761,8 @@ static int libxl__build_device_model_args_old(libxl__gc *gc, > > > int nr_set_cpus = 0; > > > char *s; > > > > > > + flexarray_append_pair(dm_envs, "XEN_DOMAIN_ID", GCSPRINTF("%d", domid)); > > > + > > > if (b_info->kernel) { > > > LOGD(ERROR, domid, "HVM direct kernel boot is not supported by " > > > "qemu-xen-traditional"); > > > @@ -1547,8 +1549,10 @@ static int libxl__build_device_model_args_new(libxl__gc *gc, > > > flexarray_append(dm_args, "-netdev"); > > > flexarray_append(dm_args, > > > GCSPRINTF("type=tap,id=net%d,ifname=%s," > > > + "br=%s," > > > "script=%s,downscript=%s", > > > nics[i].devid, ifname, > > > + nics[i].bridge, > > > > You have some hard tabs in there. > > Yes. What's the problem ? This file (and libxenlight) uses only spaces for indentation, it breaks the coding style. The line you added above uses spaces and it's fine. > > > > Also looking at the manual the br= option seems to only be available > > for the bridge networking mode, while here Xen is using tap instead? > > Unless I missed something, the bridge networking mode is using the > tap interface, to connect qemu to the bridge. And indeed, the qemu-ifup > script is doing > exec /sbin/brconfig $2 add $1 > > (the script is called with: qemu-ifup <tap if> <bridge if>) > > This is a problem that hit me when I converted NetBSD to qemu-xen: > qemu-traditional does call the qemu-ifup script with the 2 parameters, > while qemu-xen calls it only with the tap if. So the qemu-ifup script can't > know to which bridge the tap interface should be attached to. OK, so the only functional difference of adding the br parameter is that it gets passed to the script. I would add that to the commit message: "The only functional difference of using the br parameter is that the bridge name gets passed to the QEMU script." Note also that there are networking modes that don't use a bridge, so you could likely end up with nics[i].bridge == NULL? I also wonder why NetBSD needs to add the tap interface to the bridge in the QEMU script instead of doing it from the hotplug script called by libxl, like Linux and FreeBSD do. Thanks, Roger.
On Mon, Jan 18, 2021 at 09:36:42AM +0100, Roger Pau Monné wrote: > On Sat, Jan 16, 2021 at 12:25:02PM +0100, Manuel Bouyer wrote: > > On Sat, Jan 16, 2021 at 11:16:06AM +0100, Roger Pau Monné wrote: > > > On Tue, Jan 12, 2021 at 07:12:37PM +0100, Manuel Bouyer wrote: > > > > From: Manuel Bouyer <bouyer@netbsd.org> > > > > > > > > Pass bridge name to qemu as command line option > > > > When starting qemu, set an environnement variable XEN_DOMAIN_ID, > > > > to be used by qemu helper scripts > > > > > > > > Signed-off-by: Manuel Bouyer <bouyer@netbsd.org> > > > > --- > > > > tools/libs/light/libxl_dm.c | 6 ++++++ > > > > 1 file changed, 6 insertions(+) > > > > > > > > diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c > > > > index 3da83259c0..8866c3f5ad 100644 > > > > --- a/tools/libs/light/libxl_dm.c > > > > +++ b/tools/libs/light/libxl_dm.c > > > > @@ -761,6 +761,8 @@ static int libxl__build_device_model_args_old(libxl__gc *gc, > > > > int nr_set_cpus = 0; > > > > char *s; > > > > > > > > + flexarray_append_pair(dm_envs, "XEN_DOMAIN_ID", GCSPRINTF("%d", domid)); > > > > + > > > > if (b_info->kernel) { > > > > LOGD(ERROR, domid, "HVM direct kernel boot is not supported by " > > > > "qemu-xen-traditional"); > > > > @@ -1547,8 +1549,10 @@ static int libxl__build_device_model_args_new(libxl__gc *gc, > > > > flexarray_append(dm_args, "-netdev"); > > > > flexarray_append(dm_args, > > > > GCSPRINTF("type=tap,id=net%d,ifname=%s," > > > > + "br=%s," > > > > "script=%s,downscript=%s", > > > > nics[i].devid, ifname, > > > > + nics[i].bridge, > > > > > > You have some hard tabs in there. > > > > Yes. What's the problem ? > > This file (and libxenlight) uses only spaces for indentation, it > breaks the coding style. > > The line you added above uses spaces and it's fine. Ha OK, will fix. I didn't see a difference in my editor. > > > > > > > Also looking at the manual the br= option seems to only be available > > > for the bridge networking mode, while here Xen is using tap instead? > > > > Unless I missed something, the bridge networking mode is using the > > tap interface, to connect qemu to the bridge. And indeed, the qemu-ifup > > script is doing > > exec /sbin/brconfig $2 add $1 > > > > (the script is called with: qemu-ifup <tap if> <bridge if>) > > > > This is a problem that hit me when I converted NetBSD to qemu-xen: > > qemu-traditional does call the qemu-ifup script with the 2 parameters, > > while qemu-xen calls it only with the tap if. So the qemu-ifup script can't > > know to which bridge the tap interface should be attached to. > > OK, so the only functional difference of adding the br parameter is > that it gets passed to the script. I would add that to the commit > message: > > "The only functional difference of using the br parameter is that the > bridge name gets passed to the QEMU script." will do. > > Note also that there are networking modes that don't use a bridge, so > you could likely end up with nics[i].bridge == NULL? I guess it would be nics[i].bridge="" (or some other default value) but I will check. AFAIK qemu will always be called with nics tap mode ? > > I also wonder why NetBSD needs to add the tap interface to the bridge > in the QEMU script instead of doing it from the hotplug script called > by libxl, like Linux and FreeBSD do. the tap interface is created by qemu itself, its name is not known outside of qemu. Also, is there guarantee that qemu has created the tap before the hotplug script is called ?
On Mon, Jan 18, 2021 at 09:52:14AM +0100, Manuel Bouyer wrote: > On Mon, Jan 18, 2021 at 09:36:42AM +0100, Roger Pau Monné wrote: > > I also wonder why NetBSD needs to add the tap interface to the bridge > > in the QEMU script instead of doing it from the hotplug script called > > by libxl, like Linux and FreeBSD do. > > the tap interface is created by qemu itself, its name is not known outside > of qemu. Also, is there guarantee that qemu has created the tap before > the hotplug script is called ? Yes, the toolstack will wait for QEMU to be initialized at which point the tap interface has been created. I think I remember now why this didn't work on NetBSD. We ask QEMU to create the tap interface with a specific name (using the vifname= parameter), but NetBSD doesn't have the ioctl to rename network interfaces implemented, and thus cannot rename the interface from tapX to vifX.Y-emu, and hence you need to use the QEMU script from QEMU itself because it's the only entity that knows the name of the tap interface created. If you agree, can you add something along the lines to the commit message? So that we remember why NetBSD needs to use the QEMU scripts. Thanks, Roger.
On Mon, Jan 18, 2021 at 10:07:22AM +0100, Roger Pau Monné wrote: > On Mon, Jan 18, 2021 at 09:52:14AM +0100, Manuel Bouyer wrote: > > On Mon, Jan 18, 2021 at 09:36:42AM +0100, Roger Pau Monné wrote: > > > I also wonder why NetBSD needs to add the tap interface to the bridge > > > in the QEMU script instead of doing it from the hotplug script called > > > by libxl, like Linux and FreeBSD do. > > > > the tap interface is created by qemu itself, its name is not known outside > > of qemu. Also, is there guarantee that qemu has created the tap before > > the hotplug script is called ? > > Yes, the toolstack will wait for QEMU to be initialized at which point > the tap interface has been created. > > I think I remember now why this didn't work on NetBSD. We ask QEMU to > create the tap interface with a specific name (using the vifname= > parameter), but NetBSD doesn't have the ioctl to rename network > interfaces implemented, and thus cannot rename the interface from tapX > to vifX.Y-emu, and hence you need to use the QEMU script from QEMU > itself because it's the only entity that knows the name of the tap > interface created. Yes; at some point I proposed to support name aliases to interface name in NetBSD but it got rejected ... so we have to use the name the kernel did choose for us. > > If you agree, can you add something along the lines to the commit > message? So that we remember why NetBSD needs to use the QEMU scripts. Will do. thanks
On Mon, Jan 18, 2021 at 09:36:42AM +0100, Roger Pau Monné wrote: > [...] > > Note also that there are networking modes that don't use a bridge, so > you could likely end up with nics[i].bridge == NULL? I couldn't cause this to happen. If no bridge is specified in the domU's config file, qemu-ifup is called with xenbr0 as bridge name. I tried this: vif = [ 'mac=00:16:3e:00:10:54, gatewaydev=wm0 type=ioemu, model=e1000' ]
On Tue, Jan 26, 2021 at 11:42:23PM +0100, Manuel Bouyer wrote: > On Mon, Jan 18, 2021 at 09:36:42AM +0100, Roger Pau Monné wrote: > > [...] > > > > Note also that there are networking modes that don't use a bridge, so > > you could likely end up with nics[i].bridge == NULL? > > I couldn't cause this to happen. If no bridge is specified in the > domU's config file, qemu-ifup is called with xenbr0 as bridge name. > > I tried this: > vif = [ 'mac=00:16:3e:00:10:54, gatewaydev=wm0 type=ioemu, model=e1000' ] Right, that's because libxl__device_nic_setdefault will set the bridge field to xenbr0 if empty. I'm not opposed to this behavior, seeing as we don't have much other option I'm afraid. I guess we could open the tap interface in libxl and pass it to QEMU, so that libxl can call the hotplug script knowing the tap interface name, but that seems non-trivial. IMO if we go that route it needs to be documented that NetBSD only supports bridged mode for emulated HVM network interfaces for the time being. Thanks, Roger.
On Wed, Jan 27, 2021 at 10:06:43AM +0100, Roger Pau Monné wrote: > On Tue, Jan 26, 2021 at 11:42:23PM +0100, Manuel Bouyer wrote: > > On Mon, Jan 18, 2021 at 09:36:42AM +0100, Roger Pau Monné wrote: > > > [...] > > > > > > Note also that there are networking modes that don't use a bridge, so > > > you could likely end up with nics[i].bridge == NULL? > > > > I couldn't cause this to happen. If no bridge is specified in the > > domU's config file, qemu-ifup is called with xenbr0 as bridge name. > > > > I tried this: > > vif = [ 'mac=00:16:3e:00:10:54, gatewaydev=wm0 type=ioemu, model=e1000' ] > > Right, that's because libxl__device_nic_setdefault will set the bridge > field to xenbr0 if empty. > > I'm not opposed to this behavior, seeing as we don't have much other > option I'm afraid. I guess we could open the tap interface in libxl > and pass it to QEMU, so that libxl can call the hotplug script knowing > the tap interface name, but that seems non-trivial. > > IMO if we go that route it needs to be documented that NetBSD only > supports bridged mode for emulated HVM network interfaces for the time > being. that's not quite true. As long as you have the domain ID in the qemu-if script, you can query parameters from the store and do whatever you want here (for example I use it to setup firewall rules).
diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c index 3da83259c0..8866c3f5ad 100644 --- a/tools/libs/light/libxl_dm.c +++ b/tools/libs/light/libxl_dm.c @@ -761,6 +761,8 @@ static int libxl__build_device_model_args_old(libxl__gc *gc, int nr_set_cpus = 0; char *s; + flexarray_append_pair(dm_envs, "XEN_DOMAIN_ID", GCSPRINTF("%d", domid)); + if (b_info->kernel) { LOGD(ERROR, domid, "HVM direct kernel boot is not supported by " "qemu-xen-traditional"); @@ -1547,8 +1549,10 @@ static int libxl__build_device_model_args_new(libxl__gc *gc, flexarray_append(dm_args, "-netdev"); flexarray_append(dm_args, GCSPRINTF("type=tap,id=net%d,ifname=%s," + "br=%s," "script=%s,downscript=%s", nics[i].devid, ifname, + nics[i].bridge, libxl_tapif_script(gc), libxl_tapif_script(gc))); @@ -1825,6 +1829,8 @@ static int libxl__build_device_model_args_new(libxl__gc *gc, flexarray_append(dm_args, GCSPRINTF("%"PRId64, ram_size)); if (b_info->type == LIBXL_DOMAIN_TYPE_HVM) { + flexarray_append_pair(dm_envs, "XEN_DOMAIN_ID", GCSPRINTF("%d", guest_domid)); + if (b_info->u.hvm.hdtype == LIBXL_HDTYPE_AHCI) flexarray_append_pair(dm_args, "-device", "ahci,id=ahci0"); for (i = 0; i < num_disks; i++) {