From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53393) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YIo1z-0002ko-DV for qemu-devel@nongnu.org; Tue, 03 Feb 2015 19:41:24 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YIo1w-0001QB-5h for qemu-devel@nongnu.org; Tue, 03 Feb 2015 19:41:23 -0500 Received: from mga02.intel.com ([134.134.136.20]:64562) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YIo1v-0001Q3-So for qemu-devel@nongnu.org; Tue, 03 Feb 2015 19:41:20 -0500 Message-ID: <54D16AAB.9040008@intel.com> Date: Wed, 04 Feb 2015 08:41:15 +0800 From: "Chen, Tiejun" MIME-Version: 1.0 References: <1422839843-25622-1-git-send-email-tiejun.chen@intel.com> <20150202121940.GA28773@zion.uk.xensource.com> <54D01E01.6010504@intel.com> <20150203101921.GA12437@zion.uk.xensource.com> In-Reply-To: <20150203101921.GA12437@zion.uk.xensource.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit 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: Wei Liu Cc: 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 2015/2/3 18:19, Wei Liu wrote: > 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. I have to provide this exact info after those qemu stuffs are merged, but now at lease you guys agree this option named as 'igd-passthrough', its enough to first continue working all qemu patches out. Once its finished I will go back. > >> 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. > Sure. @@ -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 = GCSPRINTF("%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]); Thanks Tiejun