From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42288) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YM4pr-00057S-Hg for qemu-devel@nongnu.org; Thu, 12 Feb 2015 20:14:28 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YM4po-0003NN-9n for qemu-devel@nongnu.org; Thu, 12 Feb 2015 20:14:23 -0500 Received: from mga14.intel.com ([192.55.52.115]:49330) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YM4pn-0003NC-Vs for qemu-devel@nongnu.org; Thu, 12 Feb 2015 20:14:20 -0500 Message-ID: <54DD4FE6.1030201@intel.com> Date: Fri, 13 Feb 2015 09:14:14 +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> In-Reply-To: <54DAC247.6080004@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 Ian, Just ping this, or do you think I should send this as a patch? Thanks Tiejun On 2015/2/11 10:45, Chen, Tiejun wrote: > On 2015/2/9 19:05, Ian Campbell wrote: >> On Mon, 2015-02-09 at 14:28 +0800, Chen, Tiejun wrote: >> >>> What about this? >> >> I've not read the code in detail,since I'm travelling but from a quick >> glance it looks to be implementing the sort of thing I meant, thanks. > > Thanks for your time. > >> >> A couple of higher level comments: >> >> I'd suggest to put the code for reading the vid/did into a helper >> function so it can be reused. > > Looks good. > >> >> 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?). Perhaps that >> isn't needed for a first cut though and it would be a libxl API so >> thought required. > > What about 'gfx_passthru_force'? Because what we're doing is, we want to > make sure if we have a such a IGD that needs to workaround by posting a > parameter to qemu. So in case of non-listed devices we just need to > provide a bool to force this regardless of that real device. > >> >> I think it should probably log something at a lowish level when it has >> autodetected IGD. >> > > So I tried to rebase that according to your all comments, > > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c > index 98687bd..398d9da 100644 > --- a/tools/libxl/libxl_create.c > +++ b/tools/libxl/libxl_create.c > @@ -361,6 +361,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc, > libxl_defbool_setdefault(&b_info->u.hvm.nographic, false); > > libxl_defbool_setdefault(&b_info->u.hvm.gfx_passthru, false); > + libxl_defbool_setdefault(&b_info->u.hvm.gfx_passthru_force, > false); > > break; > case LIBXL_DOMAIN_TYPE_PV: > 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..07b9e22 100644 > --- a/tools/libxl/libxl_pci.c > +++ b/tools/libxl/libxl_pci.c > @@ -491,6 +491,130 @@ static int sysfs_dev_unbind(libxl__gc *gc, > libxl_device_pci *pcidev, > return 0; > } > > +static unsigned long sysfs_dev_get_vendor(libxl__gc *gc, > + libxl_device_pci *pcidev) > +{ > + char *pci_device_vendor_path = > + libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/vendor", > + pcidev->domain, pcidev->bus, pcidev->dev, > + pcidev->func); > + int read_items; > + unsigned long pci_device_vendor; > + > + 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); > + return 0xffff; > + } > + 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); > + return 0xffff; > + } > + > + return pci_device_vendor; > +} > + > +static unsigned long sysfs_dev_get_device(libxl__gc *gc, > + libxl_device_pci *pcidev) > +{ > + 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_device; > + > + FILE *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); > + return 0xffff; > + } > + 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); > + return 0xffff; > + } > + > + return pci_device_device; > +} > + > +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) > +{ > + unsigned int i, j, num = ARRAY_SIZE(igd_ids); > + libxl_ctx *ctx = libxl__gc_owner(gc); > + > + if (!libxl_defbool_val(d_config->b_info.u.hvm.gfx_passthru)) > + return 0; > + > + if (libxl_defbool_val(d_config->b_info.u.hvm.gfx_passthru_force)) { > + LIBXL__LOG(ctx, LIBXL__LOG_WARNING, "Force to workaround IGD."); > + return 1; > + } > + > + for (i = 0 ; i < d_config->num_pcidevs ; i++) { > + libxl_device_pci *pcidev = &d_config->pcidevs[i]; > + > + if (sysfs_dev_get_vendor(gc, pcidev) != 0x8086) /* Intel class */ > + continue; > + > + for (j = 0 ; j < num ; j++) { > + if (sysfs_dev_get_device(gc, pcidev) == igd_ids[j]) { /* > IGD */ > + LIBXL__LOG(ctx, LIBXL__LOG_INFO, "Detected supported > IGD."); > + return 1; > + } > + } > + } > + > + return 0; > +} > + > /* > * A brief comment about slots. I don't know what slots are for; > however, > * I have by experimentation determined: > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl > index 02be466..d3015bc 100644 > --- a/tools/libxl/libxl_types.idl > +++ b/tools/libxl/libxl_types.idl > @@ -430,6 +430,7 @@ libxl_domain_build_info = Struct("domain_build_info",[ > ("spice", libxl_spice_info), > > ("gfx_passthru", libxl_defbool), > + ("gfx_passthru_force", > libxl_defbool), > > ("serial", string), > ("boot", string), > > Thanks > Tiejun > > >