[v2,2/2] libxl: introduce gfx_passthru_kind

> This duplicates the code from above. I think this would be best done as:
>
> static int libxl__detect_gfx_passthru_kind(libxl__gc *gc, guest_config)
> {
>      if (b_info->u.hvm.gfx_passthru_kind != LIBXL_GFX_PASSTHRU_KIND_DEFAULT)
>          return 0;
>
>      if (libxl__is_igd_vga_passthru(gc, guest_config)) {
>          b_info->u.hvm.gfx_passthru_kind = LIBXL_GFX_PASSTHRU_KIND_IGD;
>          return 0;
>      }
>
>      LOG(ERROR, "Unable to detect graphics passthru kind");
>      return 1;
> }
>
> Then for the code in libxl__build_device_model_args_new:
>
>           if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>               if (!libxl__detect_gfx_passthru_kind(gc, guest_config))
>                    return NULL
>               switch (b_info->u.hvm.gfx_passthru_kind) {
>               case LIBXL_GFX_PASSTHRU_KIND_IGD:
>                   machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
>                   break;
>               default:
>                   LOG(ERROR, "unknown gfx_passthru_kind\n");
>                  return NULL;
>               }
>          }
>
> That is, a helper to try and autodetect kind if it is default and then a
> single switch entry for each kind.
>
>> +            default:
>> +                LOG(WARN, "gfx_passthru_kind is invalid so ignored.\n");
>
> Please return an error here, as I've shown above.

Looks good and thanks, but here 'guest_config' is a const so we 
shouldn't/can't reset b_info->u.hvm.gfx_passthru_kind like this,

b_info->u.hvm.gfx_passthru_kind = LIBXL_GFX_PASSTHRU_KIND_IGD;

So I tried to refactor a little bit to follow up yours,

*guest_config,
@@ -427,7 +444,7 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,
      const char *keymap = dm_keymap(guest_config);
      char *machinearg;
      flexarray_t *dm_args;
-    int i, connection, devid;
+    int i, connection, devid, gfx_passthru_kind;
      uint64_t ram_size;
      const char *path, *chardev;

@@ -710,9 +727,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");
@@ -757,6 +771,20 @@ 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)) {
+            gfx_passthru_kind = libxl__detect_gfx_passthru_kind(gc,
+ 
guest_config);
+            switch (gfx_passthru_kind) {
+            case LIBXL_GFX_PASSTHRU_KIND_IGD:
+                machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
+                break;
+            default:
+                LOG(ERROR, "unknown gfx_passthru_kind\n");
+                return NULL;
+            }
+        }
+
          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_internal.h b/tools/libxl/libxl_internal.h
>> index c97c62d..8912421 100644
>> --- a/tools/libxl/libxl_internal.h
>> +++ b/tools/libxl/libxl_internal.h
>> @@ -3632,6 +3632,11 @@ static inline void
>> libxl__update_config_vtpm(libxl__gc *gc,
>>     */
>>    void libxl__bitmap_copy_best_effort(libxl__gc *gc, libxl_bitmap *dptr,
>>                                        const libxl_bitmap *sptr);
>> +
>> +#ifdef LIBXL_HAVE_GFX_PASSTHRU_KIND
>
> No need for this ifdef.

Removed.

Thanks
Tiejun

On Thu, 2015-03-19 at 10:07 +0800, Chen, Tiejun wrote:
> > This duplicates the code from above. I think this would be best done as:
> >
> > static int libxl__detect_gfx_passthru_kind(libxl__gc *gc, guest_config)
> > {
> >      if (b_info->u.hvm.gfx_passthru_kind != LIBXL_GFX_PASSTHRU_KIND_DEFAULT)
> >          return 0;
> >
> >      if (libxl__is_igd_vga_passthru(gc, guest_config)) {
> >          b_info->u.hvm.gfx_passthru_kind = LIBXL_GFX_PASSTHRU_KIND_IGD;
> >          return 0;
> >      }
> >
> >      LOG(ERROR, "Unable to detect graphics passthru kind");
> >      return 1;
> > }
> >
> > Then for the code in libxl__build_device_model_args_new:
> >
> >           if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
> >               if (!libxl__detect_gfx_passthru_kind(gc, guest_config))
> >                    return NULL
> >               switch (b_info->u.hvm.gfx_passthru_kind) {
> >               case LIBXL_GFX_PASSTHRU_KIND_IGD:
> >                   machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
> >                   break;
> >               default:
> >                   LOG(ERROR, "unknown gfx_passthru_kind\n");
> >                  return NULL;
> >               }
> >          }
> >
> > That is, a helper to try and autodetect kind if it is default and then a
> > single switch entry for each kind.
> >
> >> +            default:
> >> +                LOG(WARN, "gfx_passthru_kind is invalid so ignored.\n");
> >
> > Please return an error here, as I've shown above.
> 
> Looks good and thanks, but here 'guest_config' is a const so we 
> shouldn't/can't reset b_info->u.hvm.gfx_passthru_kind like this,
> 
> b_info->u.hvm.gfx_passthru_kind = LIBXL_GFX_PASSTHRU_KIND_IGD;
> 
> So I tried to refactor a little bit to follow up yours,
> 
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 8599a6a..605b17c 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -409,6 +409,23 @@ static char *dm_spice_options(libxl__gc *gc,
>       return opt;
>   }
> 
> +static int
> +libxl__detect_gfx_passthru_kind(libxl__gc *gc,
> +                                const libxl_domain_config *guest_config)
> +{
> +    const libxl_domain_build_info *b_info = &guest_config->b_info;
> +
> +    if (b_info->u.hvm.gfx_passthru_kind != LIBXL_GFX_PASSTHRU_KIND_DEFAULT)
> +        return b_info->u.hvm.gfx_passthru_kind;
> +
> +    if (libxl__is_igd_vga_passthru(gc, guest_config)) {
> +        return LIBXL_GFX_PASSTHRU_KIND_IGD;
> +    }
> +
> +    LOG(ERROR, "Unable to detect graphics passthru kind");
> +    return -1;

I think you can make this function return enum
libxl_gfx_passthrough_kind and then return
LIBXL_GFX_PASSTHRU_KIND_DEFAULT in this case.

> +}
> +
>   static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>                                           const char *dm, int guest_domid,
>                                           const libxl_domain_config 
> *guest_config,
> @@ -427,7 +444,7 @@ static char ** 
> libxl__build_device_model_args_new(libxl__gc *gc,
>       const char *keymap = dm_keymap(guest_config);
>       char *machinearg;
>       flexarray_t *dm_args;
> -    int i, connection, devid;
> +    int i, connection, devid, gfx_passthru_kind;

Please declare this in the smallest necessary scope...
[...]
> +        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
> +            gfx_passthru_kind = libxl__detect_gfx_passthru_kind(gc,

i.e. here.

> + 
> guest_config);
> +            switch (gfx_passthru_kind) {
> +            case LIBXL_GFX_PASSTHRU_KIND_IGD:
> +                machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
> +                break;
> +            default:

With the suggestion to return KIND_DEFAULT if detection fails then I
think an extra case should be added:
        case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
                LOG(ERROR, "unable to detect required
                gfx_passthru_kind");
                return NULL;

> +                LOG(ERROR, "unknown gfx_passthru_kind\n");

I think LOG is supposed to not include the final \n.

Ian.