| Message ID | 1425632903-5502-3-git-send-email-tiejun.chen@intel.com |
|---|---|
| State | New |
| Headers | show |
On 2015/3/6 17:08, Tiejun Chen wrote: > Although we already have 'gfx_passthru' in b_info, this doesn' suffice > after we want to handle IGD specifically. Now we define a new field of > type, gfx_passthru_kind, to indicate we're trying to pass IGD. Actually > this means we can benefit this to support other specific devices just > by extending gfx_passthru_kind. And then we can cooperate with > gfx_passthru to address IGD cases as follows: > > gfx_passthru = 0 => sets build_info.u.gfx_passthru to false > gfx_passthru = 1 => sets build_info.u.gfx_passthru to false and > build_info.u.gfx_passthru_kind to DEFAULT > gfx_passthru = "igd" => sets build_info.u.gfx_passthru to false > and build_info.u.gfx_passthru_kind to IGD > > Here if gfx_passthru_kind = DEFAULT, we will call > libxl__is_igd_vga_passthru() to check if we're hitting that table to need > to pass that option to qemu. But if gfx_passthru_kind = "igd" we always > force to pass that. > > Signed-off-by: Tiejun Chen <tiejun.chen@intel.com> > --- > tools/libxl/libxl_dm.c | 13 +++++++++++++ > tools/libxl/libxl_types.idl | 6 ++++++ > tools/libxl/xl_cmdimpl.c | 19 +++++++++++++++++-- > 3 files changed, 36 insertions(+), 2 deletions(-) > > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c > index 8599a6a..780dd2a 100644 > --- a/tools/libxl/libxl_dm.c > +++ b/tools/libxl/libxl_dm.c Sorry, looks I'm missing to remove '-gfx_passthru' since this should be gone in the case of qemu upstream, @@ -710,9 +710,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"); I will fold this into next revision after this review. Thanks Tiejun > @@ -757,6 +757,19 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc, > machinearg, max_ram_below_4g); > } > } > + > + if (b_info->u.hvm.gfx_passthru_kind == > + LIBXL_GFX_PASSTHRU_KIND_DEFAULT) { > + if (libxl__is_igd_vga_passthru(gc, guest_config)) { > + machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg); > + } > + } else if (b_info->u.hvm.gfx_passthru_kind == > + LIBXL_GFX_PASSTHRU_KIND_IGD) { > + machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg); > + } else { > + LOG(WARN, "gfx_passthru_kind is invalid so ignored.\n"); > + } > + > 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]); > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl > index 02be466..e209460 100644 > --- a/tools/libxl/libxl_types.idl > +++ b/tools/libxl/libxl_types.idl > @@ -140,6 +140,11 @@ libxl_tsc_mode = Enumeration("tsc_mode", [ > (3, "native_paravirt"), > ]) > > +libxl_gfx_passthru_kind = Enumeration("gfx_passthru_kind", [ > + (0, "default"), > + (1, "igd"), > + ], init_val = "LIBXL_GFX_PASSTHRU_KIND_DEFAULT") > + > # Consistent with the values defined for HVM_PARAM_TIMER_MODE. > libxl_timer_mode = Enumeration("timer_mode", [ > (-1, "unknown"), > @@ -430,6 +435,7 @@ libxl_domain_build_info = Struct("domain_build_info",[ > ("spice", libxl_spice_info), > > ("gfx_passthru", libxl_defbool), > + ("gfx_passthru_kind", libxl_gfx_passthru_kind), > > ("serial", string), > ("boot", string), > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > index 440db78..3f7fede 100644 > --- a/tools/libxl/xl_cmdimpl.c > +++ b/tools/libxl/xl_cmdimpl.c > @@ -1953,8 +1953,23 @@ skip_vfb: > xlu_cfg_replace_string (config, "spice_streaming_video", > &b_info->u.hvm.spice.streaming_video, 0); > xlu_cfg_get_defbool(config, "nographic", &b_info->u.hvm.nographic, 0); > - xlu_cfg_get_defbool(config, "gfx_passthru", > - &b_info->u.hvm.gfx_passthru, 0); > + if (!xlu_cfg_get_long(config, "gfx_passthru", &l, 1)) { > + if (!l) { > + libxl_defbool_set(&b_info->u.hvm.gfx_passthru, false); > + } else if (i == 1) { > + libxl_defbool_set(&b_info->u.hvm.gfx_passthru, true); > + } else { > + fprintf(stderr, "ERROR: invalid value %ld for \"gfx_passthru\"\n", l); > + exit (1); > + } > + } else if (!xlu_cfg_get_string(config, "gfx_passthru", &buf, 0)) { > + if (libxl_gfx_passthru_kind_from_string(buf, &b_info->u.hvm.gfx_passthru_kind)) { > + fprintf(stderr, "ERROR: invalid value \"%s\" for \"gfx_passthru\"\n", > + buf); > + exit (1); > + } > + libxl_defbool_set(&b_info->u.hvm.gfx_passthru, false); > + } > switch (xlu_cfg_get_list_as_string_list(config, "serial", > &b_info->u.hvm.serial_list, > 1)) >
On Fri, Mar 06, 2015 at 05:08:23PM +0800, Tiejun Chen wrote: > Although we already have 'gfx_passthru' in b_info, this doesn' suffice > after we want to handle IGD specifically. Now we define a new field of > type, gfx_passthru_kind, to indicate we're trying to pass IGD. Actually > this means we can benefit this to support other specific devices just > by extending gfx_passthru_kind. And then we can cooperate with > gfx_passthru to address IGD cases as follows: > > gfx_passthru = 0 => sets build_info.u.gfx_passthru to false > gfx_passthru = 1 => sets build_info.u.gfx_passthru to false and ^^^^^ true? > build_info.u.gfx_passthru_kind to DEFAULT > gfx_passthru = "igd" => sets build_info.u.gfx_passthru to false > and build_info.u.gfx_passthru_kind to IGD > > Here if gfx_passthru_kind = DEFAULT, we will call > libxl__is_igd_vga_passthru() to check if we're hitting that table to need > to pass that option to qemu. But if gfx_passthru_kind = "igd" we always > force to pass that. > > Signed-off-by: Tiejun Chen <tiejun.chen@intel.com> > --- > tools/libxl/libxl_dm.c | 13 +++++++++++++ > tools/libxl/libxl_types.idl | 6 ++++++ > tools/libxl/xl_cmdimpl.c | 19 +++++++++++++++++-- > 3 files changed, 36 insertions(+), 2 deletions(-) > > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c > index 8599a6a..780dd2a 100644 > --- a/tools/libxl/libxl_dm.c > +++ b/tools/libxl/libxl_dm.c > @@ -757,6 +757,19 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc, > machinearg, max_ram_below_4g); > } > } > + > + if (b_info->u.hvm.gfx_passthru_kind == > + LIBXL_GFX_PASSTHRU_KIND_DEFAULT) { > + if (libxl__is_igd_vga_passthru(gc, guest_config)) { > + machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg); > + } You can remove that {} around machinearg -- it's only one line after `if'. > + } else if (b_info->u.hvm.gfx_passthru_kind == > + LIBXL_GFX_PASSTHRU_KIND_IGD) { > + machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg); > + } else { > + LOG(WARN, "gfx_passthru_kind is invalid so ignored.\n"); > + } > + > 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]); > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl > index 02be466..e209460 100644 > --- a/tools/libxl/libxl_types.idl > +++ b/tools/libxl/libxl_types.idl > @@ -140,6 +140,11 @@ libxl_tsc_mode = Enumeration("tsc_mode", [ > (3, "native_paravirt"), > ]) > > +libxl_gfx_passthru_kind = Enumeration("gfx_passthru_kind", [ > + (0, "default"), > + (1, "igd"), > + ], init_val = "LIBXL_GFX_PASSTHRU_KIND_DEFAULT") > + You don't need to set init_val if the default value is 0. > # Consistent with the values defined for HVM_PARAM_TIMER_MODE. > libxl_timer_mode = Enumeration("timer_mode", [ > (-1, "unknown"), > @@ -430,6 +435,7 @@ libxl_domain_build_info = Struct("domain_build_info",[ > ("spice", libxl_spice_info), > > ("gfx_passthru", libxl_defbool), > + ("gfx_passthru_kind", libxl_gfx_passthru_kind), ^^^^ One space is enough, I think. > > ("serial", string), > ("boot", string), > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > index 440db78..3f7fede 100644 > --- a/tools/libxl/xl_cmdimpl.c > +++ b/tools/libxl/xl_cmdimpl.c > @@ -1953,8 +1953,23 @@ skip_vfb: > xlu_cfg_replace_string (config, "spice_streaming_video", > &b_info->u.hvm.spice.streaming_video, 0); > xlu_cfg_get_defbool(config, "nographic", &b_info->u.hvm.nographic, 0); > - xlu_cfg_get_defbool(config, "gfx_passthru", > - &b_info->u.hvm.gfx_passthru, 0); > + if (!xlu_cfg_get_long(config, "gfx_passthru", &l, 1)) { > + if (!l) { > + libxl_defbool_set(&b_info->u.hvm.gfx_passthru, false); > + } else if (i == 1) { > + libxl_defbool_set(&b_info->u.hvm.gfx_passthru, true); > + } else { > + fprintf(stderr, "ERROR: invalid value %ld for \"gfx_passthru\"\n", l); Please wrap this line to be < 80 column. > + exit (1); > + } > + } else if (!xlu_cfg_get_string(config, "gfx_passthru", &buf, 0)) { > + if (libxl_gfx_passthru_kind_from_string(buf, &b_info->u.hvm.gfx_passthru_kind)) { Ditto. > + fprintf(stderr, "ERROR: invalid value \"%s\" for \"gfx_passthru\"\n", Ditto. Wei. > + buf); > + exit (1); > + } > + libxl_defbool_set(&b_info->u.hvm.gfx_passthru, false); > + } > switch (xlu_cfg_get_list_as_string_list(config, "serial", > &b_info->u.hvm.serial_list, > 1)) > -- > 1.9.1
On Fri, Mar 06, 2015 at 05:18:36PM +0800, Chen, Tiejun wrote: > On 2015/3/6 17:08, Tiejun Chen wrote: > >Although we already have 'gfx_passthru' in b_info, this doesn' suffice > >after we want to handle IGD specifically. Now we define a new field of > >type, gfx_passthru_kind, to indicate we're trying to pass IGD. Actually > >this means we can benefit this to support other specific devices just > >by extending gfx_passthru_kind. And then we can cooperate with > >gfx_passthru to address IGD cases as follows: > > > > gfx_passthru = 0 => sets build_info.u.gfx_passthru to false > > gfx_passthru = 1 => sets build_info.u.gfx_passthru to false and > > build_info.u.gfx_passthru_kind to DEFAULT > > gfx_passthru = "igd" => sets build_info.u.gfx_passthru to false > > and build_info.u.gfx_passthru_kind to IGD > > > >Here if gfx_passthru_kind = DEFAULT, we will call > >libxl__is_igd_vga_passthru() to check if we're hitting that table to need > >to pass that option to qemu. But if gfx_passthru_kind = "igd" we always > >force to pass that. > > > >Signed-off-by: Tiejun Chen <tiejun.chen@intel.com> > >--- > > tools/libxl/libxl_dm.c | 13 +++++++++++++ > > tools/libxl/libxl_types.idl | 6 ++++++ > > tools/libxl/xl_cmdimpl.c | 19 +++++++++++++++++-- > > 3 files changed, 36 insertions(+), 2 deletions(-) > > > >diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c > >index 8599a6a..780dd2a 100644 > >--- a/tools/libxl/libxl_dm.c > >+++ b/tools/libxl/libxl_dm.c > > Sorry, looks I'm missing to remove '-gfx_passthru' since this should be gone > in the case of qemu upstream, > > @@ -710,9 +710,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"); > > I will fold this into next revision after this review. > I think this change alone warrants a separate changeset. It should be patch 0 of your series as a preparatory patch. Please remember to elaborate in commit message why that hunk is not needed in upstream QEMU. Wei.
On 2015/3/6 20:55, Wei Liu wrote: > On Fri, Mar 06, 2015 at 05:08:23PM +0800, Tiejun Chen wrote: >> Although we already have 'gfx_passthru' in b_info, this doesn' suffice >> after we want to handle IGD specifically. Now we define a new field of >> type, gfx_passthru_kind, to indicate we're trying to pass IGD. Actually >> this means we can benefit this to support other specific devices just >> by extending gfx_passthru_kind. And then we can cooperate with >> gfx_passthru to address IGD cases as follows: >> >> gfx_passthru = 0 => sets build_info.u.gfx_passthru to false >> gfx_passthru = 1 => sets build_info.u.gfx_passthru to false and > ^^^^^ > true? I just simply pick up this from Campbell's comment but I agree you. > >> build_info.u.gfx_passthru_kind to DEFAULT >> gfx_passthru = "igd" => sets build_info.u.gfx_passthru to false >> and build_info.u.gfx_passthru_kind to IGD >> >> Here if gfx_passthru_kind = DEFAULT, we will call >> libxl__is_igd_vga_passthru() to check if we're hitting that table to need >> to pass that option to qemu. But if gfx_passthru_kind = "igd" we always >> force to pass that. >> >> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com> >> --- >> tools/libxl/libxl_dm.c | 13 +++++++++++++ >> tools/libxl/libxl_types.idl | 6 ++++++ >> tools/libxl/xl_cmdimpl.c | 19 +++++++++++++++++-- >> 3 files changed, 36 insertions(+), 2 deletions(-) >> >> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c >> index 8599a6a..780dd2a 100644 >> --- a/tools/libxl/libxl_dm.c >> +++ b/tools/libxl/libxl_dm.c >> @@ -757,6 +757,19 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc, >> machinearg, max_ram_below_4g); >> } >> } >> + >> + if (b_info->u.hvm.gfx_passthru_kind == >> + LIBXL_GFX_PASSTHRU_KIND_DEFAULT) { >> + if (libxl__is_igd_vga_passthru(gc, guest_config)) { >> + machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg); >> + } > > You can remove that {} around machinearg -- it's only one line after > `if'. Okay. > >> + } else if (b_info->u.hvm.gfx_passthru_kind == >> + LIBXL_GFX_PASSTHRU_KIND_IGD) { >> + machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg); >> + } else { >> + LOG(WARN, "gfx_passthru_kind is invalid so ignored.\n"); >> + } >> + >> 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]); >> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl >> index 02be466..e209460 100644 >> --- a/tools/libxl/libxl_types.idl >> +++ b/tools/libxl/libxl_types.idl >> @@ -140,6 +140,11 @@ libxl_tsc_mode = Enumeration("tsc_mode", [ >> (3, "native_paravirt"), >> ]) >> >> +libxl_gfx_passthru_kind = Enumeration("gfx_passthru_kind", [ >> + (0, "default"), >> + (1, "igd"), >> + ], init_val = "LIBXL_GFX_PASSTHRU_KIND_DEFAULT") >> + > > You don't need to set init_val if the default value is 0. Got it. > >> # Consistent with the values defined for HVM_PARAM_TIMER_MODE. >> libxl_timer_mode = Enumeration("timer_mode", [ >> (-1, "unknown"), >> @@ -430,6 +435,7 @@ libxl_domain_build_info = Struct("domain_build_info",[ >> ("spice", libxl_spice_info), >> >> ("gfx_passthru", libxl_defbool), >> + ("gfx_passthru_kind", libxl_gfx_passthru_kind), > ^^^^ > One space is enough, I think. Okay. > >> >> ("serial", string), >> ("boot", string), >> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c >> index 440db78..3f7fede 100644 >> --- a/tools/libxl/xl_cmdimpl.c >> +++ b/tools/libxl/xl_cmdimpl.c >> @@ -1953,8 +1953,23 @@ skip_vfb: >> xlu_cfg_replace_string (config, "spice_streaming_video", >> &b_info->u.hvm.spice.streaming_video, 0); >> xlu_cfg_get_defbool(config, "nographic", &b_info->u.hvm.nographic, 0); >> - xlu_cfg_get_defbool(config, "gfx_passthru", >> - &b_info->u.hvm.gfx_passthru, 0); >> + if (!xlu_cfg_get_long(config, "gfx_passthru", &l, 1)) { >> + if (!l) { >> + libxl_defbool_set(&b_info->u.hvm.gfx_passthru, false); >> + } else if (i == 1) { >> + libxl_defbool_set(&b_info->u.hvm.gfx_passthru, true); >> + } else { >> + fprintf(stderr, "ERROR: invalid value %ld for \"gfx_passthru\"\n", l); > > Please wrap this line to be < 80 column. > >> + exit (1); >> + } >> + } else if (!xlu_cfg_get_string(config, "gfx_passthru", &buf, 0)) { >> + if (libxl_gfx_passthru_kind_from_string(buf, &b_info->u.hvm.gfx_passthru_kind)) { > > Ditto. > >> + fprintf(stderr, "ERROR: invalid value \"%s\" for \"gfx_passthru\"\n", > > Ditto. So, @@ -1959,13 +1959,15 @@ skip_vfb: } else if (i == 1) { libxl_defbool_set(&b_info->u.hvm.gfx_passthru, true); } else { - fprintf(stderr, "ERROR: invalid value %ld for \"gfx_passthru\"\n", l); + fprintf(stderr, "ERROR: invalid value %ld for" + " \"gfx_passthru\"\n", l); exit (1); } } else if (!xlu_cfg_get_string(config, "gfx_passthru", &buf, 0)) { - if (libxl_gfx_passthru_kind_from_string(buf, &b_info->u.hvm.gfx_passthru_kind)) { - fprintf(stderr, "ERROR: invalid value \"%s\" for \"gfx_passthru\"\n", - buf); + if (libxl_gfx_passthru_kind_from_string(buf, + &b_info->u.hvm.gfx_passthru_kind)) { + fprintf(stderr, "ERROR: invalid value \"%s\" for" + " \"gfx_passthru\"\n", buf); exit (1); } libxl_defbool_set(&b_info->u.hvm.gfx_passthru, false); Thanks Tiejun
On 2015/3/6 20:59, Wei Liu wrote: > On Fri, Mar 06, 2015 at 05:18:36PM +0800, Chen, Tiejun wrote: >> On 2015/3/6 17:08, Tiejun Chen wrote: >>> Although we already have 'gfx_passthru' in b_info, this doesn' suffice >>> after we want to handle IGD specifically. Now we define a new field of >>> type, gfx_passthru_kind, to indicate we're trying to pass IGD. Actually >>> this means we can benefit this to support other specific devices just >>> by extending gfx_passthru_kind. And then we can cooperate with >>> gfx_passthru to address IGD cases as follows: >>> >>> gfx_passthru = 0 => sets build_info.u.gfx_passthru to false >>> gfx_passthru = 1 => sets build_info.u.gfx_passthru to false and >>> build_info.u.gfx_passthru_kind to DEFAULT >>> gfx_passthru = "igd" => sets build_info.u.gfx_passthru to false >>> and build_info.u.gfx_passthru_kind to IGD >>> >>> Here if gfx_passthru_kind = DEFAULT, we will call >>> libxl__is_igd_vga_passthru() to check if we're hitting that table to need >>> to pass that option to qemu. But if gfx_passthru_kind = "igd" we always >>> force to pass that. >>> >>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com> >>> --- >>> tools/libxl/libxl_dm.c | 13 +++++++++++++ >>> tools/libxl/libxl_types.idl | 6 ++++++ >>> tools/libxl/xl_cmdimpl.c | 19 +++++++++++++++++-- >>> 3 files changed, 36 insertions(+), 2 deletions(-) >>> >>> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c >>> index 8599a6a..780dd2a 100644 >>> --- a/tools/libxl/libxl_dm.c >>> +++ b/tools/libxl/libxl_dm.c >> >> Sorry, looks I'm missing to remove '-gfx_passthru' since this should be gone >> in the case of qemu upstream, >> >> @@ -710,9 +710,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"); >> >> I will fold this into next revision after this review. >> > > I think this change alone warrants a separate changeset. It should be > patch 0 of your series as a preparatory patch. Please remember to > elaborate in commit message why that hunk is not needed in upstream > QEMU. > What about this? "-gfx_passthru" is just introduced to work for qemu-xen-traditional so we should get this away from libxl__build_device_model_args_new() in the case of qemu upstream. Thanks Tiejun
On Mon, Mar 09, 2015 at 02:50:42PM +0800, Chen, Tiejun wrote: > On 2015/3/6 20:59, Wei Liu wrote: > >On Fri, Mar 06, 2015 at 05:18:36PM +0800, Chen, Tiejun wrote: > >>On 2015/3/6 17:08, Tiejun Chen wrote: > >>>Although we already have 'gfx_passthru' in b_info, this doesn' suffice > >>>after we want to handle IGD specifically. Now we define a new field of > >>>type, gfx_passthru_kind, to indicate we're trying to pass IGD. Actually > >>>this means we can benefit this to support other specific devices just > >>>by extending gfx_passthru_kind. And then we can cooperate with > >>>gfx_passthru to address IGD cases as follows: > >>> > >>> gfx_passthru = 0 => sets build_info.u.gfx_passthru to false > >>> gfx_passthru = 1 => sets build_info.u.gfx_passthru to false and > >>> build_info.u.gfx_passthru_kind to DEFAULT > >>> gfx_passthru = "igd" => sets build_info.u.gfx_passthru to false > >>> and build_info.u.gfx_passthru_kind to IGD > >>> > >>>Here if gfx_passthru_kind = DEFAULT, we will call > >>>libxl__is_igd_vga_passthru() to check if we're hitting that table to need > >>>to pass that option to qemu. But if gfx_passthru_kind = "igd" we always > >>>force to pass that. > >>> > >>>Signed-off-by: Tiejun Chen <tiejun.chen@intel.com> > >>>--- > >>> tools/libxl/libxl_dm.c | 13 +++++++++++++ > >>> tools/libxl/libxl_types.idl | 6 ++++++ > >>> tools/libxl/xl_cmdimpl.c | 19 +++++++++++++++++-- > >>> 3 files changed, 36 insertions(+), 2 deletions(-) > >>> > >>>diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c > >>>index 8599a6a..780dd2a 100644 > >>>--- a/tools/libxl/libxl_dm.c > >>>+++ b/tools/libxl/libxl_dm.c > >> > >>Sorry, looks I'm missing to remove '-gfx_passthru' since this should be gone > >>in the case of qemu upstream, > >> > >>@@ -710,9 +710,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"); > >> > >>I will fold this into next revision after this review. > >> > > > >I think this change alone warrants a separate changeset. It should be > >patch 0 of your series as a preparatory patch. Please remember to > >elaborate in commit message why that hunk is not needed in upstream > >QEMU. > > > > What about this? > > "-gfx_passthru" is just introduced to work for qemu-xen-traditional so we > should get this away from libxl__build_device_model_args_new() in the case > of qemu upstream. > This looks good. Wei. > Thanks > Tiejun
On Mon, Mar 09, 2015 at 02:45:36PM +0800, Chen, Tiejun wrote: [...] > > > >>+ exit (1); > >>+ } > >>+ } else if (!xlu_cfg_get_string(config, "gfx_passthru", &buf, 0)) { > >>+ if (libxl_gfx_passthru_kind_from_string(buf, &b_info->u.hvm.gfx_passthru_kind)) { > > > >Ditto. > > > >>+ fprintf(stderr, "ERROR: invalid value \"%s\" for \"gfx_passthru\"\n", > > > >Ditto. > > So, > > @@ -1959,13 +1959,15 @@ skip_vfb: > } else if (i == 1) { > libxl_defbool_set(&b_info->u.hvm.gfx_passthru, true); > } else { > - fprintf(stderr, "ERROR: invalid value %ld for > \"gfx_passthru\"\n", l); > + fprintf(stderr, "ERROR: invalid value %ld for" > + " \"gfx_passthru\"\n", l); > exit (1); > } > } else if (!xlu_cfg_get_string(config, "gfx_passthru", &buf, 0)) { > - if (libxl_gfx_passthru_kind_from_string(buf, > &b_info->u.hvm.gfx_passthru_kind)) { > - fprintf(stderr, "ERROR: invalid value \"%s\" for > \"gfx_passthru\"\n", > - buf); > + if (libxl_gfx_passthru_kind_from_string(buf, > + &b_info->u.hvm.gfx_passthru_kind)) { > + fprintf(stderr, "ERROR: invalid value \"%s\" for" > + " \"gfx_passthru\"\n", buf); > exit (1); > } > libxl_defbool_set(&b_info->u.hvm.gfx_passthru, false); > Your changes are mangled by your email client. But we don't normally split the format string so that it's easier to grep. What we normally do is fprintf(stderr, "FORMAT STRING", a, b, c); If format string still treads over 80 column it's fine. We can live with that. Wei. > Thanks > Tiejun
On 2015/3/9 18:17, Wei Liu wrote: > On Mon, Mar 09, 2015 at 02:45:36PM +0800, Chen, Tiejun wrote: > [...] >>> >>>> + exit (1); >>>> + } >>>> + } else if (!xlu_cfg_get_string(config, "gfx_passthru", &buf, 0)) { >>>> + if (libxl_gfx_passthru_kind_from_string(buf, &b_info->u.hvm.gfx_passthru_kind)) { >>> >>> Ditto. >>> >>>> + fprintf(stderr, "ERROR: invalid value \"%s\" for \"gfx_passthru\"\n", >>> >>> Ditto. >> >> So, >> >> @@ -1959,13 +1959,15 @@ skip_vfb: >> } else if (i == 1) { >> libxl_defbool_set(&b_info->u.hvm.gfx_passthru, true); >> } else { >> - fprintf(stderr, "ERROR: invalid value %ld for >> \"gfx_passthru\"\n", l); >> + fprintf(stderr, "ERROR: invalid value %ld for" >> + " \"gfx_passthru\"\n", l); >> exit (1); >> } >> } else if (!xlu_cfg_get_string(config, "gfx_passthru", &buf, 0)) { >> - if (libxl_gfx_passthru_kind_from_string(buf, >> &b_info->u.hvm.gfx_passthru_kind)) { >> - fprintf(stderr, "ERROR: invalid value \"%s\" for >> \"gfx_passthru\"\n", >> - buf); >> + if (libxl_gfx_passthru_kind_from_string(buf, >> + &b_info->u.hvm.gfx_passthru_kind)) { >> + fprintf(stderr, "ERROR: invalid value \"%s\" for" >> + " \"gfx_passthru\"\n", buf); >> exit (1); >> } >> libxl_defbool_set(&b_info->u.hvm.gfx_passthru, false); >> > > Your changes are mangled by your email client. But we don't normally > split the format string so that it's easier to grep. Yeah. > > What we normally do is > > fprintf(stderr, > "FORMAT STRING", > a, b, c); > > If format string still treads over 80 column it's fine. We can live > with that. > Understood. Thanks Tiejun
On Mon, 2015-03-09 at 14:45 +0800, Chen, Tiejun wrote: > On 2015/3/6 20:55, Wei Liu wrote: > > On Fri, Mar 06, 2015 at 05:08:23PM +0800, Tiejun Chen wrote: > >> Although we already have 'gfx_passthru' in b_info, this doesn' suffice ^t > >> after we want to handle IGD specifically. Now we define a new field of > >> type, gfx_passthru_kind, to indicate we're trying to pass IGD. Actually > >> this means we can benefit this to support other specific devices just > >> by extending gfx_passthru_kind. And then we can cooperate with > >> gfx_passthru to address IGD cases as follows: > >> > >> gfx_passthru = 0 => sets build_info.u.gfx_passthru to false > >> gfx_passthru = 1 => sets build_info.u.gfx_passthru to false and > > ^^^^^ > > true? > > I just simply pick up this from Campbell's comment but I agree you. Sorry! Wei is right.
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c index 8599a6a..780dd2a 100644 --- a/tools/libxl/libxl_dm.c +++ b/tools/libxl/libxl_dm.c @@ -757,6 +757,19 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc, machinearg, max_ram_below_4g); } } + + if (b_info->u.hvm.gfx_passthru_kind == + LIBXL_GFX_PASSTHRU_KIND_DEFAULT) { + if (libxl__is_igd_vga_passthru(gc, guest_config)) { + machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg); + } + } else if (b_info->u.hvm.gfx_passthru_kind == + LIBXL_GFX_PASSTHRU_KIND_IGD) { + machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg); + } else { + LOG(WARN, "gfx_passthru_kind is invalid so ignored.\n"); + } + 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]); diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl index 02be466..e209460 100644 --- a/tools/libxl/libxl_types.idl +++ b/tools/libxl/libxl_types.idl @@ -140,6 +140,11 @@ libxl_tsc_mode = Enumeration("tsc_mode", [ (3, "native_paravirt"), ]) +libxl_gfx_passthru_kind = Enumeration("gfx_passthru_kind", [ + (0, "default"), + (1, "igd"), + ], init_val = "LIBXL_GFX_PASSTHRU_KIND_DEFAULT") + # Consistent with the values defined for HVM_PARAM_TIMER_MODE. libxl_timer_mode = Enumeration("timer_mode", [ (-1, "unknown"), @@ -430,6 +435,7 @@ libxl_domain_build_info = Struct("domain_build_info",[ ("spice", libxl_spice_info), ("gfx_passthru", libxl_defbool), + ("gfx_passthru_kind", libxl_gfx_passthru_kind), ("serial", string), ("boot", string), diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c index 440db78..3f7fede 100644 --- a/tools/libxl/xl_cmdimpl.c +++ b/tools/libxl/xl_cmdimpl.c @@ -1953,8 +1953,23 @@ skip_vfb: xlu_cfg_replace_string (config, "spice_streaming_video", &b_info->u.hvm.spice.streaming_video, 0); xlu_cfg_get_defbool(config, "nographic", &b_info->u.hvm.nographic, 0); - xlu_cfg_get_defbool(config, "gfx_passthru", - &b_info->u.hvm.gfx_passthru, 0); + if (!xlu_cfg_get_long(config, "gfx_passthru", &l, 1)) { + if (!l) { + libxl_defbool_set(&b_info->u.hvm.gfx_passthru, false); + } else if (i == 1) { + libxl_defbool_set(&b_info->u.hvm.gfx_passthru, true); + } else { + fprintf(stderr, "ERROR: invalid value %ld for \"gfx_passthru\"\n", l); + exit (1); + } + } else if (!xlu_cfg_get_string(config, "gfx_passthru", &buf, 0)) { + if (libxl_gfx_passthru_kind_from_string(buf, &b_info->u.hvm.gfx_passthru_kind)) { + fprintf(stderr, "ERROR: invalid value \"%s\" for \"gfx_passthru\"\n", + buf); + exit (1); + } + libxl_defbool_set(&b_info->u.hvm.gfx_passthru, false); + } switch (xlu_cfg_get_list_as_string_list(config, "serial", &b_info->u.hvm.serial_list, 1))
Although we already have 'gfx_passthru' in b_info, this doesn' suffice after we want to handle IGD specifically. Now we define a new field of type, gfx_passthru_kind, to indicate we're trying to pass IGD. Actually this means we can benefit this to support other specific devices just by extending gfx_passthru_kind. And then we can cooperate with gfx_passthru to address IGD cases as follows: gfx_passthru = 0 => sets build_info.u.gfx_passthru to false gfx_passthru = 1 => sets build_info.u.gfx_passthru to false and build_info.u.gfx_passthru_kind to DEFAULT gfx_passthru = "igd" => sets build_info.u.gfx_passthru to false and build_info.u.gfx_passthru_kind to IGD Here if gfx_passthru_kind = DEFAULT, we will call libxl__is_igd_vga_passthru() to check if we're hitting that table to need to pass that option to qemu. But if gfx_passthru_kind = "igd" we always force to pass that. Signed-off-by: Tiejun Chen <tiejun.chen@intel.com> --- tools/libxl/libxl_dm.c | 13 +++++++++++++ tools/libxl/libxl_types.idl | 6 ++++++ tools/libxl/xl_cmdimpl.c | 19 +++++++++++++++++-- 3 files changed, 36 insertions(+), 2 deletions(-)