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

[Manuel Bouyer <bouyer@xxxxxxxxxxxxxxx>]

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@xxxxxxxxxx>
> > > > 
> > > > 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@xxxxxxxxxx>
> > > > ---
> > > >  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@xxxxxxxxxxxxxxx>
     NetBSD: 26 ans d'experience feront toujours la difference
--