[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind
From: |
Ian Campbell |
Subject: |
Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind |
Date: |
Thu, 12 Mar 2015 12:26:26 +0000 |
On Thu, 2015-03-12 at 11:18 +0800, Chen, Tiejun wrote:
> >> +
> >> + 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");
> >> + }
> >
> > I think this whole block should be inside an:
> > if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)
> > the semantics should be that kind is ignored unless passthru is enabled.
> >
> > and then the if/else chain could become a switch perhaps?
>
> Okay.
>
> >
> > I'm unsure what we should do if kind==DEFAULT but
> > libxl__is_igd_vga_passthru fails. At a minimum a warning seems
> > desirable. I'm not sure if it should warrant a failure or not.
>
> I think a warning message plus abort() should be enough, and then user
> can determine what's next,
In libxl an abort is only warranted if something _cannot_ happen and is
not under user control. I think think that applies here unless some
earlier code is guaranteed to have validated the value before it reaches
this point.
> Please take a look at this,
>
> +
> + if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
> + switch (b_info->u.hvm.gfx_passthru_kind) {
> + case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
> + if (libxl__is_igd_vga_passthru(gc, guest_config)) {
> + machinearg = GCSPRINTF("%s,igd-passthru=on",
> machinearg);
> + } else {
> + LOG(WARN, "No supported IGD to passthru,"
> + " or please force set gfx_passthru=\"igd\".\n");
> + abort();
I don't think you can abort here, since a user can set
b_info->u.hvm.gfx_passthru_kind to default. You would need to return an
error.
> @@ -720,6 +720,13 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst,
> libxl_mac *src);
> #define LIBXL_HAVE_PSR_MBM 1
> #endif
>
> +/*
> + * LIBXL_HAVE_GFX_PASSTHRU_KIND
> + *
> + * If this is defined, the Graphic Device Passthrough Override is
> supported.
Almost, please also explicitly name the type field as other similar
comments do for clarity.
> + */
> +#define LIBXL_HAVE_GFX_PASSTHRU_KIND 1
> +
> typedef char **libxl_string_list;
> void libxl_string_list_dispose(libxl_string_list *sl);
> int libxl_string_list_length(const libxl_string_list *sl);
> @@ -1501,6 +1508,11 @@ int libxl_psr_cmt_get_sample(libxl_ctx *ctx,
> uint64_t *tsc_r);
> #endif
>
> +#ifdef LIBXL_HAVE_GFX_PASSTHRU_KIND
No ifdef needed here (or in libxl_internal.h)
> +bool libxl__is_igd_vga_passthru(libxl__gc *gc,
> + const libxl_domain_config *d_config);
and this should be in libxl_internal.h not here...
> But looks libxl__gc{} is defined in the libxl_internal.h file... I guess
> we need a conversion, or other idea?
... which answers this question.
> Here I update this chunk of code based on your two comments:
>
> @@ -1953,8 +1953,22 @@ 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, true);
> + } else {
> + libxl_defbool_set(&b_info->u.hvm.gfx_passthru, false);
> + }
This is exactly the same as:
libxl_defbool_set(&b_info->u.hvm.gfx_passthru, l);
Ian.
- [Qemu-devel] [v2][PATCH 0/2] libxl: try to support IGD passthrough for qemu upstream, Tiejun Chen, 2015/03/10
- [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Tiejun Chen, 2015/03/10
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Ian Campbell, 2015/03/11
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Chen, Tiejun, 2015/03/11
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind,
Ian Campbell <=
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Chen, Tiejun, 2015/03/12
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Ian Campbell, 2015/03/13
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Chen, Tiejun, 2015/03/15
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Ian Campbell, 2015/03/16
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Chen, Tiejun, 2015/03/17
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Ian Campbell, 2015/03/17
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Chen, Tiejun, 2015/03/18
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Ian Campbell, 2015/03/18
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Chen, Tiejun, 2015/03/18
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Ian Campbell, 2015/03/19