[PATCH] libs/light: pass some infos to qemu

Manuel Bouyer posted 1 patch 2 weeks, 1 day ago
Test gitlab-ci failed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20210112181242.1570-17-bouyer@antioche.eu.org
tools/libs/light/libxl_dm.c | 6 ++++++
1 file changed, 6 insertions(+)

[PATCH] libs/light: pass some infos to qemu

Posted by Manuel Bouyer 2 weeks, 1 day ago
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,
                                            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++) {
-- 
2.29.2


Re: [PATCH] libs/light: pass some infos to qemu

Posted by Roger Pau Monné 1 week, 4 days ago
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.

Re: [PATCH] libs/light: pass some infos to qemu

Posted by Manuel Bouyer 1 week, 4 days ago
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.

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Re: [PATCH] libs/light: pass some infos to qemu

Posted by Roger Pau Monné 1 week, 2 days ago
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.

Re: [PATCH] libs/light: pass some infos to qemu

Posted by Manuel Bouyer 1 day, 5 hours ago
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' ]

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Re: [PATCH] libs/light: pass some infos to qemu

Posted by Roger Pau Monné 19 hours ago
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.

Re: [PATCH] libs/light: pass some infos to qemu

Posted by Manuel Bouyer 18 hours ago
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). 

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Re: [PATCH] libs/light: pass some infos to qemu

Posted by Manuel Bouyer 1 week, 2 days ago
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 ?

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Re: [PATCH] libs/light: pass some infos to qemu

Posted by Roger Pau Monné 1 week, 2 days ago
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.

Re: [PATCH] libs/light: pass some infos to qemu

Posted by Manuel Bouyer 1 week, 2 days ago
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 

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--