From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40195) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X0SHY-0007Hk-Oz for qemu-devel@nongnu.org; Fri, 27 Jun 2014 05:17:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X0SHS-0006sK-JH for qemu-devel@nongnu.org; Fri, 27 Jun 2014 05:17:20 -0400 Received: from mga09.intel.com ([134.134.136.24]:46851) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X0SHR-0006sC-WD for qemu-devel@nongnu.org; Fri, 27 Jun 2014 05:17:14 -0400 Message-ID: <53AD366A.1040206@intel.com> Date: Fri, 27 Jun 2014 17:16:26 +0800 From: "Chen, Tiejun" MIME-Version: 1.0 References: <1403662641-28526-1-git-send-email-tiejun.chen@intel.com> <1403662641-28526-4-git-send-email-tiejun.chen@intel.com> <20140625070439.GC25563@redhat.com> In-Reply-To: <20140625070439.GC25563@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [v5][PATCH 3/5] xen, gfx passthrough: support Intel IGD passthrough with VT-D List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: peter.maydell@linaro.org, xen-devel@lists.xensource.com, stefano.stabellini@eu.citrix.com, allen.m.kay@intel.com, qemu-devel@nongnu.org, Kelly.Zytaruk@amd.com, yang.z.zhang@intel.com, anthony@codemonkey.ws, anthony.perard@citrix.com, pbonzini@redhat.com On 2014/6/25 15:04, Michael S. Tsirkin wrote: > On Wed, Jun 25, 2014 at 10:17:19AM +0800, Tiejun Chen wrote: >> Some registers of Intel IGD are mapped in host bridge, so it needs to [snip] >> static int is_vga_passthrough(XenHostPCIDevice *dev) >> { >> @@ -291,3 +292,158 @@ static int create_pseudo_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice *hdev) >> XEN_PT_LOG(dev, "The pseudo Intel PCH ISA bridge created.\n"); >> return 0; >> } >> + >> +int pci_create_pch(PCIBus *bus) > > > Please prefix all xen specific non static functions > with xen_ or something like this. Okay. > pci_ is for pci core. > > In fact it's a good idea to do this for static functions > as well, in case we add a conflicting function in > some header. > >> +{ >> + XenHostPCIDevice hdev; >> + int r = 0; >> + >> + if (!xen_has_gfx_passthru) { >> + return r; >> + } >> + >> + r = xen_host_pci_device_get(&hdev, 0, 0, 0x1f, 0); >> + if (r) { >> + XEN_PT_ERR(NULL, "Failed to find Intel PCH on host\n"); >> + goto err; >> + } >> + >> + if (hdev.vendor_id == PCI_VENDOR_ID_INTEL) { >> + r = create_pseudo_pch_isa_bridge(bus, &hdev); >> + if (r) { >> + XEN_PT_ERR(NULL, "Failed to create PCH ISA bridge.\n"); >> + goto err; >> + } >> + } > > Does it work on non intel? IGD means this should work on Intel platform. > It seems to return success. Okay, I'd like to change this a void. > Maybe you should just verify that vendor and device > ID have the expected values on the host, and Vendor id is enough. > fail otherwise. > >> + >> + xen_host_pci_device_put(&hdev); >> + >> +err: >> + return r; >> +} >> + >> +/* >> + * Currently we just pass this physical host bridge for IGD, 00:02.0. >> + * >> + * Here pci_dev is just that host bridge, so we have to get that real >> + * passthrough device by that given devfn to further confirm. >> + */ > > > confirm what? So change like: * passthrough device by that given devfn to avoid other devices access. > Comments like this need to document what function does. > > Maybe > > /* Can we support IGD passthrough for this device? > * We require ... > */ > >> +static int is_igd_passthrough(PCIDevice *pci_dev) >> +{ >> + PCIDevice *f = pci_dev->bus->devices[PCI_DEVFN(2, 0)]; >> + if (pci_dev->bus->devices[PCI_DEVFN(2, 0)]) { >> + XenPCIPassthroughState *s = DO_UPCAST(XenPCIPassthroughState, dev, f); >> + return (is_vga_passthrough(&s->real_device) >> + && (s->real_device.vendor_id == PCI_VENDOR_ID_INTEL)); >> + } else { >> + return 0; >> + } >> +} >> + >> +void igd_pci_write(PCIDevice *pci_dev, uint32_t config_addr, >> + uint32_t val, int len) > > Same here, xen_ everywhere please. Okay. > >> +{ >> + XenHostPCIDevice dev; >> + int r; >> + >> + /* IGD read/write is through the host bridge. >> + * ISA bridge is only for detect purpose. In i915 driver it will >> + * probe ISA bridge to discover the IGD, see comment in i915_drv.c: >> + * intel_detect_pch(). > > You mean in linux kernel I guess? So change like, * probe ISA bridge to discover the IGD, see comment in Linux:i915_drv.c: > >> + */ >> + >> + assert(pci_dev->devfn == 0x00); >> + >> + if (!is_igd_passthrough(pci_dev)) { >> + goto write_default; >> + } >> + >> + /* Just work for the i915 driver. */ >> + switch (config_addr) { >> + case 0x58: /* PAVPC Offset */ >> + break; >> + default: >> + /* Just sets the emulated values. */ >> + goto write_default; >> + } >> + >> + /* Host write */ >> + r = xen_host_pci_device_get(&dev, 0, 0, 0, 0); >> + if (r) { >> + XEN_PT_ERR(pci_dev, "Can't get pci_dev_host_bridge\n"); >> + abort(); >> + } >> + >> + r = xen_host_pci_set_block(&dev, config_addr, (uint8_t *)&val, len); >> + if (r) { >> + XEN_PT_ERR(pci_dev, "Can't get pci_dev_host_bridge\n"); >> + abort(); >> + } > > > Cleaner: > > if (config_addr == 0x58) { Maybe we add other offset in the future, so we'd better keep in them in switch(). > /* Host write */ > r = xen_host_pci_device_get(&dev, 0, 0, 0, 0); > if (r) { > XEN_PT_ERR(pci_dev, "Can't get pci_dev_host_bridge\n"); > abort(); > } > > r = xen_host_pci_set_block(&dev, config_addr, (uint8_t *)&val, len); > if (r) { > XEN_PT_ERR(pci_dev, "Can't get pci_dev_host_bridge\n"); > abort(); > } > } > > Note this does not work on e.g. BE. Why do we need take BE into consideration here? Shouldn't PCI already be LE? > The best way is really to make the register writeable in wmask. > Then > pci_default_write_config(pci_dev, config_addr, val, len); > if (range_covers_byte(addr, len, 0x58)) { > .... > > r = xen_host_pci_set_block(&dev, config_addr, > pci_dev->config + config_addr, len); > } > > > > > >> + >> + xen_host_pci_device_put(&dev); >> + >> + return; >> + >> +write_default: >> + pci_default_write_config(pci_dev, config_addr, val, len); >> +} >> + >> +uint32_t igd_pci_read(PCIDevice *pci_dev, uint32_t config_addr, int len) >> +{ >> + XenHostPCIDevice dev; >> + uint32_t val; >> + int r; >> + >> + /* IGD read/write is through the host bridge. >> + * ISA bridge is only for detect purpose. In i915 driver it will >> + * probe ISA bridge to discover the IGD, see comment in i915_drv.c: >> + * intel_detect_pch(). >> + */ >> + assert(pci_dev->devfn == 0x00); >> + >> + if (!is_igd_passthrough(pci_dev)) { >> + goto read_default; >> + } >> + >> + /* Just work for the i915 driver. */ >> + switch (config_addr) { >> + case 0x08: /* revision id */ >> + case 0x2c: /* sybsystem vendor id */ >> + case 0x2e: /* sybsystem id */ > > Since you set them to match host previously, > should be no need to override now. Just *read* the host bridge, no *write* here. > >> + case 0x44: /* MCHBAR I915 */ >> + case 0x48: /* MCHBAR I965 */ >> + case 0x50: /* SNB: processor graphics control register */ >> + case 0x52: /* processor graphics control register */ >> + case 0xa0: /* top of memory */ >> + case 0xb0: /* ILK: BSM: should read from dev 2 offset 0x5c */ >> + case 0x58: /* SNB: PAVPC Offset */ >> + case 0xa4: /* SNB: graphics base of stolen memory */ >> + case 0xa8: /* SNB: base of GTT stolen memory */ > > Move the actual code here. > Sorry, whats the actual code? Are you saying we can read all values from the host bridge in advance then restore them here? Thanks Tiejun >> + break; >> + default: >> + /* Just gets the emulated values. */ > > and here. > >> + goto read_default; >> + } >> + >> + /* Host read */ >> + r = xen_host_pci_device_get(&dev, 0, 0, 0, 0); >> + if (r) { >> + goto err_out; >> + } >> + >> + r = xen_host_pci_get_block(&dev, config_addr, (uint8_t *)&val, len); >> + if (r) { >> + goto err_out; >> + } >> + >> + xen_host_pci_device_put(&dev); >> + >> + return val; >> + >> +read_default: >> + return pci_default_read_config(pci_dev, config_addr, len); >> + >> +err_out: >> + XEN_PT_ERR(pci_dev, "Can't get pci_dev_host_bridge\n"); >> + return -1; >> +} >> -- >> 1.9.1 > > From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [v5][PATCH 3/5] xen, gfx passthrough: support Intel IGD passthrough with VT-D Date: Fri, 27 Jun 2014 17:16:26 +0800 Message-ID: <53AD366A.1040206@intel.com> References: <1403662641-28526-1-git-send-email-tiejun.chen@intel.com> <1403662641-28526-4-git-send-email-tiejun.chen@intel.com> <20140625070439.GC25563@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20140625070439.GC25563@redhat.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org Sender: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org To: "Michael S. Tsirkin" Cc: peter.maydell@linaro.org, xen-devel@lists.xensource.com, stefano.stabellini@eu.citrix.com, allen.m.kay@intel.com, qemu-devel@nongnu.org, Kelly.Zytaruk@amd.com, yang.z.zhang@intel.com, anthony@codemonkey.ws, anthony.perard@citrix.com, pbonzini@redhat.com List-Id: xen-devel@lists.xenproject.org On 2014/6/25 15:04, Michael S. Tsirkin wrote: > On Wed, Jun 25, 2014 at 10:17:19AM +0800, Tiejun Chen wrote: >> Some registers of Intel IGD are mapped in host bridge, so it needs to [snip] >> static int is_vga_passthrough(XenHostPCIDevice *dev) >> { >> @@ -291,3 +292,158 @@ static int create_pseudo_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice *hdev) >> XEN_PT_LOG(dev, "The pseudo Intel PCH ISA bridge created.\n"); >> return 0; >> } >> + >> +int pci_create_pch(PCIBus *bus) > > > Please prefix all xen specific non static functions > with xen_ or something like this. Okay. > pci_ is for pci core. > > In fact it's a good idea to do this for static functions > as well, in case we add a conflicting function in > some header. > >> +{ >> + XenHostPCIDevice hdev; >> + int r = 0; >> + >> + if (!xen_has_gfx_passthru) { >> + return r; >> + } >> + >> + r = xen_host_pci_device_get(&hdev, 0, 0, 0x1f, 0); >> + if (r) { >> + XEN_PT_ERR(NULL, "Failed to find Intel PCH on host\n"); >> + goto err; >> + } >> + >> + if (hdev.vendor_id == PCI_VENDOR_ID_INTEL) { >> + r = create_pseudo_pch_isa_bridge(bus, &hdev); >> + if (r) { >> + XEN_PT_ERR(NULL, "Failed to create PCH ISA bridge.\n"); >> + goto err; >> + } >> + } > > Does it work on non intel? IGD means this should work on Intel platform. > It seems to return success. Okay, I'd like to change this a void. > Maybe you should just verify that vendor and device > ID have the expected values on the host, and Vendor id is enough. > fail otherwise. > >> + >> + xen_host_pci_device_put(&hdev); >> + >> +err: >> + return r; >> +} >> + >> +/* >> + * Currently we just pass this physical host bridge for IGD, 00:02.0. >> + * >> + * Here pci_dev is just that host bridge, so we have to get that real >> + * passthrough device by that given devfn to further confirm. >> + */ > > > confirm what? So change like: * passthrough device by that given devfn to avoid other devices access. > Comments like this need to document what function does. > > Maybe > > /* Can we support IGD passthrough for this device? > * We require ... > */ > >> +static int is_igd_passthrough(PCIDevice *pci_dev) >> +{ >> + PCIDevice *f = pci_dev->bus->devices[PCI_DEVFN(2, 0)]; >> + if (pci_dev->bus->devices[PCI_DEVFN(2, 0)]) { >> + XenPCIPassthroughState *s = DO_UPCAST(XenPCIPassthroughState, dev, f); >> + return (is_vga_passthrough(&s->real_device) >> + && (s->real_device.vendor_id == PCI_VENDOR_ID_INTEL)); >> + } else { >> + return 0; >> + } >> +} >> + >> +void igd_pci_write(PCIDevice *pci_dev, uint32_t config_addr, >> + uint32_t val, int len) > > Same here, xen_ everywhere please. Okay. > >> +{ >> + XenHostPCIDevice dev; >> + int r; >> + >> + /* IGD read/write is through the host bridge. >> + * ISA bridge is only for detect purpose. In i915 driver it will >> + * probe ISA bridge to discover the IGD, see comment in i915_drv.c: >> + * intel_detect_pch(). > > You mean in linux kernel I guess? So change like, * probe ISA bridge to discover the IGD, see comment in Linux:i915_drv.c: > >> + */ >> + >> + assert(pci_dev->devfn == 0x00); >> + >> + if (!is_igd_passthrough(pci_dev)) { >> + goto write_default; >> + } >> + >> + /* Just work for the i915 driver. */ >> + switch (config_addr) { >> + case 0x58: /* PAVPC Offset */ >> + break; >> + default: >> + /* Just sets the emulated values. */ >> + goto write_default; >> + } >> + >> + /* Host write */ >> + r = xen_host_pci_device_get(&dev, 0, 0, 0, 0); >> + if (r) { >> + XEN_PT_ERR(pci_dev, "Can't get pci_dev_host_bridge\n"); >> + abort(); >> + } >> + >> + r = xen_host_pci_set_block(&dev, config_addr, (uint8_t *)&val, len); >> + if (r) { >> + XEN_PT_ERR(pci_dev, "Can't get pci_dev_host_bridge\n"); >> + abort(); >> + } > > > Cleaner: > > if (config_addr == 0x58) { Maybe we add other offset in the future, so we'd better keep in them in switch(). > /* Host write */ > r = xen_host_pci_device_get(&dev, 0, 0, 0, 0); > if (r) { > XEN_PT_ERR(pci_dev, "Can't get pci_dev_host_bridge\n"); > abort(); > } > > r = xen_host_pci_set_block(&dev, config_addr, (uint8_t *)&val, len); > if (r) { > XEN_PT_ERR(pci_dev, "Can't get pci_dev_host_bridge\n"); > abort(); > } > } > > Note this does not work on e.g. BE. Why do we need take BE into consideration here? Shouldn't PCI already be LE? > The best way is really to make the register writeable in wmask. > Then > pci_default_write_config(pci_dev, config_addr, val, len); > if (range_covers_byte(addr, len, 0x58)) { > .... > > r = xen_host_pci_set_block(&dev, config_addr, > pci_dev->config + config_addr, len); > } > > > > > >> + >> + xen_host_pci_device_put(&dev); >> + >> + return; >> + >> +write_default: >> + pci_default_write_config(pci_dev, config_addr, val, len); >> +} >> + >> +uint32_t igd_pci_read(PCIDevice *pci_dev, uint32_t config_addr, int len) >> +{ >> + XenHostPCIDevice dev; >> + uint32_t val; >> + int r; >> + >> + /* IGD read/write is through the host bridge. >> + * ISA bridge is only for detect purpose. In i915 driver it will >> + * probe ISA bridge to discover the IGD, see comment in i915_drv.c: >> + * intel_detect_pch(). >> + */ >> + assert(pci_dev->devfn == 0x00); >> + >> + if (!is_igd_passthrough(pci_dev)) { >> + goto read_default; >> + } >> + >> + /* Just work for the i915 driver. */ >> + switch (config_addr) { >> + case 0x08: /* revision id */ >> + case 0x2c: /* sybsystem vendor id */ >> + case 0x2e: /* sybsystem id */ > > Since you set them to match host previously, > should be no need to override now. Just *read* the host bridge, no *write* here. > >> + case 0x44: /* MCHBAR I915 */ >> + case 0x48: /* MCHBAR I965 */ >> + case 0x50: /* SNB: processor graphics control register */ >> + case 0x52: /* processor graphics control register */ >> + case 0xa0: /* top of memory */ >> + case 0xb0: /* ILK: BSM: should read from dev 2 offset 0x5c */ >> + case 0x58: /* SNB: PAVPC Offset */ >> + case 0xa4: /* SNB: graphics base of stolen memory */ >> + case 0xa8: /* SNB: base of GTT stolen memory */ > > Move the actual code here. > Sorry, whats the actual code? Are you saying we can read all values from the host bridge in advance then restore them here? Thanks Tiejun >> + break; >> + default: >> + /* Just gets the emulated values. */ > > and here. > >> + goto read_default; >> + } >> + >> + /* Host read */ >> + r = xen_host_pci_device_get(&dev, 0, 0, 0, 0); >> + if (r) { >> + goto err_out; >> + } >> + >> + r = xen_host_pci_get_block(&dev, config_addr, (uint8_t *)&val, len); >> + if (r) { >> + goto err_out; >> + } >> + >> + xen_host_pci_device_put(&dev); >> + >> + return val; >> + >> +read_default: >> + return pci_default_read_config(pci_dev, config_addr, len); >> + >> +err_out: >> + XEN_PT_ERR(pci_dev, "Can't get pci_dev_host_bridge\n"); >> + return -1; >> +} >> -- >> 1.9.1 > >