From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33498) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YSjix-00015e-NE for qemu-devel@nongnu.org; Tue, 03 Mar 2015 05:06:48 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YSjiu-0001Y2-Bj for qemu-devel@nongnu.org; Tue, 03 Mar 2015 05:06:47 -0500 Received: from mga02.intel.com ([134.134.136.20]:19266) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YSjiu-0001XU-0o for qemu-devel@nongnu.org; Tue, 03 Mar 2015 05:06:44 -0500 Message-ID: <54F587AE.9010607@intel.com> Date: Tue, 03 Mar 2015 18:06:38 +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> <21711.29549.892862.333392@mariner.uk.xensource.com> <54D01EAB.1020005@intel.com> <1422961628.9323.32.camel@citrix.com> <54D1770C.4020904@intel.com> <1423046493.17711.28.camel@citrix.com> <54D2C5E5.40407@intel.com> <1423129922.24924.46.camel@citrix.com> <54D41274.9090400@intel.com> <54D8537C.2080805@intel.com> <1423479910.23098.34.camel@citrix.com> <54DAC247.6080004@intel.com> <1424265750.27775.51.camel@citrix.com> <54EEBEBE.10108@intel.com> <1424967433.14641.96.camel@citrix.com> <54F00E9C.5@intel.com> <1425035066.14641.167.camel@citrix.com> <54F3BACB.4070400@intel.com> In-Reply-To: <54F3BACB.4070400@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [Xen-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: Ian Campbell Cc: Wei Liu , Ian Jackson , qemu-devel@nongnu.org, xen-devel@lists.xen.org, stefano.stabellini@citrix.com, kraxel@redhat.com Campbell, Are you free to look at my reply? Thanks Tiejun On 2015/3/2 9:20, Chen, Tiejun wrote: >>> Here I just mean when Qemu realizes IGD is passed through but without >>> that appropriate option set, Qemu can post something to explicitly >>> notify user that this option is needed in his case. But it may be a lazy >>> idea. >> >> In any case I think the additions of such warnings in qemu are a >> separate to the discussion in this thread, so I propose to leave it >> alone for now. > > Okay. > >> >>> So now I think I'd better go back handling this on Xen side with your >>> comments. As you said the Boolean doesn't suffice to indicate that IGD >>> workarounds are needed. So I think we can reuse that existing bool >>> 'gfx_passthru'. >>> >>> Firstly we can redefine this as string, >> >> Unfortunately not since libxl's API guarantee requires older clients to >> keep working, i.e. those who use libxl_defbool_set on this field. >> >> Probably the best which can be done is to deprecate this field in favour >> of a new one (the old field would need to be obeyed only if the new one >> was set to its default value). >> >> Probably an Enumeration would be better than a raw string here as well. >> >> This approach doesn't allow for the possibility of multiple such >> workarounds though. It's unclear to me if this matters or not. >> >> The other option which I've mentioned is to leave gfx_passthru and have >> libxl figure out which workarounds to enable based on the set of PCI >> devices passed through. I guess you don't like that approach? (due to >> the need to maintain the pci vid:did list?) > > No, I like that approach currently :) Please see the below, > >> >>> >>> - ("gfx_passthru", >>> libxl_defbool), >>> + ("gfx_passthru", string), >>> >>> Then >>> >>> + >>> + if (libxl__is_igd_vga_passthru(gc, guest_config) || > > This is just working out that way that I already posted previously, and > here I paste this code fragment again, > > +static const pci_info fixup_ids[] = { > + /* Intel HSW Classic */ > + {0x8086, 0x0402}, /* HSWGT1D, HSWD_w7 */ > + {0x8086, 0x0406}, /* HSWGT1M, HSWM_w7 */ > + {0x8086, 0x0412}, /* HSWGT2D, HSWD_w7 */ > + {0x8086, 0x0416}, /* HSWGT2M, HSWM_w7 */ > + {0x8086, 0x041E}, /* HSWGT15D, HSWD_w7 */ > + /* Intel HSW ULT */ > + {0x8086, 0x0A06}, /* HSWGT1UT, HSWM_w7 */ > + {0x8086, 0x0A16}, /* HSWGT2UT, HSWM_w7 */ > + {0x8086, 0x0A26}, /* HSWGT3UT, HSWM_w7 */ > + {0x8086, 0x0A2E}, /* HSWGT3UT28W, HSWM_w7 */ > + {0x8086, 0x0A1E}, /* HSWGT2UX, HSWM_w7 */ > + {0x8086, 0x0A0E}, /* HSWGT1ULX, HSWM_w7 */ > + /* Intel HSW CRW */ > + {0x8086, 0x0D26}, /* HSWGT3CW, HSWM_w7 */ > + {0x8086, 0x0D22}, /* HSWGT3CWDT, HSWD_w7 */ > + /* Intel HSW Server */ > + {0x8086, 0x041A}, /* HSWSVGT2, HSWD_w7 */ > + /* Intel HSW SRVR */ > + {0x8086, 0x040A}, /* HSWSVGT1, HSWD_w7 */ > + /* Intel BSW */ > + {0x8086, 0x1606}, /* BDWULTGT1, BDWM_w7 */ > + {0x8086, 0x1616}, /* BDWULTGT2, BDWM_w7 */ > + {0x8086, 0x1626}, /* BDWULTGT3, BDWM_w7 */ > + {0x8086, 0x160E}, /* BDWULXGT1, BDWM_w7 */ > + {0x8086, 0x161E}, /* BDWULXGT2, BDWM_w7 */ > + {0x8086, 0x1602}, /* BDWHALOGT1, BDWM_w7 */ > + {0x8086, 0x1612}, /* BDWHALOGT2, BDWM_w7 */ > + {0x8086, 0x1622}, /* BDWHALOGT3, BDWM_w7 */ > + {0x8086, 0x162B}, /* BDWHALO28W, BDWM_w7 */ > + {0x8086, 0x162A}, /* BDWGT3WRKS, BDWM_w7 */ > + {0x8086, 0x162D}, /* BDWGT3SRVR, BDWM_w7 */ > +}; > + > +/* > + * Some devices may need some ways to work well. Here like IGD, > + * we have to pass a specific option to qemu. > + */ > +int libxl__is_igd_vga_passthru(libxl__gc *gc, > + const libxl_domain_config *d_config) > +{ > + unsigned int i, j, num = ARRAY_SIZE(fixup_ids); > + uint16_t vendor, device; > + > + for (i = 0 ; i < d_config->num_pcidevs ; i++) { > + libxl_device_pci *pcidev = &d_config->pcidevs[i]; > + > + for (j = 0 ; j < num ; j++) { > + vendor = fixup_ids[j].vendor; > + device = fixup_ids[j].device; > + > + if (sysfs_dev_get_vendor(gc, pcidev) == vendor && > + sysfs_dev_get_device(gc, pcidev) == device) > + return 1; > + } > + } > + > + return 0; > +} > + > > Is this expected? > >>> + (b_info->u.hvm.gfx_passthru && >>> + strncmp(b_info->u.hvm.gfx_passthru, "igd", 3) == 0) ) { > > But as you mentioned previously, > > " > You might like to optionally consider add a forcing option somehow so > that people with new devices not in the list can control things without > the need to recompile (e.g. gfx_passthru_kind_override?). > " > > Here I was trying to convert "gfx_passthru" to address this thing. > According to your comment right now, you prefer we should introduce a > new field instead of the original 'gfx_passthru' to enumerate such a > type. So what about this? > > libxl_gfx_passthru_kind_type = Enumeration("gfx_passthru_kind_type", [ > (0, "unknown"), > (1, "igd"), > ]) > > Then if we want to override this, just submit the following line into .cfg: > > gfx_passthru_kind_override = "igd" > > Thanks > Tiejun > >>> + machinearg = GCSPRINTF("%s,igd-passthru=on", >>> machinearg); >>> + } >>> + >>> >>> Of course we need modify something else to align this change. >>> >>> Thanks >>> Tiejun >> >> >> > > >