Re: [Xen-devel] [v2][PATCH] libxl: add one machine property to support IGD GFX passthrough

["Chen, Tiejun" <tiejun.chen@xxxxxxxxx>]

On 2015/2/3 18:19, Wei Liu wrote:
> On Tue, Feb 03, 2015 at 09:01:53AM +0800, Chen, Tiejun wrote:
> > On 2015/2/2 20:19, Wei Liu wrote:
> > > On Mon, Feb 02, 2015 at 09:17:23AM +0800, Tiejun Chen wrote:
> > > > When we're working to support IGD GFX passthrough with qemu
> > > > upstream, instead of "-gfx_passthru" we'd like to make that
> > > > a machine option, "-machine xxx,-igd-passthru=on". This need
> > > > to bring a change on tool side.
> > > >
> > > > Signed-off-by: Tiejun Chen <tiejun.chen@xxxxxxxxx>
> > > > ---
> > > > v2:
> > > >
> > > > * Based on some discussions with Wei we'd like to keep both old
> > > >    option, -gfx_passthru, and new machine property option,
> > > >    "-machine xxx,-igd-passthru=on" at the same time but deprecate
> > > >    the old one. Then finally we remove the old one at that point
> > > >    that to give downstream (in this case, Xen) time to cope with the
> > > >    change.
> > > My suggestion has one premise -- if upstream QEMU has already released
> > > that -gfx_passthru option. If there is no "old one" (in upstream QEMU)
> > > at all, then there is nothing to keep and deprecate.
> > Understood.
> >
> > > >   tools/libxl/libxl_dm.c | 10 ++++++++++
> > > >   1 file changed, 10 insertions(+)
> > > >
> > > > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> > > > index c2b0487..8405f0b 100644
> > > > --- a/tools/libxl/libxl_dm.c
> > > > +++ b/tools/libxl/libxl_dm.c
> > > > @@ -701,6 +701,11 @@ static char ** 
> > > > libxl__build_device_model_args_new(libxl__gc *gc,
> > > Note this function is upstream QEMU specfic.
> > Yeah.
> >
> > > >               flexarray_append(dm_args, "-net");
> > > >               flexarray_append(dm_args, "none");
> > > >           }
> > > > +        /*
> > > > +         * Although we already introduce 'igd-passthru', but we'd like
> > > > +         * to remove this until we give downstream time to cope with
> > > > +         * the change.
> > > > +         */
> > > >           if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
> > > >               flexarray_append(dm_args, "-gfx_passthru");
> > > >           }
> > > The comment contradicts what I know (or what I think I know). In our
> > > last email exchange you said there was no "-gfx_passthru" in any version
> > > of upstream QEMU.
> > >
> > > So, shouldn't you just remove this `if' statement?
> > Right. So what about this?
> >
> >      libxl: add one machine property to support IGD GFX passthrough
> >
> >      When we're working to support IGD GFX passthrough with qemu
> >      upstream, we'd like to introduce a machine option,
> >      "-machine xxx,igd-passthru=on", to enable/disable that feature.
> >      And we also remove that old option, "-gfx_passthru", just from
> >      the case of LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN since actually
> >      no any qemu stream version really need or use that.
> >
> >      Signed-off-by: Tiejun Chen <tiejun.chen@xxxxxxxxx>
> Yes. I think a patch like this reflects the reality.
>
> It would be nice, as Ian J suggested, to state which version of QEMU
> upstream introduces that new option in commit message.
I have to provide this exact info after those qemu stuffs are merged, 
but now at lease you guys agree this option named as 'igd-passthrough', 
its enough to first continue working all qemu patches out. Once its 
finished I will go back.
> > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> > index c2b0487..b888f19 100644
> > --- a/tools/libxl/libxl_dm.c
> > +++ b/tools/libxl/libxl_dm.c
> > @@ -701,9 +701,6 @@ static char **
> > libxl__build_device_model_args_new(libxl__gc *gc,
> >               flexarray_append(dm_args, "-net");
> >               flexarray_append(dm_args, "none");
> >           }
> > -        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
> > -            flexarray_append(dm_args, "-gfx_passthru");
> > -        }
> >       } else {
> >           if (!sdl && !vnc) {
> >               flexarray_append(dm_args, "-nographic");
> > @@ -748,6 +745,11 @@ static char **
> > libxl__build_device_model_args_new(libxl__gc *gc,
> >                                               machinearg, max_ram_below_4g);
> >               }
> >           }
> > +
> > +        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
> > +            machinearg = libxl__sprintf(gc, "%s,igd-passthru=on",
> > machinearg);
> > +        }
> > +
> Please use GCSPRINTF macro.
Sure.

@@ -748,6 +745,11 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,
                                             machinearg, max_ram_below_4g);
             }
         }
+
+        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
+            machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
+        }
+
         flexarray_append(dm_args, machinearg);
         for (i = 0; b_info->extra_hvm && b_info->extra_hvm[i] != NULL; 
i++)
             flexarray_append(dm_args, b_info->extra_hvm[i]);


Thanks
Tiejun

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel