diff mbox series

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

Message ID 20210126224800.1246-12-bouyer@netbsd.org (mailing list archive)
State Superseded
Headers show
Series [v2] libs/light: pass some infos to qemu | expand

Commit Message

Manuel Bouyer Jan. 26, 2021, 10:47 p.m. UTC
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>
---
 tools/libs/light/libxl_dm.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Roger Pau Monné Jan. 28, 2021, 11:08 a.m. UTC | #1
On Tue, Jan 26, 2021 at 11:47:58PM +0100, Manuel Bouyer wrote:
> 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>

If you have a moment might be worth adding a note in
xl-network-configuration.5.pod that NetBSD in HVM mode requires
bridged networking I think?

Also, the qemu-ifup script doesn't seem to be part of the NetBSD
scripts that are upstream, is this something carried by the NetBSD
package?

I certainly don't mind adding those extra parameters/env variables,
but might be nice to clarify the expectations, that can be done in a
separate patch.

Thanks, Roger.
Manuel Bouyer Jan. 29, 2021, 10:46 a.m. UTC | #2
On Thu, Jan 28, 2021 at 12:08:02PM +0100, Roger Pau Monné wrote:
> On Tue, Jan 26, 2021 at 11:47:58PM +0100, Manuel Bouyer wrote:
> > 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>
> 
> If you have a moment might be worth adding a note in
> xl-network-configuration.5.pod that NetBSD in HVM mode requires
> bridged networking I think?

With the default qemu-ifup script only. As you can do whatever you want
in the script, you can support whatever network configuration you want.
It's quite easy to do IP routing for example.

> 
> Also, the qemu-ifup script doesn't seem to be part of the NetBSD
> scripts that are upstream, is this something carried by the NetBSD
> package?

Ha maybe I overlooked this. I'll add it, but maybe it can be submitted in
a separate patch ?
Roger Pau Monné Jan. 29, 2021, 2:52 p.m. UTC | #3
On Fri, Jan 29, 2021 at 11:46:53AM +0100, Manuel Bouyer wrote:
> On Thu, Jan 28, 2021 at 12:08:02PM +0100, Roger Pau Monné wrote:
> > On Tue, Jan 26, 2021 at 11:47:58PM +0100, Manuel Bouyer wrote:
> > > 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>
> > 
> > If you have a moment might be worth adding a note in
> > xl-network-configuration.5.pod that NetBSD in HVM mode requires
> > bridged networking I think?
> 
> With the default qemu-ifup script only. As you can do whatever you want
> in the script, you can support whatever network configuration you want.
> It's quite easy to do IP routing for example.

Right, but the default script provided will do bridging mode only, and
even if you add 'script=vif-ip' to the network configuration line it
won't do what you expect. Instead it will try to add the tap network
interface to the default xenbr0 bridge.

I'm not opposed to having it this way right now, as it's better to
have this than no support at all, but we should have the shortcoming
documented somewhere. Can be done as a separate patch.

> > 
> > Also, the qemu-ifup script doesn't seem to be part of the NetBSD
> > scripts that are upstream, is this something carried by the NetBSD
> > package?
> 
> Ha maybe I overlooked this. I'll add it, but maybe it can be submitted in
> a separate patch ?

Yes, that's fine, can be added as a separate patch.

Thanks, Roger.
Manuel Bouyer Jan. 30, 2021, 11:50 a.m. UTC | #4
On Thu, Jan 28, 2021 at 12:08:02PM +0100, Roger Pau Monné wrote:
> [...]
> Also, the qemu-ifup script doesn't seem to be part of the NetBSD
> scripts that are upstream, is this something carried by the NetBSD
> package?

Actually, the script is part of qemu-xen-traditional:
tools/qemu-xen-traditional/i386-dm/qemu-ifup-NetBSD

and it's installed as part of 'make install'. The same script can be used
for both qemu-xen-traditional and qemu-xen as long as we support only
bridged mode by default.

qemu-xen-traditional did call the script with the bridge name.
This patch makes qemu-xen call the script with the same parameters,
and add the XEN_DOMAIN_ID environnement variable.

Is it OK to keep the script from qemu-xen-traditional (and installed as
part of qemu-xen-traditional) for now ?
Manuel Bouyer Jan. 30, 2021, 11:07 p.m. UTC | #5
On Fri, Jan 29, 2021 at 03:52:14PM +0100, Roger Pau Monné wrote:
> 
> Right, but the default script provided will do bridging mode only, and
> even if you add 'script=vif-ip' to the network configuration line it
> won't do what you expect. Instead it will try to add the tap network
> interface to the default xenbr0 bridge.
> 
> I'm not opposed to having it this way right now, as it's better to
> have this than no support at all, but we should have the shortcoming
> documented somewhere. Can be done as a separate patch.

I just sent a v3, with a patch to xl-network-configuration.5.pod
Roger Pau Monné Feb. 1, 2021, 8:06 a.m. UTC | #6
On Sat, Jan 30, 2021 at 12:50:13PM +0100, Manuel Bouyer wrote:
> On Thu, Jan 28, 2021 at 12:08:02PM +0100, Roger Pau Monné wrote:
> > [...]
> > Also, the qemu-ifup script doesn't seem to be part of the NetBSD
> > scripts that are upstream, is this something carried by the NetBSD
> > package?
> 
> Actually, the script is part of qemu-xen-traditional:
> tools/qemu-xen-traditional/i386-dm/qemu-ifup-NetBSD
> 
> and it's installed as part of 'make install'. The same script can be used
> for both qemu-xen-traditional and qemu-xen as long as we support only
> bridged mode by default.
> 
> qemu-xen-traditional did call the script with the bridge name.
> This patch makes qemu-xen call the script with the same parameters,
> and add the XEN_DOMAIN_ID environnement variable.
> 
> Is it OK to keep the script from qemu-xen-traditional (and installed as
> part of qemu-xen-traditional) for now ?

I think you want to move the script into hotplug/NetBSD/ because it
should be possible to install a system without qemu-xen-traditional
(--disable-qemu-traditional) and only qemu-upstream, and the script
will still be needed in that case.

Thanks, Roger.
Manuel Bouyer Feb. 1, 2021, 9:39 a.m. UTC | #7
On Mon, Feb 01, 2021 at 09:06:13AM +0100, Roger Pau Monné wrote:
> On Sat, Jan 30, 2021 at 12:50:13PM +0100, Manuel Bouyer wrote:
> > On Thu, Jan 28, 2021 at 12:08:02PM +0100, Roger Pau Monné wrote:
> > > [...]
> > > Also, the qemu-ifup script doesn't seem to be part of the NetBSD
> > > scripts that are upstream, is this something carried by the NetBSD
> > > package?
> > 
> > Actually, the script is part of qemu-xen-traditional:
> > tools/qemu-xen-traditional/i386-dm/qemu-ifup-NetBSD
> > 
> > and it's installed as part of 'make install'. The same script can be used
> > for both qemu-xen-traditional and qemu-xen as long as we support only
> > bridged mode by default.
> > 
> > qemu-xen-traditional did call the script with the bridge name.
> > This patch makes qemu-xen call the script with the same parameters,
> > and add the XEN_DOMAIN_ID environnement variable.
> > 
> > Is it OK to keep the script from qemu-xen-traditional (and installed as
> > part of qemu-xen-traditional) for now ?
> 
> I think you want to move the script into hotplug/NetBSD/ because it
> should be possible to install a system without qemu-xen-traditional
> (--disable-qemu-traditional) and only qemu-upstream, and the script
> will still be needed in that case.

I can, but how do I get the ecript removed from qemu-traditional ?
It's a different repo, isn't it ?
Roger Pau Monné Feb. 1, 2021, 10:54 a.m. UTC | #8
On Mon, Feb 01, 2021 at 10:39:39AM +0100, Manuel Bouyer wrote:
> On Mon, Feb 01, 2021 at 09:06:13AM +0100, Roger Pau Monné wrote:
> > On Sat, Jan 30, 2021 at 12:50:13PM +0100, Manuel Bouyer wrote:
> > > On Thu, Jan 28, 2021 at 12:08:02PM +0100, Roger Pau Monné wrote:
> > > > [...]
> > > > Also, the qemu-ifup script doesn't seem to be part of the NetBSD
> > > > scripts that are upstream, is this something carried by the NetBSD
> > > > package?
> > > 
> > > Actually, the script is part of qemu-xen-traditional:
> > > tools/qemu-xen-traditional/i386-dm/qemu-ifup-NetBSD
> > > 
> > > and it's installed as part of 'make install'. The same script can be used
> > > for both qemu-xen-traditional and qemu-xen as long as we support only
> > > bridged mode by default.
> > > 
> > > qemu-xen-traditional did call the script with the bridge name.
> > > This patch makes qemu-xen call the script with the same parameters,
> > > and add the XEN_DOMAIN_ID environnement variable.
> > > 
> > > Is it OK to keep the script from qemu-xen-traditional (and installed as
> > > part of qemu-xen-traditional) for now ?
> > 
> > I think you want to move the script into hotplug/NetBSD/ because it
> > should be possible to install a system without qemu-xen-traditional
> > (--disable-qemu-traditional) and only qemu-upstream, and the script
> > will still be needed in that case.
> 
> I can, but how do I get the ecript removed from qemu-traditional ?
> It's a different repo, isn't it ?

Yes, it's:

http://xenbits.xen.org/gitweb/?p=qemu-xen-traditional.git;a=summary

I would remove it from qemu-trad and then only install from
hotplug/NetBSD if it's not already there? Or maybe just force-install
it from hotplug/NetBSD even if it's already present?

Thanks, Roger.
Manuel Bouyer Feb. 1, 2021, 11:21 a.m. UTC | #9
On Mon, Feb 01, 2021 at 11:54:35AM +0100, Roger Pau Monné wrote:
> > I can, but how do I get the ecript removed from qemu-traditional ?
> > It's a different repo, isn't it ?
> 
> Yes, it's:
> 
> http://xenbits.xen.org/gitweb/?p=qemu-xen-traditional.git;a=summary
> 
> I would remove it from qemu-trad and then only install from
> hotplug/NetBSD if it's not already there? Or maybe just force-install
> it from hotplug/NetBSD even if it's already present?

OK will try that
diff mbox series

Patch

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++) {