[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind
On 2015/3/19 18:44, Ian Campbell wrote:
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.
Refactor again,
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 8599a6a..05c8916 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 enum libxl_gfx_passthru_kind
+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 LIBXL_GFX_PASSTHRU_KIND_DEFAULT;
+}
+
static char ** libxl__build_device_model_args_new(libxl__gc *gc,
const char *dm, int guest_domid,
const libxl_domain_config
*guest_config,
@@ -757,6 +771,21 @@ 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)) {
+ enum libxl_gfx_passthru_kind 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;
+ case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
+ LOG(ERROR, "unable to detect required gfx_passthru_kind");
+ default:
+ 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]);
Thanks
Tiejun
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
