Re: [Qemu-devel] [PATCH 2/2] libxl: introduce gfx_passthru_kind

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

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