| Message ID | 54D01E01.6010504@intel.com |
|---|---|
| State | New |
| Headers | show |
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@intel.com> > >>--- > >>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@intel.com> > 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. > 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. Wei. > 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 > > > > >Wei. > > > >>@@ -748,6 +753,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); > >>+ } > >>+ > >> 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]); > >>-- > >>1.9.1 > >
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@intel.com> >>>> --- >>>> 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@intel.com> >> > > 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
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) {