From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36074) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XqL8r-0003cu-6F for qemu-devel@nongnu.org; Mon, 17 Nov 2014 07:10:54 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XqL8m-00027i-35 for qemu-devel@nongnu.org; Mon, 17 Nov 2014 07:10:49 -0500 Received: from mx1.redhat.com ([209.132.183.28]:47742) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XqL8l-00027I-Rr for qemu-devel@nongnu.org; Mon, 17 Nov 2014 07:10:44 -0500 Date: Mon, 17 Nov 2014 14:10:34 +0200 From: "Michael S. Tsirkin" Message-ID: <20141117121034.GB10709@redhat.com> References: <1415172179-7965-1-git-send-email-tiejun.chen@intel.com> <1415172179-7965-2-git-send-email-tiejun.chen@intel.com> <20141105140906.GA4912@redhat.com> <546961DC.4040300@intel.com> <20141117061010.GB19718@redhat.com> <5469B660.6040107@intel.com> <20141117092501.GA20133@redhat.com> <5469C2F4.4020304@intel.com> <20141117101342.GF20133@redhat.com> <5469D979.8050404@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5469D979.8050404@intel.com> Subject: Re: [Qemu-devel] [RFC][PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Chen, Tiejun" Cc: xen-devel@lists.xensource.com, allen.m.kay@intel.com, qemu-devel@nongnu.org, aliguori@amazon.com, pbonzini@redhat.com, rth@twiddle.net On Mon, Nov 17, 2014 at 07:18:17PM +0800, Chen, Tiejun wrote: > On 2014/11/17 18:13, Michael S. Tsirkin wrote: > >On Mon, Nov 17, 2014 at 05:42:12PM +0800, Chen, Tiejun wrote: > >>On 2014/11/17 17:25, Michael S. Tsirkin wrote: > >>>On Mon, Nov 17, 2014 at 04:48:32PM +0800, Chen, Tiejun wrote: > >>>>On 2014/11/17 14:10, Michael S. Tsirkin wrote: > >>>>>On Mon, Nov 17, 2014 at 10:47:56AM +0800, Chen, Tiejun wrote: > >>>>>>On 2014/11/5 22:09, Michael S. Tsirkin wrote: > >>>>>>>On Wed, Nov 05, 2014 at 03:22:59PM +0800, Tiejun Chen wrote: > >>>>>>>>Currently IGD drivers always need to access PCH by 1f.0, and > >>>>>>>>PCH vendor/device id is used to identify the card. > >>>>>>>> > >>>>>>>>Signed-off-by: Tiejun Chen > >>>>>>>>--- > >> > >>[snip] > >> > >>>Cleaner: > >>> if (!pci_dev) { > >>> fprintf > >>> return; > >>> } > >>> pci_config_set_device_id(pci_dev->config, pch_id); > >> > >>I will address all comments and thanks. > >> > >>> > >>>>+ } > >>>>+} > >>>>+ > >>>> /* init */ > >>>> > >>>> static int xen_pt_initfn(PCIDevice *d) > >>>>@@ -682,6 +770,9 @@ static int xen_pt_initfn(PCIDevice *d) > >>>> return -1; > >>>> } > >>>> > >>>>+ /* Register ISA bridge for passthrough GFX. */ > >>>>+ xen_igd_passthrough_isa_bridge_create(s, &s->real_device); > >>>>+ > >>>> /* reinitialize each config register to be emulated */ > >>>> if (xen_pt_config_init(s)) { > >>>> XEN_PT_ERR(d, "PCI Config space initialisation failed.\n"); > >>>> > >>>>Note I will introduce a inline function in another patch, > >>>> > >>>>+static inline int is_vga_passthrough(XenHostPCIDevice *dev) > >>>>+{ > >>>>+ return (xen_has_gfx_passthru && (dev->vendor_id == PCI_VENDOR_ID_INTEL) > >>>>+ && ((dev->class_code >> 0x8) == PCI_CLASS_DISPLAY_VGA)); > >>>>+} > >>>> > >>>>Thanks > >>>>Tiejun > >>> > >>>Why bother with all these conditions? > >>>Won't it be enough to check dev->vendor_id == PCI_VENDOR_ID_INTEL? > >>> > >> > >>If this is just used for IGD, its always fine without checking vendor_id. > > > >You need to match device ID to *know* it's IGD. > > > >>So after remove that check, I guess I need to rename that as > >>is_igd_vga_passthrough() to make sense. > >> > >>Thanks > >>Tiejun > > > >There is no need to check class code or xen_has_gfx_passthru flag. > >Device ID + vendor ID identifies each device uniquely. > > > > Yeah. > > Here I assume vendor ID is always PCI_VENDOR_ID_INTEL so looks you means I > also need to check that table to do something like, > > is_igd_vga_passthugh(dev) > { > int i; > int num = ARRAY_SIZE(xen_igd_combo_id_infos); > for (i = 0; i < num; i++) { > if (dev->device_id == xen_igd_combo_id_infos[i].gpu_device_id) > return 1; > return 0; > } > > Then we can simplify the subsequent codes based on this, right? > > Thanks > Tiejun Yea. Basically let's try to treat this simply as a device quirk, and see where this gets us. -- MST