From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38946) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YSF1z-00042D-Vl for qemu-devel@nongnu.org; Sun, 01 Mar 2015 20:20:25 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YSF1t-00052m-Gz for qemu-devel@nongnu.org; Sun, 01 Mar 2015 20:20:23 -0500 Received: from mga11.intel.com ([192.55.52.93]:44771) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YSF1t-000522-7o for qemu-devel@nongnu.org; Sun, 01 Mar 2015 20:20:17 -0500 Message-ID: <54F3BACB.4070400@intel.com> Date: Mon, 02 Mar 2015 09:20:11 +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> In-Reply-To: <1425035066.14641.167.camel@citrix.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 >> 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 > > >