From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59646) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WzhOQ-0003dz-PA for qemu-devel@nongnu.org; Wed, 25 Jun 2014 03:13:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WzhOM-00051B-9B for qemu-devel@nongnu.org; Wed, 25 Jun 2014 03:13:18 -0400 Received: from mx1.redhat.com ([209.132.183.28]:27256) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WzhOM-0004yy-1b for qemu-devel@nongnu.org; Wed, 25 Jun 2014 03:13:14 -0400 Date: Wed, 25 Jun 2014 10:13:21 +0300 From: "Michael S. Tsirkin" Message-ID: <20140625071321.GD25563@redhat.com> References: <1403662641-28526-1-git-send-email-tiejun.chen@intel.com> <1403662641-28526-6-git-send-email-tiejun.chen@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1403662641-28526-6-git-send-email-tiejun.chen@intel.com> Subject: Re: [Qemu-devel] [v5][PATCH 5/5] xen, gfx passthrough: add opregion mapping List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Tiejun Chen 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 Wed, Jun 25, 2014 at 10:17:21AM +0800, Tiejun Chen wrote: > The OpRegion shouldn't be mapped 1:1 because the address in the host > can't be used in the guest directly. > > This patch traps read and write access to the opregion of the Intel > GPU config space (offset 0xfc). > > The original patch is from Jean Guyader > > Signed-off-by: Yang Zhang > Signed-off-by: Tiejun Chen > Cc: Jean Guyader > --- > v5: > > * Nothing is changed. > > v4: > > * Nothing is changed. > > v3: > > * Fix some typos. > * Add more comments to make that readable. > * To unmap igd_opregion when call xen_pt_unregister_vga_regions(). > * Improve some return paths. > * We need to map 3 pages for opregion as hvmloader set. > * Force to convert igd_guest/host_opoegion as an unsigned long type > while calling xc_domain_memory_mapping(). > > v2: > > * We should return zero as an invalid address value while calling > igd_read_opregion(). > > hw/xen/xen_pt.h | 4 ++- > hw/xen/xen_pt_config_init.c | 50 ++++++++++++++++++++++++++++++++++- > hw/xen/xen_pt_graphics.c | 64 +++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 116 insertions(+), 2 deletions(-) > > diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h > index 507165c..25147cf 100644 > --- a/hw/xen/xen_pt.h > +++ b/hw/xen/xen_pt.h > @@ -63,7 +63,7 @@ typedef int (*xen_pt_conf_byte_read) > #define XEN_PT_BAR_UNMAPPED (-1) > > #define PCI_CAP_MAX 48 > - > +#define PCI_INTEL_OPREGION 0xfc > XEN_.... please PCI_CAP_MAX should be fixed too. > typedef enum { > XEN_PT_GRP_TYPE_HARDWIRED = 0, /* 0 Hardwired reg group */ > @@ -306,5 +306,7 @@ 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); > +uint32_t igd_read_opregion(XenPCIPassthroughState *s); > +void igd_write_opregion(XenPCIPassthroughState *s, uint32_t val); > > #endif /* !XEN_PT_H */ > diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c > index de9a20f..6eaaa9a 100644 > --- a/hw/xen/xen_pt_config_init.c > +++ b/hw/xen/xen_pt_config_init.c > @@ -575,6 +575,22 @@ static int xen_pt_exp_rom_bar_reg_write(XenPCIPassthroughState *s, > return 0; > } > > +static int xen_pt_intel_opregion_read(XenPCIPassthroughState *s, > + XenPTReg *cfg_entry, > + uint32_t *value, uint32_t valid_mask) > +{ > + *value = igd_read_opregion(s); > + return 0; > +} > + > +static int xen_pt_intel_opregion_write(XenPCIPassthroughState *s, > + XenPTReg *cfg_entry, uint32_t *value, > + uint32_t dev_value, uint32_t valid_mask) > +{ > + igd_write_opregion(s, *value); > + return 0; > +} > + > /* Header Type0 reg static information table */ > static XenPTRegInfo xen_pt_emu_reg_header0[] = { > /* Vendor ID reg */ > @@ -1440,6 +1456,20 @@ static XenPTRegInfo xen_pt_emu_reg_msix[] = { > }, > }; > > +static XenPTRegInfo xen_pt_emu_reg_igd_opregion[] = { > + /* Intel IGFX OpRegion reg */ > + { > + .offset = 0x0, > + .size = 4, > + .init_val = 0, > + .no_wb = 1, > + .u.dw.read = xen_pt_intel_opregion_read, > + .u.dw.write = xen_pt_intel_opregion_write, > + }, > + { > + .size = 0, > + }, > +}; > > /**************************** > * Capabilities > @@ -1677,6 +1707,14 @@ static const XenPTRegGroupInfo xen_pt_emu_reg_grps[] = { > .size_init = xen_pt_msix_size_init, > .emu_regs = xen_pt_emu_reg_msix, > }, > + /* Intel IGD Opregion group */ > + { > + .grp_id = PCI_INTEL_OPREGION, > + .grp_type = XEN_PT_GRP_TYPE_EMU, > + .grp_size = 0x4, > + .size_init = xen_pt_reg_grp_size_init, > + .emu_regs = xen_pt_emu_reg_igd_opregion, > + }, > { > .grp_size = 0, > }, > @@ -1806,7 +1844,8 @@ int xen_pt_config_init(XenPCIPassthroughState *s) > uint32_t reg_grp_offset = 0; > XenPTRegGroup *reg_grp_entry = NULL; > > - if (xen_pt_emu_reg_grps[i].grp_id != 0xFF) { > + if (xen_pt_emu_reg_grps[i].grp_id != 0xFF > + && xen_pt_emu_reg_grps[i].grp_id != PCI_INTEL_OPREGION) { > if (xen_pt_hide_dev_cap(&s->real_device, > xen_pt_emu_reg_grps[i].grp_id)) { > continue; > @@ -1819,6 +1858,15 @@ int xen_pt_config_init(XenPCIPassthroughState *s) > } > } > > + /* > + * By default we will trap up to 0x40 in the cfg space. > + * If an intel device is pass through we need to trap 0xfc, > + * therefore the size should be 0xff. > + */ > + if (xen_pt_emu_reg_grps[i].grp_id == PCI_INTEL_OPREGION) { > + reg_grp_offset = PCI_INTEL_OPREGION; > + } > + > reg_grp_entry = g_new0(XenPTRegGroup, 1); > QLIST_INIT(®_grp_entry->reg_tbl_list); > QLIST_INSERT_HEAD(&s->reg_grps, reg_grp_entry, entries); > diff --git a/hw/xen/xen_pt_graphics.c b/hw/xen/xen_pt_graphics.c > index f3fbfed..fa341ad 100644 > --- a/hw/xen/xen_pt_graphics.c > +++ b/hw/xen/xen_pt_graphics.c > @@ -6,6 +6,9 @@ > #include "hw/xen/xen_backend.h" > #include "hw/pci/pci_bus.h" > > +static unsigned long igd_guest_opregion; > +static unsigned long igd_host_opregion; > + > static int is_vga_passthrough(XenHostPCIDevice *dev) > { > return (xen_has_gfx_passthru > @@ -88,6 +91,7 @@ int xen_pt_register_vga_regions(XenHostPCIDevice *dev) > int xen_pt_unregister_vga_regions(XenHostPCIDevice *dev) > { > int i = 0; > + int ret = 0; > > if (!is_vga_passthrough(dev)) { > return 0; > @@ -114,6 +118,17 @@ int xen_pt_unregister_vga_regions(XenHostPCIDevice *dev) > } > } > > + if (igd_guest_opregion) { > + ret = xc_domain_memory_mapping(xen_xc, xen_domid, > + (unsigned long)(igd_guest_opregion >> XC_PAGE_SHIFT), > + (unsigned long)(igd_host_opregion >> XC_PAGE_SHIFT), don't spread casts all around. Should be a last resort. > + 3, > + DPCI_REMOVE_MAPPING); > + if (ret) { > + return ret; > + } > + } > + > return 0; > } > > @@ -447,3 +462,52 @@ err_out: > XEN_PT_ERR(pci_dev, "Can't get pci_dev_host_bridge\n"); > return -1; > } > + > +uint32_t igd_read_opregion(XenPCIPassthroughState *s) > +{ > + uint32_t val = 0; > + > + if (igd_guest_opregion == 0) { !igd_guest_opregion is shorter and does the same, > + return val; > + } > + > + val = igd_guest_opregion; > + > + XEN_PT_LOG(&s->dev, "Read opregion val=%x\n", val); > + return val; > +} > + > +void igd_write_opregion(XenPCIPassthroughState *s, uint32_t val) > +{ > + int ret; > + > + if (igd_guest_opregion) { > + XEN_PT_LOG(&s->dev, "opregion register already been set, ignoring %x\n", > + val); > + return; > + } > + > + xen_host_pci_get_block(&s->real_device, PCI_INTEL_OPREGION, > + (uint8_t *)&igd_host_opregion, 4); > + igd_guest_opregion = (unsigned long)(val & ~0xfff) > + | (igd_host_opregion & 0xfff); > + Clearly broken on BE. Maybe not important here but writing clean code is just as easy. uint8_t igd_host_opregion[4]; ... xen_host_pci_get_block(&s->real_device, PCI_INTEL_OPREGION, igd_host_opregion, sizeof igd_host_opregion); igd_guest_opregion = (val & ~0xfff) | (pci_get_word(igd_host_opregion) & 0xfff); 0xfff should be a macro too to avoid duplication. > + ret = xc_domain_memory_mapping(xen_xc, xen_domid, > + (unsigned long)(igd_guest_opregion >> XC_PAGE_SHIFT), > + (unsigned long)(igd_host_opregion >> XC_PAGE_SHIFT), > + 3, > + DPCI_ADD_MAPPING); > + > + if (ret) { > + XEN_PT_ERR(&s->dev, "[%d]:Can't map IGD host opregion:0x%lx to" > + " guest opregion:0x%lx.\n", ret, > + (unsigned long)(igd_host_opregion >> XC_PAGE_SHIFT), > + (unsigned long)(igd_guest_opregion >> XC_PAGE_SHIFT)); > + igd_guest_opregion = 0; > + return; > + } > + > + XEN_PT_LOG(&s->dev, "Map OpRegion: 0x%lx -> 0x%lx\n", > + (unsigned long)(igd_host_opregion >> XC_PAGE_SHIFT), > + (unsigned long)(igd_guest_opregion >> XC_PAGE_SHIFT)); > +} > -- > 1.9.1 From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: [v5][PATCH 5/5] xen, gfx passthrough: add opregion mapping Date: Wed, 25 Jun 2014 10:13:21 +0300 Message-ID: <20140625071321.GD25563@redhat.com> References: <1403662641-28526-1-git-send-email-tiejun.chen@intel.com> <1403662641-28526-6-git-send-email-tiejun.chen@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <1403662641-28526-6-git-send-email-tiejun.chen@intel.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: Tiejun Chen 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 Wed, Jun 25, 2014 at 10:17:21AM +0800, Tiejun Chen wrote: > The OpRegion shouldn't be mapped 1:1 because the address in the host > can't be used in the guest directly. > > This patch traps read and write access to the opregion of the Intel > GPU config space (offset 0xfc). > > The original patch is from Jean Guyader > > Signed-off-by: Yang Zhang > Signed-off-by: Tiejun Chen > Cc: Jean Guyader > --- > v5: > > * Nothing is changed. > > v4: > > * Nothing is changed. > > v3: > > * Fix some typos. > * Add more comments to make that readable. > * To unmap igd_opregion when call xen_pt_unregister_vga_regions(). > * Improve some return paths. > * We need to map 3 pages for opregion as hvmloader set. > * Force to convert igd_guest/host_opoegion as an unsigned long type > while calling xc_domain_memory_mapping(). > > v2: > > * We should return zero as an invalid address value while calling > igd_read_opregion(). > > hw/xen/xen_pt.h | 4 ++- > hw/xen/xen_pt_config_init.c | 50 ++++++++++++++++++++++++++++++++++- > hw/xen/xen_pt_graphics.c | 64 +++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 116 insertions(+), 2 deletions(-) > > diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h > index 507165c..25147cf 100644 > --- a/hw/xen/xen_pt.h > +++ b/hw/xen/xen_pt.h > @@ -63,7 +63,7 @@ typedef int (*xen_pt_conf_byte_read) > #define XEN_PT_BAR_UNMAPPED (-1) > > #define PCI_CAP_MAX 48 > - > +#define PCI_INTEL_OPREGION 0xfc > XEN_.... please PCI_CAP_MAX should be fixed too. > typedef enum { > XEN_PT_GRP_TYPE_HARDWIRED = 0, /* 0 Hardwired reg group */ > @@ -306,5 +306,7 @@ 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); > +uint32_t igd_read_opregion(XenPCIPassthroughState *s); > +void igd_write_opregion(XenPCIPassthroughState *s, uint32_t val); > > #endif /* !XEN_PT_H */ > diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c > index de9a20f..6eaaa9a 100644 > --- a/hw/xen/xen_pt_config_init.c > +++ b/hw/xen/xen_pt_config_init.c > @@ -575,6 +575,22 @@ static int xen_pt_exp_rom_bar_reg_write(XenPCIPassthroughState *s, > return 0; > } > > +static int xen_pt_intel_opregion_read(XenPCIPassthroughState *s, > + XenPTReg *cfg_entry, > + uint32_t *value, uint32_t valid_mask) > +{ > + *value = igd_read_opregion(s); > + return 0; > +} > + > +static int xen_pt_intel_opregion_write(XenPCIPassthroughState *s, > + XenPTReg *cfg_entry, uint32_t *value, > + uint32_t dev_value, uint32_t valid_mask) > +{ > + igd_write_opregion(s, *value); > + return 0; > +} > + > /* Header Type0 reg static information table */ > static XenPTRegInfo xen_pt_emu_reg_header0[] = { > /* Vendor ID reg */ > @@ -1440,6 +1456,20 @@ static XenPTRegInfo xen_pt_emu_reg_msix[] = { > }, > }; > > +static XenPTRegInfo xen_pt_emu_reg_igd_opregion[] = { > + /* Intel IGFX OpRegion reg */ > + { > + .offset = 0x0, > + .size = 4, > + .init_val = 0, > + .no_wb = 1, > + .u.dw.read = xen_pt_intel_opregion_read, > + .u.dw.write = xen_pt_intel_opregion_write, > + }, > + { > + .size = 0, > + }, > +}; > > /**************************** > * Capabilities > @@ -1677,6 +1707,14 @@ static const XenPTRegGroupInfo xen_pt_emu_reg_grps[] = { > .size_init = xen_pt_msix_size_init, > .emu_regs = xen_pt_emu_reg_msix, > }, > + /* Intel IGD Opregion group */ > + { > + .grp_id = PCI_INTEL_OPREGION, > + .grp_type = XEN_PT_GRP_TYPE_EMU, > + .grp_size = 0x4, > + .size_init = xen_pt_reg_grp_size_init, > + .emu_regs = xen_pt_emu_reg_igd_opregion, > + }, > { > .grp_size = 0, > }, > @@ -1806,7 +1844,8 @@ int xen_pt_config_init(XenPCIPassthroughState *s) > uint32_t reg_grp_offset = 0; > XenPTRegGroup *reg_grp_entry = NULL; > > - if (xen_pt_emu_reg_grps[i].grp_id != 0xFF) { > + if (xen_pt_emu_reg_grps[i].grp_id != 0xFF > + && xen_pt_emu_reg_grps[i].grp_id != PCI_INTEL_OPREGION) { > if (xen_pt_hide_dev_cap(&s->real_device, > xen_pt_emu_reg_grps[i].grp_id)) { > continue; > @@ -1819,6 +1858,15 @@ int xen_pt_config_init(XenPCIPassthroughState *s) > } > } > > + /* > + * By default we will trap up to 0x40 in the cfg space. > + * If an intel device is pass through we need to trap 0xfc, > + * therefore the size should be 0xff. > + */ > + if (xen_pt_emu_reg_grps[i].grp_id == PCI_INTEL_OPREGION) { > + reg_grp_offset = PCI_INTEL_OPREGION; > + } > + > reg_grp_entry = g_new0(XenPTRegGroup, 1); > QLIST_INIT(®_grp_entry->reg_tbl_list); > QLIST_INSERT_HEAD(&s->reg_grps, reg_grp_entry, entries); > diff --git a/hw/xen/xen_pt_graphics.c b/hw/xen/xen_pt_graphics.c > index f3fbfed..fa341ad 100644 > --- a/hw/xen/xen_pt_graphics.c > +++ b/hw/xen/xen_pt_graphics.c > @@ -6,6 +6,9 @@ > #include "hw/xen/xen_backend.h" > #include "hw/pci/pci_bus.h" > > +static unsigned long igd_guest_opregion; > +static unsigned long igd_host_opregion; > + > static int is_vga_passthrough(XenHostPCIDevice *dev) > { > return (xen_has_gfx_passthru > @@ -88,6 +91,7 @@ int xen_pt_register_vga_regions(XenHostPCIDevice *dev) > int xen_pt_unregister_vga_regions(XenHostPCIDevice *dev) > { > int i = 0; > + int ret = 0; > > if (!is_vga_passthrough(dev)) { > return 0; > @@ -114,6 +118,17 @@ int xen_pt_unregister_vga_regions(XenHostPCIDevice *dev) > } > } > > + if (igd_guest_opregion) { > + ret = xc_domain_memory_mapping(xen_xc, xen_domid, > + (unsigned long)(igd_guest_opregion >> XC_PAGE_SHIFT), > + (unsigned long)(igd_host_opregion >> XC_PAGE_SHIFT), don't spread casts all around. Should be a last resort. > + 3, > + DPCI_REMOVE_MAPPING); > + if (ret) { > + return ret; > + } > + } > + > return 0; > } > > @@ -447,3 +462,52 @@ err_out: > XEN_PT_ERR(pci_dev, "Can't get pci_dev_host_bridge\n"); > return -1; > } > + > +uint32_t igd_read_opregion(XenPCIPassthroughState *s) > +{ > + uint32_t val = 0; > + > + if (igd_guest_opregion == 0) { !igd_guest_opregion is shorter and does the same, > + return val; > + } > + > + val = igd_guest_opregion; > + > + XEN_PT_LOG(&s->dev, "Read opregion val=%x\n", val); > + return val; > +} > + > +void igd_write_opregion(XenPCIPassthroughState *s, uint32_t val) > +{ > + int ret; > + > + if (igd_guest_opregion) { > + XEN_PT_LOG(&s->dev, "opregion register already been set, ignoring %x\n", > + val); > + return; > + } > + > + xen_host_pci_get_block(&s->real_device, PCI_INTEL_OPREGION, > + (uint8_t *)&igd_host_opregion, 4); > + igd_guest_opregion = (unsigned long)(val & ~0xfff) > + | (igd_host_opregion & 0xfff); > + Clearly broken on BE. Maybe not important here but writing clean code is just as easy. uint8_t igd_host_opregion[4]; ... xen_host_pci_get_block(&s->real_device, PCI_INTEL_OPREGION, igd_host_opregion, sizeof igd_host_opregion); igd_guest_opregion = (val & ~0xfff) | (pci_get_word(igd_host_opregion) & 0xfff); 0xfff should be a macro too to avoid duplication. > + ret = xc_domain_memory_mapping(xen_xc, xen_domid, > + (unsigned long)(igd_guest_opregion >> XC_PAGE_SHIFT), > + (unsigned long)(igd_host_opregion >> XC_PAGE_SHIFT), > + 3, > + DPCI_ADD_MAPPING); > + > + if (ret) { > + XEN_PT_ERR(&s->dev, "[%d]:Can't map IGD host opregion:0x%lx to" > + " guest opregion:0x%lx.\n", ret, > + (unsigned long)(igd_host_opregion >> XC_PAGE_SHIFT), > + (unsigned long)(igd_guest_opregion >> XC_PAGE_SHIFT)); > + igd_guest_opregion = 0; > + return; > + } > + > + XEN_PT_LOG(&s->dev, "Map OpRegion: 0x%lx -> 0x%lx\n", > + (unsigned long)(igd_host_opregion >> XC_PAGE_SHIFT), > + (unsigned long)(igd_guest_opregion >> XC_PAGE_SHIFT)); > +} > -- > 1.9.1