| Message ID | 1400237624-8505-7-git-send-email-tiejun.chen@intel.com |
|---|---|
| State | New |
| Headers | show |
On Fri, May 16, 2014 at 06:53:42PM +0800, Tiejun Chen wrote: > Some registers of Intel IGD are mapped in host bridge, so it needs to > passthrough these registers of physical host bridge to guest because > emulated host bridge in guest doesn't have these mappings. When you say mapped - you mean they are aliased - so if I change the value in the IGD they will change in the host bridge as well, right? > > The original patch is from Weidong Han < weidong.han @ intel.com > > > Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com> > Signed-off-by: Tiejun Chen <tiejun.chen@intel.com> > Cc:Weidong Han <weidong.han@intel.com> > --- > v2: > > * To introduce is_igd_passthrough() to make sure we touch physical host bridge > only in IGD case. > > hw/xen/xen_pt.h | 4 ++ > hw/xen/xen_pt_graphics.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 144 insertions(+) > > diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h > index 4d3a18d..507165c 100644 > --- a/hw/xen/xen_pt.h > +++ b/hw/xen/xen_pt.h > @@ -302,5 +302,9 @@ extern int xen_has_gfx_passthru; > int xen_pt_register_vga_regions(XenHostPCIDevice *dev); > int xen_pt_unregister_vga_regions(XenHostPCIDevice *dev); > int xen_pt_setup_vga(XenHostPCIDevice *dev); > +int pci_create_pch(PCIBus *bus); > +void igd_pci_write(PCIDevice *pci_dev, uint32_t config_addr, > + uint32_t val, int len); > +uint32_t igd_pci_read(PCIDevice *pci_dev, uint32_t config_addr, int len); > > #endif /* !XEN_PT_H */ > diff --git a/hw/xen/xen_pt_graphics.c b/hw/xen/xen_pt_graphics.c > index 6b86293..066bc4d 100644 > --- a/hw/xen/xen_pt_graphics.c > +++ b/hw/xen/xen_pt_graphics.c > @@ -4,6 +4,7 @@ > #include "xen_pt.h" > #include "xen-host-pci-device.h" > #include "hw/xen/xen_backend.h" > +#include "hw/pci/pci_bus.h" > > static int is_vga_passthrough(XenHostPCIDevice *dev) > { > @@ -246,3 +247,142 @@ static int create_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice *hdev) > XEN_PT_LOG(dev, "Intel PCH ISA bridge is created.\n"); > return 0; > } > + > +int pci_create_pch(PCIBus *bus) > +{ > + XenHostPCIDevice hdev; > + int r = 0; > + > + if (!xen_has_gfx_passthru) { > + return -1; > + } > + > + r = xen_host_pci_device_get(&hdev, 0, 0, 0x1f, 0); > + if (r) { > + XEN_PT_ERR(NULL, "Fail to find intel PCH in host\n"); Failed to find Intel PCH on host! > + goto err; > + } > + > + if (hdev.vendor_id == PCI_VENDOR_ID_INTEL) { > + r = create_pch_isa_bridge(bus, &hdev); > + if (r) { > + XEN_PT_ERR(NULL, "Fail to create PCH ISA bridge.\n"); Failed > + goto err; > + } > + } > + > + xen_host_pci_device_put(&hdev); > + > + return r; Remove this return and let it go through. > + > +err: > + return r; > +} > + > +/* > + * Currently we just pass this physical host bridge for IGD, 00:02.0. > + */ > +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) > +{ > + XenHostPCIDevice dev; > + int r; > + > + assert(pci_dev->devfn == 0x00); Now I am confused. That would mean the host bridge, while earlier (pci_create_pch) you were using the ISA bridge. Could you add a comment please? > + > + if (!is_igd_passthrough(pci_dev)) { > + goto write_default; > + } > + > + switch (config_addr) { > + case 0x58: /* PAVPC Offset */ > + break; > + default: > + goto write_default; As I understand this function will be called when the guest tries to touch the configuration of the VGA and write in the configuration entries. > + } > + > + /* 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(); > + } > + > + xen_host_pci_device_put(&dev); > + > + return; > + > +write_default: > + pci_default_write_config(pci_dev, config_addr, val, len); and we just allow it through. But what happens if the guest decides to change the BAR sizes? Or fiddle with the GTT? Ouch. That really looks dangerous - or maybe I am too paranoid? > +} > + > +uint32_t igd_pci_read(PCIDevice *pci_dev, uint32_t config_addr, int len) > +{ > + XenHostPCIDevice dev; > + uint32_t val; > + int r; > + > + assert(pci_dev->devfn == 0x00); > + > + if (!is_igd_passthrough(pci_dev)) { > + goto read_default; > + } > + > + switch (config_addr) { > + case 0x00: /* vendor id */ > + case 0x02: /* device id */ > + case 0x08: /* revision id */ > + case 0x2c: /* sybsystem vendor id */ > + case 0x2e: /* sybsystem id */ > + 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 */ > + break; > + default: > + 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 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
Konrad Rzeszutek Wilk wrote on 2014-05-16: > On Fri, May 16, 2014 at 06:53:42PM +0800, Tiejun Chen wrote: > > Some registers of Intel IGD are mapped in host bridge, so it needs to > > passthrough these registers of physical host bridge to guest because > > emulated host bridge in guest doesn't have these mappings. Thanks for your review for the whole series patch. > > When you say mapped - you mean they are aliased - so if I change the value in > the IGD they will change in the host bridge as well, right? I guess you mean the physical host bridge. For it, only PAVPC will write to physical host bridge directly, see: + switch (config_addr) { + case 0x58: /* PAVPC Offset */ + break; + default: + goto write_default; + } > > > > > The original patch is from Weidong Han < weidong.han @ intel.com > > > > > Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com> > > Signed-off-by: Tiejun Chen <tiejun.chen@intel.com> Cc:Weidong Han > > <weidong.han@intel.com> > > --- > > v2: > > > > * To introduce is_igd_passthrough() to make sure we touch physical host > bridge > > only in IGD case. > > > > hw/xen/xen_pt.h | 4 ++ > > hw/xen/xen_pt_graphics.c | 140 > > +++++++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 144 insertions(+) > > > > diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h index 4d3a18d..507165c > > 100644 > > --- a/hw/xen/xen_pt.h > > +++ b/hw/xen/xen_pt.h > > @@ -302,5 +302,9 @@ extern int xen_has_gfx_passthru; int > > xen_pt_register_vga_regions(XenHostPCIDevice *dev); int > > xen_pt_unregister_vga_regions(XenHostPCIDevice *dev); int > > xen_pt_setup_vga(XenHostPCIDevice *dev); > > +int pci_create_pch(PCIBus *bus); > > +void igd_pci_write(PCIDevice *pci_dev, uint32_t config_addr, > > + uint32_t val, int len); uint32_t > > +igd_pci_read(PCIDevice *pci_dev, uint32_t config_addr, int len); > > > > #endif /* !XEN_PT_H */ > > diff --git a/hw/xen/xen_pt_graphics.c b/hw/xen/xen_pt_graphics.c index > > 6b86293..066bc4d 100644 > > --- a/hw/xen/xen_pt_graphics.c > > +++ b/hw/xen/xen_pt_graphics.c > > @@ -4,6 +4,7 @@ > > #include "xen_pt.h" > > #include "xen-host-pci-device.h" > > #include "hw/xen/xen_backend.h" > > +#include "hw/pci/pci_bus.h" > > > > static int is_vga_passthrough(XenHostPCIDevice *dev) { @@ -246,3 > > +247,142 @@ static int create_pch_isa_bridge(PCIBus *bus, > XenHostPCIDevice *hdev) > > XEN_PT_LOG(dev, "Intel PCH ISA bridge is created.\n"); > > return 0; > > } > > + > > +int pci_create_pch(PCIBus *bus) > > +{ > > + XenHostPCIDevice hdev; > > + int r = 0; > > + > > + if (!xen_has_gfx_passthru) { > > + return -1; > > + } > > + > > + r = xen_host_pci_device_get(&hdev, 0, 0, 0x1f, 0); > > + if (r) { > > + XEN_PT_ERR(NULL, "Fail to find intel PCH in host\n"); > > Failed to find Intel PCH on host! > > > + goto err; > > + } > > + > > + if (hdev.vendor_id == PCI_VENDOR_ID_INTEL) { > > + r = create_pch_isa_bridge(bus, &hdev); > > + if (r) { > > + XEN_PT_ERR(NULL, "Fail to create PCH ISA bridge.\n"); > > Failed > > + goto err; > > + } > > + } > > + > > + xen_host_pci_device_put(&hdev); > > + > > + return r; > > Remove this return and let it go through. > > + > > +err: > > + return r; > > +} > > + > > +/* > > + * Currently we just pass this physical host bridge for IGD, 00:02.0. > > + */ > > +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) { > > + XenHostPCIDevice dev; > > + int r; > > + > > + assert(pci_dev->devfn == 0x00); > > Now I am confused. That would mean the host bridge, while earlier > (pci_create_pch) you were using the ISA bridge. 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(): * The reason to probe ISA bridge instead of Dev31:Fun0 is to * make graphics device passthrough work easy for VMM, that only * need to expose ISA bridge to let driver know the real hardware * underneath. This is a requirement from virtualization team. > > Could you add a comment please? Sure. > > + > > + if (!is_igd_passthrough(pci_dev)) { > > + goto write_default; > > + } > > + > > + switch (config_addr) { > > + case 0x58: /* PAVPC Offset */ > > + break; > > + default: > > + goto write_default; > > As I understand this function will be called when the guest tries to touch the > configuration of the VGA and write in the configuration entries. > > > + } > > + > > + /* 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(); > > + } > > + > > + xen_host_pci_device_put(&dev); > > + > > + return; > > + > > +write_default: > > + pci_default_write_config(pci_dev, config_addr, val, len); > > > and we just allow it through. But what happens if the guest decides to change > the BAR sizes? Or fiddle with the GTT? > > Ouch. That really looks dangerous - or maybe I am too paranoid? > I do not quite understand your concern. We only pass through PAVPC to physical host bridge. The others are handled by current logic. We don't change any of it. So what problem will be exposed by this patch? Best regards, Yang
> -----Original Message----- > From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com] > Sent: Friday, May 16, 2014 10:35 PM > To: Chen, Tiejun > Cc: anthony.perard@citrix.com; stefano.stabellini@eu.citrix.com; > mst@redhat.com; Kelly.Zytaruk@amd.com; peter.maydell@linaro.org; > xen-devel@lists.xensource.com; weidong.han@intel.com; Kay, Allen M; > qemu-devel@nongnu.org; jean.guyader@eu.citrix.com; > anthony@codemonkey.ws; Zhang, Yang Z > Subject: Re: [Xen-devel] [v2][PATCH 6/8] xen, gfx passthrough: support Intel > IGD passthrough with VT-D > > On Fri, May 16, 2014 at 06:53:42PM +0800, Tiejun Chen wrote: > > Some registers of Intel IGD are mapped in host bridge, so it needs to > > passthrough these registers of physical host bridge to guest because > > emulated host bridge in guest doesn't have these mappings. > > When you say mapped - you mean they are aliased - so if I change the value in > the IGD they will change in the host bridge as well, right? > Loos Yang already address some most points, so I just correct others based on your comments. Thanks Tiejun
On Mon, May 19, 2014 at 12:58:50AM +0000, Zhang, Yang Z wrote: > Konrad Rzeszutek Wilk wrote on 2014-05-16: > > On Fri, May 16, 2014 at 06:53:42PM +0800, Tiejun Chen wrote: > > > Some registers of Intel IGD are mapped in host bridge, so it needs to > > > passthrough these registers of physical host bridge to guest because > > > emulated host bridge in guest doesn't have these mappings. > > Thanks for your review for the whole series patch. Sure thing! .. snip.. > > > +write_default: > > > + pci_default_write_config(pci_dev, config_addr, val, len); > > > > > > and we just allow it through. But what happens if the guest decides to change > > the BAR sizes? Or fiddle with the GTT? > > > > Ouch. That really looks dangerous - or maybe I am too paranoid? > > > > I do not quite understand your concern. We only pass through PAVPC to physical host bridge. The others are handled by current logic. We don't change any of it. So what problem will be exposed by this patch? Ah, I assumed that pci_default_write_config would be writting everything without any checks. But it looks to be doing the right thing and just sets the emulated values. Could we just add comment saying that it writes to the emulated values? That way it won't trip folks. > > Best regards, > Yang > >
diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h index 4d3a18d..507165c 100644 --- a/hw/xen/xen_pt.h +++ b/hw/xen/xen_pt.h @@ -302,5 +302,9 @@ extern int xen_has_gfx_passthru; int xen_pt_register_vga_regions(XenHostPCIDevice *dev); int xen_pt_unregister_vga_regions(XenHostPCIDevice *dev); int xen_pt_setup_vga(XenHostPCIDevice *dev); +int pci_create_pch(PCIBus *bus); +void igd_pci_write(PCIDevice *pci_dev, uint32_t config_addr, + uint32_t val, int len); +uint32_t igd_pci_read(PCIDevice *pci_dev, uint32_t config_addr, int len); #endif /* !XEN_PT_H */ diff --git a/hw/xen/xen_pt_graphics.c b/hw/xen/xen_pt_graphics.c index 6b86293..066bc4d 100644 --- a/hw/xen/xen_pt_graphics.c +++ b/hw/xen/xen_pt_graphics.c @@ -4,6 +4,7 @@ #include "xen_pt.h" #include "xen-host-pci-device.h" #include "hw/xen/xen_backend.h" +#include "hw/pci/pci_bus.h" static int is_vga_passthrough(XenHostPCIDevice *dev) { @@ -246,3 +247,142 @@ static int create_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice *hdev) XEN_PT_LOG(dev, "Intel PCH ISA bridge is created.\n"); return 0; } + +int pci_create_pch(PCIBus *bus) +{ + XenHostPCIDevice hdev; + int r = 0; + + if (!xen_has_gfx_passthru) { + return -1; + } + + r = xen_host_pci_device_get(&hdev, 0, 0, 0x1f, 0); + if (r) { + XEN_PT_ERR(NULL, "Fail to find intel PCH in host\n"); + goto err; + } + + if (hdev.vendor_id == PCI_VENDOR_ID_INTEL) { + r = create_pch_isa_bridge(bus, &hdev); + if (r) { + XEN_PT_ERR(NULL, "Fail to create PCH ISA bridge.\n"); + goto err; + } + } + + xen_host_pci_device_put(&hdev); + + return r; + +err: + return r; +} + +/* + * Currently we just pass this physical host bridge for IGD, 00:02.0. + */ +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) +{ + XenHostPCIDevice dev; + int r; + + assert(pci_dev->devfn == 0x00); + + if (!is_igd_passthrough(pci_dev)) { + goto write_default; + } + + switch (config_addr) { + case 0x58: /* PAVPC Offset */ + break; + default: + 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(); + } + + 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; + + assert(pci_dev->devfn == 0x00); + + if (!is_igd_passthrough(pci_dev)) { + goto read_default; + } + + switch (config_addr) { + case 0x00: /* vendor id */ + case 0x02: /* device id */ + case 0x08: /* revision id */ + case 0x2c: /* sybsystem vendor id */ + case 0x2e: /* sybsystem id */ + 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 */ + break; + default: + 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; +}