From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58795) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YUrAR-0001DP-4A for qemu-devel@nongnu.org; Mon, 09 Mar 2015 02:27:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YUrAN-0006g2-2I for qemu-devel@nongnu.org; Mon, 09 Mar 2015 02:27:55 -0400 Received: from mga09.intel.com ([134.134.136.24]:64781) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YUrAM-0006fn-Oe for qemu-devel@nongnu.org; Mon, 09 Mar 2015 02:27:50 -0400 Message-ID: <54FD3D62.6080707@intel.com> Date: Mon, 09 Mar 2015 14:27:46 +0800 From: "Chen, Tiejun" MIME-Version: 1.0 References: <1425632903-5502-1-git-send-email-tiejun.chen@intel.com> <1425632903-5502-2-git-send-email-tiejun.chen@intel.com> <20150306124003.GO12103@zion.uk.xensource.com> In-Reply-To: <20150306124003.GO12103@zion.uk.xensource.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/2] libxl: introduce libxl__is_igd_vga_passthru List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wei Liu Cc: qemu-devel@nongnu.org, stefano.stabellini@citrix.com, Ian.Jackson@eu.citrix.com, ian.campbell@citrix.com, xen-devel@lists.xen.org On 2015/3/6 20:40, Wei Liu wrote: > On Fri, Mar 06, 2015 at 05:08:22PM +0800, Tiejun Chen wrote: >> While working with qemu, IGD is a specific device in the case of pass through >> so we need to identify that to handle more later. Here we define a table to >> record all IGD types currently we can support. Also we need to introduce two >> helper functions to get vendor and device ids to lookup that table. >> >> Signed-off-by: Tiejun Chen >> --- >> tools/libxl/libxl_internal.h | 2 + >> tools/libxl/libxl_pci.c | 124 +++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 126 insertions(+) >> >> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h >> index 934465a..8b952b8 100644 >> --- a/tools/libxl/libxl_internal.h >> +++ b/tools/libxl/libxl_internal.h >> @@ -1176,6 +1176,8 @@ _hidden int libxl__device_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pc >> _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 -----*/ >> >> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c >> index f3ae132..dc5a89e 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) > > uint16_t? > >> +{ >> + char *pci_device_vendor_path = >> + libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/vendor", >> + pcidev->domain, pcidev->bus, pcidev->dev, >> + pcidev->func); > > Please use GCSPRINTF macro. Okay. > >> + int read_items; >> + unsigned long pci_device_vendor; > > uint16_t? Yes, I can but I don't see other similar helpers are doing this in this file :) > > Same comments apply to _get_device function. And especially, if we really set that as uint16_t, > >> + >> + 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); we have to refactor this as well, read_items = fscanf(f, "0x%hx\n", &pci_device_vendor); Right? >> + 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; >> +} >> + > > [...] > >> +/* >> + * 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, > > bool. Okay. > >> + 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; > > Get vendor and device in outer loop to avoid wasting cpu cycles. :-) > Yeah. Thanks Tiejun