From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind Date: Tue, 17 Mar 2015 09:26:36 +0000 Message-ID: <1426584396.18247.136.camel__21463.142699986$1426584498$gmane$org@citrix.com> References: <1425980538-5508-1-git-send-email-tiejun.chen@intel.com> <1425980538-5508-3-git-send-email-tiejun.chen@intel.com> <1426073641.21353.192.camel@citrix.com> <55010596.2030106@intel.com> <1426163186.21353.409.camel@citrix.com> <55023FDA.7030204@intel.com> <1426241479.32572.94.camel@citrix.com> <55062CD2.8050305@intel.com> <1426508400.18247.33.camel@citrix.com> <5507DBD6.8030401@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5507DBD6.8030401@intel.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: "Chen, Tiejun" Cc: Ian.Jackson@eu.citrix.com, wei.liu2@citrix.com, qemu-devel@nongnu.org, stefano.stabellini@citrix.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On Tue, 2015-03-17 at 15:46 +0800, Chen, Tiejun wrote: > >>> If I remember the context correctly this is in the autodetect case, > >>> so I think shouldn't mention IGD. Something like "Unable to detect > >>> graphics passthru kind, please set gfx_passthru_kind. See xl.cfg(5) > >>> for more > >> > >> s/gfx_passthru_kind/gfx_passthru, right? Because actually we always get > >> 'gfx_passthru_kind' from 'gfx_passthru'. > > > > I think you have it backwards. > > > > In the case here gfx_passthru=1 has been set by the user, but > > gfx_passthru_kind=DEFAULT. So libxl has tried to autodetect but it has > > failed. > > > > So if the user wants to make progress they should set gfx_passthru_kind > > to whatever type of passthrough they were trying to do. > > Looks you're saying 'gfx_passthru_kind' shouldn't reply on > 'gfx_passthru', so 'gfx_passthru_kind' can override that previous value > parsed from 'gfx_passthru', right? I think perhaps the confusion arises because libxl and xl differ here. I was commenting on some libxl code, which has two fields, whereas you are talking about the xl cfg level, which only has one, which can be parsed in two ways. So I think I've probably been adding lots of confusion here, sorry. I think the _libxl_ message needs to be just "Unable to detect graphics passthru kind". i.e. it can't/shouldn't reference anything to do with xl config options etc (which would make no sense if libvirt was being used). That's not very user friendly though, so you may want to consider adding a new specific error code for this case and returning it here, such that xl or libvirt can then give a more comprehensible error message. > So looks the whole policy should be something like this, > > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > index 5c40e84..5518759 100644 > --- a/tools/libxl/xl_cmdimpl.c > +++ b/tools/libxl/xl_cmdimpl.c > @@ -1953,8 +1953,27 @@ 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)) { > + libxl_defbool_set(&b_info->u.hvm.gfx_passthru, l); > + } 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, true); > + } Up to here is fine. > + if (!xlu_cfg_get_string(config, "gfx_passthru_kind", &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_kind\"\n", > + buf); > + exit (1); > + } > + } I don't think you need this bit.