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

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

[Manuel Bouyer]

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
The only functional difference of using the br parameter is that the
bridge name gets passed to the QEMU script.
NetBSD doesn't have the ioctl to rename network interfaces implemented, and
thus cannot rename the interface from tapX to vifX.Y-emu. Only qemu knowns
the tap interface name, so we need to use the qemu script from qemu itself.

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

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

[PATCH v3 2/2] Document qemu-ifup on NetBSD

[Manuel Bouyer]

Document that on NetBSD, the tap interface will be configured by the
qemu-ifup script. Document the arguments, and XEN_DOMAIN_ID environnement
variable.
---
 docs/man/xl-network-configuration.5.pod | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/docs/man/xl-network-configuration.5.pod b/docs/man/xl-network-configuration.5.pod
index af058d4d3c..f6eb6c31fc 100644
--- a/docs/man/xl-network-configuration.5.pod
+++ b/docs/man/xl-network-configuration.5.pod
@@ -172,6 +172,10 @@ add it to the relevant bridge). Defaults to
 C<XEN_SCRIPT_DIR/vif-bridge> but can be set to any script. Some example
 scripts are installed in C<XEN_SCRIPT_DIR>.
 
+On NetBSD, HVM guests will always use
+C<XEN_SCRIPT_DIR/qemu-ifup> to configure the tap interface. The first argument
+is the tap interface, the second is the bridge name. the environnement variable
+C<XEN_DOMAIN_ID> contains the domU's ID.
 
 =head2 ip
 
-- 
2.29.2

Re: [PATCH v3 2/2] Document qemu-ifup on NetBSD

[Roger Pau Monné]

On Sun, Jan 31, 2021 at 12:03:00AM +0100, Manuel Bouyer wrote:
> Document that on NetBSD, the tap interface will be configured by the
> qemu-ifup script. Document the arguments, and XEN_DOMAIN_ID environnement
> variable.

You are missing a Signed-off-by tag here ;).

> ---
>  docs/man/xl-network-configuration.5.pod | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/docs/man/xl-network-configuration.5.pod b/docs/man/xl-network-configuration.5.pod
> index af058d4d3c..f6eb6c31fc 100644
> --- a/docs/man/xl-network-configuration.5.pod
> +++ b/docs/man/xl-network-configuration.5.pod
> @@ -172,6 +172,10 @@ add it to the relevant bridge). Defaults to
>  C<XEN_SCRIPT_DIR/vif-bridge> but can be set to any script. Some example
>  scripts are installed in C<XEN_SCRIPT_DIR>.
>  
> +On NetBSD, HVM guests will always use
> +C<XEN_SCRIPT_DIR/qemu-ifup> to configure the tap interface. The first argument
> +is the tap interface, the second is the bridge name. the environnement variable
> +C<XEN_DOMAIN_ID> contains the domU's ID.

LGTM, but I would make it even less technical and more user focused:

Note on NetBSD HVM guests will ignore the script option for tap
(emulated) interfaces and always use C<XEN_SCRIPT_DIR/qemu-ifup> to
configure the interface in bridged mode.

Thanks, Roger.

Re: [PATCH v3 2/2] Document qemu-ifup on NetBSD

[Manuel Bouyer]

On Mon, Feb 01, 2021 at 09:21:58AM +0100, Roger Pau Monné wrote:
> On Sun, Jan 31, 2021 at 12:03:00AM +0100, Manuel Bouyer wrote:
> > Document that on NetBSD, the tap interface will be configured by the
> > qemu-ifup script. Document the arguments, and XEN_DOMAIN_ID environnement
> > variable.
> 
> You are missing a Signed-off-by tag here ;).
> 
> > ---
> >  docs/man/xl-network-configuration.5.pod | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/docs/man/xl-network-configuration.5.pod b/docs/man/xl-network-configuration.5.pod
> > index af058d4d3c..f6eb6c31fc 100644
> > --- a/docs/man/xl-network-configuration.5.pod
> > +++ b/docs/man/xl-network-configuration.5.pod
> > @@ -172,6 +172,10 @@ add it to the relevant bridge). Defaults to
> >  C<XEN_SCRIPT_DIR/vif-bridge> but can be set to any script. Some example
> >  scripts are installed in C<XEN_SCRIPT_DIR>.
> >  
> > +On NetBSD, HVM guests will always use
> > +C<XEN_SCRIPT_DIR/qemu-ifup> to configure the tap interface. The first argument
> > +is the tap interface, the second is the bridge name. the environnement variable
> > +C<XEN_DOMAIN_ID> contains the domU's ID.
> 
> LGTM, but I would make it even less technical and more user focused:
> 
> Note on NetBSD HVM guests will ignore the script option for tap
> (emulated) interfaces and always use C<XEN_SCRIPT_DIR/qemu-ifup> to
> configure the interface in bridged mode.

Well, as a user, I want to know how the scripts are called, so that I can
tune them ...

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

Re: [PATCH v3 2/2] Document qemu-ifup on NetBSD

[Roger Pau Monné]

On Mon, Feb 01, 2021 at 10:37:47AM +0100, Manuel Bouyer wrote:
> On Mon, Feb 01, 2021 at 09:21:58AM +0100, Roger Pau Monné wrote:
> > On Sun, Jan 31, 2021 at 12:03:00AM +0100, Manuel Bouyer wrote:
> > > Document that on NetBSD, the tap interface will be configured by the
> > > qemu-ifup script. Document the arguments, and XEN_DOMAIN_ID environnement
> > > variable.
> > 
> > You are missing a Signed-off-by tag here ;).
> > 
> > > ---
> > >  docs/man/xl-network-configuration.5.pod | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/docs/man/xl-network-configuration.5.pod b/docs/man/xl-network-configuration.5.pod
> > > index af058d4d3c..f6eb6c31fc 100644
> > > --- a/docs/man/xl-network-configuration.5.pod
> > > +++ b/docs/man/xl-network-configuration.5.pod
> > > @@ -172,6 +172,10 @@ add it to the relevant bridge). Defaults to
> > >  C<XEN_SCRIPT_DIR/vif-bridge> but can be set to any script. Some example
> > >  scripts are installed in C<XEN_SCRIPT_DIR>.
> > >  
> > > +On NetBSD, HVM guests will always use
> > > +C<XEN_SCRIPT_DIR/qemu-ifup> to configure the tap interface. The first argument
> > > +is the tap interface, the second is the bridge name. the environnement variable
> > > +C<XEN_DOMAIN_ID> contains the domU's ID.
> > 
> > LGTM, but I would make it even less technical and more user focused:
> > 
> > Note on NetBSD HVM guests will ignore the script option for tap
> > (emulated) interfaces and always use C<XEN_SCRIPT_DIR/qemu-ifup> to
> > configure the interface in bridged mode.
> 
> Well, as a user, I want to know how the scripts are called, so that I can
> tune them ...

Isn't that information on the header of the script? I would expect
users that want to modify such script will open it and then the header
should already lists the parameters.

IMO I would leave the parameters out of this document because we don't
list them for any other script, so it seems odd to list them for the
qemu-ifup script only.

Thanks, Roger.

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

[Ian Jackson]

Manuel Bouyer writes ("[PATCH v3 1/2] libs/light: pass some infos to qemu"):
> 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
> The only functional difference of using the br parameter is that the
> bridge name gets passed to the QEMU script.
> NetBSD doesn't have the ioctl to rename network interfaces implemented, and
> thus cannot rename the interface from tapX to vifX.Y-emu. Only qemu knowns
> the tap interface name, so we need to use the qemu script from qemu itself.
> 
> Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

I think this is a bugfix but it has implications for non-NetBSD
systems and I think it would be best for it to get (or not get) an
explicit release-ack.

I think it is sufficiently low risk to take it now.  We don't think
this will cause trouble for other platforms but if it proves to, that
should be fairly obvious and caught in our testing.  So:

Release-Acked-by: Ian Jackson <iwj@xenproject.org>

Ian.

Re: [PATCH v3 2/2] Document qemu-ifup on NetBSD

[Ian Jackson]

Roger Pau Monné writes ("Re: [PATCH v3 2/2] Document qemu-ifup on NetBSD"):
> On Mon, Feb 01, 2021 at 10:37:47AM +0100, Manuel Bouyer wrote:
> > Well, as a user, I want to know how the scripts are called, so that I can
> > tune them ...
> 
> Isn't that information on the header of the script? I would expect
> users that want to modify such script will open it and then the header
> should already lists the parameters.
> 
> IMO I would leave the parameters out of this document because we don't
> list them for any other script, so it seems odd to list them for the
> qemu-ifup script only.

I think it is shown there, indeed.  That may not be the best place for
this information.  But maybe putting it somewhere else should be done
systematically rather than ad hoc.  IOW with my maintainer hat on I
don't feel I have a strong opinion.

OTOH from my RM hat

Release-Acked-by: Ian Jackson <iwj@xenproject.org>

since this is a documentation change.

Ian.