From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36905) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YKhpY-0003tE-35 for qemu-devel@nongnu.org; Mon, 09 Feb 2015 01:28:25 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YKhpT-0002nP-2d for qemu-devel@nongnu.org; Mon, 09 Feb 2015 01:28:24 -0500 Received: from mga01.intel.com ([192.55.52.88]:32880) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YKhpS-0002nF-P8 for qemu-devel@nongnu.org; Mon, 09 Feb 2015 01:28:19 -0500 Message-ID: <54D8537C.2080805@intel.com> Date: Mon, 09 Feb 2015 14:28:12 +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> In-Reply-To: <54D41274.9090400@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 On 2015/2/6 9:01, Chen, Tiejun wrote: > On 2015/2/5 17:52, Ian Campbell wrote: >> On Thu, 2015-02-05 at 09:22 +0800, Chen, Tiejun wrote: >>> Indeed this is not something workaround, and I think in any type of VGA >>> devices, we'd like to diminish this sort of thing gradually, right? >>> >>> This mightn't come true in real world :) >> >> It's not really something we can control, the h/w guys will do what they >> do, including integrating on-board graphics tightly with the N/S-bridges >> etc. > > Yeah. > >> >>>> >>>> I think there are three ways to achieve that: >>>> >>>> * Make the libxl/xl option something which is not generic e.g. >>>> igd_passthru=1 >>>> * Add a second option to allow the user to configure the >>>> kind of >>>> graphics device being passed thru (e.g. gfx_passthru=1, >>>> passthru_device="igd"), or combine the two by making the >>>> gfx_passthru option a string instead of a boolean. >>> >>> It makes more sense but this mean we're going to change that existing >>> rule in qemu-traditional. But here I guess we shouldn't consider that >>> case. >> >> qemu-trad is frozen so we wouldn't be adding new features such as >> support for new graphics passthru devices, so we can ignore it's >> deficiencies in this area and just improve things in the qemu-xen case. >> (we may want to add a compat handling for any new option we add to libxl >> to translate to -gfx_passthru, but that's about it) > > Understood. > >> >>>> * Make libxl detect the type of graphics device somehow and >>>> therefore automatically determine when gfx_passthru=1 => >>>> igd-passthru >>> >>> This way confounds me all. Can libxl detect the graphics device *before* >>> we intend to pass a parameter to active qemu? >> >> We know the BDF of all devices which we are going to pass to the guest >> and we can observe various properties of that device >> via /sys/bus/pci/devices/0000:B:D:F. > > So sounds what you're saying is Xen already have this sort of example in > libxl. > >> >>> >>>> >>>>> Currently, we have to set >>>>> something as follows, >>>>> >>>>> gfx_passthru=1 >>>>> pci=["00:02.0"] >>>>> >>>>> This always works for qemu-xen-traditional. >>>>> >>>>> But you should know '00:02.0' doesn't mean we are passing IGD through. >>>> >>>> But by looking at the device 00:02.0 (e.g. its PCI vendor and device ID >>>> and other properties) we can unambiguously determine if it is an IGD >>>> device or not, can't we? >>> >>> Again, like what I said above, I'm not sure if its possible in my case. >>> If I'm wrong please correct me. >> >> Is my reply above sufficient? > > Yes, I can understand what you mean but I need to take close look at > exactly what should be done in libxl :) > > In high level, this way may come out as follows, > > #1 libxl parse 'pci=[]' to get SBDF > #2 Scan SBDF by accessing sysfs to get vendor/device IDs. > #3 If this pair of IDs is identified to our target device, IGD, > "igd-passthru" would be delivered to qemu. > > Right? What about this? diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c index 8599a6a..507034f 100644 --- a/tools/libxl/libxl_dm.c +++ b/tools/libxl/libxl_dm.c @@ -710,9 +710,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"); @@ -757,6 +754,11 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc, machinearg, max_ram_below_4g); } } + + if (libxl__is_igd_vga_passthru(gc, guest_config)) { + 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]); diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h index 934465a..35ec5fc 100644 --- a/tools/libxl/libxl_internal.h +++ b/tools/libxl/libxl_internal.h @@ -1177,6 +1177,9 @@ _hidden int libxl__create_pci_backend(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev, int num); _hidden int libxl__device_pci_destroy_all(libxl__gc *gc, uint32_t domid); +_hidden int libxl__is_igd_vga_passthru(libxl__gc *gc, + const libxl_domain_config *d_config); + /*----- xswait: wait for a xenstore node to be suitable -----*/ typedef struct libxl__xswait_state libxl__xswait_state; diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c index f3ae132..9cccbfe 100644 --- a/tools/libxl/libxl_pci.c +++ b/tools/libxl/libxl_pci.c @@ -491,6 +491,108 @@ static int sysfs_dev_unbind(libxl__gc *gc, libxl_device_pci *pcidev, return 0; } +/* Currently we only support HSW and BDW in qemu upstream.*/ +static const uint16_t igd_ids[] = { + /* HSW Classic */ + 0x0402, /* HSWGT1D, HSWD_w7 */ + 0x0406, /* HSWGT1M, HSWM_w7 */ + 0x0412, /* HSWGT2D, HSWD_w7 */ + 0x0416, /* HSWGT2M, HSWM_w7 */ + 0x041E, /* HSWGT15D, HSWD_w7 */ + /* HSW ULT */ + 0x0A06, /* HSWGT1UT, HSWM_w7 */ + 0x0A16, /* HSWGT2UT, HSWM_w7 */ + 0x0A26, /* HSWGT3UT, HSWM_w7 */ + 0x0A2E, /* HSWGT3UT28W, HSWM_w7 */ + 0x0A1E, /* HSWGT2UX, HSWM_w7 */ + 0x0A0E, /* HSWGT1ULX, HSWM_w7 */ + /* HSW CRW */ + 0x0D26, /* HSWGT3CW, HSWM_w7 */ + 0x0D22, /* HSWGT3CWDT, HSWD_w7 */ + /* HSW Sever */ + 0x041A, /* HSWSVGT2, HSWD_w7 */ + /* HSW SRVR */ + 0x040A, /* HSWSVGT1, HSWD_w7 */ + /* BSW */ + 0x1606, /* BDWULTGT1, BDWM_w7 */ + 0x1616, /* BDWULTGT2, BDWM_w7 */ + 0x1626, /* BDWULTGT3, BDWM_w7 */ + 0x160E, /* BDWULXGT1, BDWM_w7 */ + 0x161E, /* BDWULXGT2, BDWM_w7 */ + 0x1602, /* BDWHALOGT1, BDWM_w7 */ + 0x1612, /* BDWHALOGT2, BDWM_w7 */ + 0x1622, /* BDWHALOGT3, BDWM_w7 */ + 0x162B, /* BDWHALO28W, BDWM_w7 */ + 0x162A, /* BDWGT3WRKS, BDWM_w7 */ + 0x162D, /* BDWGT3SRVR, BDWM_w7 */ +}; + +int libxl__is_igd_vga_passthru(libxl__gc *gc, + const libxl_domain_config *d_config) +{ + int i, j, ret = 0; + + if (!libxl_defbool_val(d_config->b_info.u.hvm.gfx_passthru)) + return ret; + + for (i = 0 ; i < d_config->num_pcidevs ; i++) { + libxl_device_pci *pcidev = &d_config->pcidevs[i]; + char *pci_device_vendor_path = + libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/vendor", + pcidev->domain, pcidev->bus, pcidev->dev, + pcidev->func); + char *pci_device_device_path = + libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/device", + pcidev->domain, pcidev->bus, pcidev->dev, + pcidev->func); + int read_items; + unsigned long pci_device_vendor, pci_device_device; + unsigned int num = ARRAY_SIZE(igd_ids); + + FILE *f = fopen(pci_device_vendor_path, "r"); + if (!f) { + LOGE(ERROR, + "pci device "PCI_BDF" does not have vendor attribute", + pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func); + continue; + } + read_items = fscanf(f, "0x%lx\n", &pci_device_vendor); + fclose(f); + if (read_items != 1) { + LOGE(ERROR, + "cannot read vendor of pci device "PCI_BDF, + pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func); + continue; + } + if (pci_device_vendor != 0x8086) /* Intel class */ + continue; + + f = fopen(pci_device_device_path, "r"); + if (!f) { + LOGE(ERROR, + "pci device "PCI_BDF" does not have device attribute", + pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func); + continue; + } + read_items = fscanf(f, "0x%lx\n", &pci_device_device); + fclose(f); + if (read_items != 1) { + LOGE(ERROR, + "cannot read device of pci device "PCI_BDF, + pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func); + continue; + } + for (j = 0 ; j < num ; j++) { + if (pci_device_device == igd_ids[j]) { /* IGD class */ + ret = 1; + break; + } + } + } + + return ret; +} + /* * A brief comment about slots. I don't know what slots are for; however, * I have by experimentation determined: Thanks Tiejun