From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53759) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YIaa9-0008Vw-91 for qemu-devel@nongnu.org; Tue, 03 Feb 2015 05:19:46 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YIaa5-0006IQ-Qw for qemu-devel@nongnu.org; Tue, 03 Feb 2015 05:19:45 -0500 Received: from smtp.citrix.com ([66.165.176.89]:27915) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YIaa5-0006II-KF for qemu-devel@nongnu.org; Tue, 03 Feb 2015 05:19:41 -0500 Date: Tue, 3 Feb 2015 10:19:21 +0000 From: Wei Liu Message-ID: <20150203101921.GA12437@zion.uk.xensource.com> References: <1422839843-25622-1-git-send-email-tiejun.chen@intel.com> <20150202121940.GA28773@zion.uk.xensource.com> <54D01E01.6010504@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <54D01E01.6010504@intel.com> Subject: Re: [Qemu-devel] [v2][PATCH] libxl: add one machine property to support IGD GFX passthrough List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Chen, Tiejun" Cc: Wei Liu , ian.campbell@citrix.com, Ian.Jackson@eu.citrix.com, qemu-devel@nongnu.org, xen-devel@lists.xen.org, stefano.stabellini@citrix.com, kraxel@redhat.com On Tue, Feb 03, 2015 at 09:01:53AM +0800, Chen, Tiejun wrote: > > On 2015/2/2 20:19, Wei Liu wrote: > >On Mon, Feb 02, 2015 at 09:17:23AM +0800, Tiejun Chen wrote: > >>When we're working to support IGD GFX passthrough with qemu > >>upstream, instead of "-gfx_passthru" we'd like to make that > >>a machine option, "-machine xxx,-igd-passthru=on". This need > >>to bring a change on tool side. > >> > >>Signed-off-by: Tiejun Chen > >>--- > >>v2: > >> > >>* Based on some discussions with Wei we'd like to keep both old > >> option, -gfx_passthru, and new machine property option, > >> "-machine xxx,-igd-passthru=on" at the same time but deprecate > >> the old one. Then finally we remove the old one at that point > >> that to give downstream (in this case, Xen) time to cope with the > >> change. > >> > > > >My suggestion has one premise -- if upstream QEMU has already released > >that -gfx_passthru option. If there is no "old one" (in upstream QEMU) > >at all, then there is nothing to keep and deprecate. > > Understood. > > > > >> tools/libxl/libxl_dm.c | 10 ++++++++++ > >> 1 file changed, 10 insertions(+) > >> > >>diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c > >>index c2b0487..8405f0b 100644 > >>--- a/tools/libxl/libxl_dm.c > >>+++ b/tools/libxl/libxl_dm.c > >>@@ -701,6 +701,11 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc, > > > >Note this function is upstream QEMU specfic. > > Yeah. > > > > >> flexarray_append(dm_args, "-net"); > >> flexarray_append(dm_args, "none"); > >> } > >>+ /* > >>+ * Although we already introduce 'igd-passthru', but we'd like > >>+ * to remove this until we give downstream time to cope with > >>+ * the change. > >>+ */ > >> if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) { > >> flexarray_append(dm_args, "-gfx_passthru"); > >> } > > > >The comment contradicts what I know (or what I think I know). In our > >last email exchange you said there was no "-gfx_passthru" in any version > >of upstream QEMU. > > > >So, shouldn't you just remove this `if' statement? > > Right. So what about this? > > libxl: add one machine property to support IGD GFX passthrough > > When we're working to support IGD GFX passthrough with qemu > upstream, we'd like to introduce a machine option, > "-machine xxx,igd-passthru=on", to enable/disable that feature. > And we also remove that old option, "-gfx_passthru", just from > the case of LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN since actually > no any qemu stream version really need or use that. > > Signed-off-by: Tiejun Chen > Yes. I think a patch like this reflects the reality. It would be nice, as Ian J suggested, to state which version of QEMU upstream introduces that new option in commit message. > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c > index c2b0487..b888f19 100644 > --- a/tools/libxl/libxl_dm.c > +++ b/tools/libxl/libxl_dm.c > @@ -701,9 +701,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"); > @@ -748,6 +745,11 @@ 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)) { > + machinearg = libxl__sprintf(gc, "%s,igd-passthru=on", > machinearg); > + } > + Please use GCSPRINTF macro. Wei. > 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 > > > > >Wei. > > > >>@@ -748,6 +753,11 @@ 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)) { > >>+ machinearg = libxl__sprintf(gc, "%s,igd-passthru=on", machinearg); > >>+ } > >>+ > >> 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]); > >>-- > >>1.9.1 > >