From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37609) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wmhe3-0003K5-RH for qemu-devel@nongnu.org; Tue, 20 May 2014 06:51:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wmhdz-0007rm-T9 for qemu-devel@nongnu.org; Tue, 20 May 2014 06:51:43 -0400 Received: from smtp02.citrix.com ([66.165.176.63]:37228) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wmhdz-0007rc-Me for qemu-devel@nongnu.org; Tue, 20 May 2014 06:51:39 -0400 Date: Tue, 20 May 2014 11:50:56 +0100 From: Stefano Stabellini In-Reply-To: Message-ID: References: <1400237624-8505-1-git-send-email-tiejun.chen@intel.com> <1400237624-8505-9-git-send-email-tiejun.chen@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Subject: Re: [Qemu-devel] [v2][PATCH 8/8] xen, gfx passthrough: add opregion mapping List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Chen, Tiejun" Cc: "peter.maydell@linaro.org" , "xen-devel@lists.xensource.com" , "mst@redhat.com" , Stefano Stabellini , "Kay, Allen M" , "qemu-devel@nongnu.org" , "Kelly.Zytaruk@amd.com" , "Zhang, Yang Z" , "anthony@codemonkey.ws" , "anthony.perard@citrix.com" On Tue, 20 May 2014, Chen, Tiejun wrote: > > -----Original Message----- > > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com] > > Sent: Monday, May 19, 2014 7:54 PM > > To: Chen, Tiejun > > Cc: anthony.perard@citrix.com; stefano.stabellini@eu.citrix.com; > > mst@redhat.com; Kelly.Zytaruk@amd.com; qemu-devel@nongnu.org; > > xen-devel@lists.xensource.com; peter.maydell@linaro.org; > > anthony@codemonkey.ws; weidong.han@intel.com; Kay, Allen M; > > jean.guyader@eu.citrix.com; Zhang, Yang Z > > Subject: Re: [v2][PATCH 8/8] xen, gfx passthrough: add opregion mapping > > > > On Fri, 16 May 2014, 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 > > > --- > > > 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 | 45 > > ++++++++++++++++++++++++++++++++++++++++++- > > > hw/xen/xen_pt_graphics.c | 47 > > +++++++++++++++++++++++++++++++++++++++++++++ > > > 3 files changed, 94 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 > > > > > > 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..cf36a40 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, > > > }, > > > > If I am not mistaken, in the original patch to qemu-xen-traditional, we were not > > adding an Intel IGD Opregion group. Instead we were changing the size of the > > header Type0 reg group: > > > > +static uint8_t pt_reg_grp_header0_size_init(struct pt_dev *ptdev, > > + struct pt_reg_grp_info_tbl *grp_reg, uint32_t base_offset) { > > + /* > > + ** 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 (igd_passthru) > > + return 0xFF; > > + return grp_reg->grp_size; > > +} > > > > Here instead we are adding the new group and forcing the offset to be > > PCI_INTEL_OPREGION, below. It looks functionally equivalent, but nicer. > > > > But wouldn't it be even better to have find_cap_offset return the right offset > > for Intel IGD Opregion group? Why do we need to manually set reg_grp_offset > > to PCI_INTEL_OPREGION? > > > > > > > > > @@ -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) { > > Actually in this emulated case we still call find_cap_offset() to get reg_grp_offset. > > > > if (xen_pt_hide_dev_cap(&s->real_device, > > > > > xen_pt_emu_reg_grps[i].grp_id)) { > > > continue; > > > @@ -1819,6 +1858,10 @@ int xen_pt_config_init(XenPCIPassthroughState > > *s) > > > } > > > } > > > > > > + if (xen_pt_emu_reg_grps[i].grp_id == PCI_INTEL_OPREGION) { > > > + reg_grp_offset = PCI_INTEL_OPREGION; > > > + } > > > + > > But for this pass through scenario, we have to set 0xfc manually since we need to trap 0xfc completely in that comment: > > + /* > + ** 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. > + */ OK. Can you please keep this comment in your patch? Thanks! From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefano Stabellini Subject: Re: [v2][PATCH 8/8] xen, gfx passthrough: add opregion mapping Date: Tue, 20 May 2014 11:50:56 +0100 Message-ID: References: <1400237624-8505-1-git-send-email-tiejun.chen@intel.com> <1400237624-8505-9-git-send-email-tiejun.chen@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Return-path: In-Reply-To: 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: "Chen, Tiejun" Cc: "peter.maydell@linaro.org" , "xen-devel@lists.xensource.com" , "mst@redhat.com" , Stefano Stabellini , "Kay, Allen M" , "qemu-devel@nongnu.org" , "Kelly.Zytaruk@amd.com" , "Zhang, Yang Z" , "anthony@codemonkey.ws" , "anthony.perard@citrix.com" List-Id: xen-devel@lists.xenproject.org On Tue, 20 May 2014, Chen, Tiejun wrote: > > -----Original Message----- > > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com] > > Sent: Monday, May 19, 2014 7:54 PM > > To: Chen, Tiejun > > Cc: anthony.perard@citrix.com; stefano.stabellini@eu.citrix.com; > > mst@redhat.com; Kelly.Zytaruk@amd.com; qemu-devel@nongnu.org; > > xen-devel@lists.xensource.com; peter.maydell@linaro.org; > > anthony@codemonkey.ws; weidong.han@intel.com; Kay, Allen M; > > jean.guyader@eu.citrix.com; Zhang, Yang Z > > Subject: Re: [v2][PATCH 8/8] xen, gfx passthrough: add opregion mapping > > > > On Fri, 16 May 2014, 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 > > > --- > > > 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 | 45 > > ++++++++++++++++++++++++++++++++++++++++++- > > > hw/xen/xen_pt_graphics.c | 47 > > +++++++++++++++++++++++++++++++++++++++++++++ > > > 3 files changed, 94 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 > > > > > > 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..cf36a40 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, > > > }, > > > > If I am not mistaken, in the original patch to qemu-xen-traditional, we were not > > adding an Intel IGD Opregion group. Instead we were changing the size of the > > header Type0 reg group: > > > > +static uint8_t pt_reg_grp_header0_size_init(struct pt_dev *ptdev, > > + struct pt_reg_grp_info_tbl *grp_reg, uint32_t base_offset) { > > + /* > > + ** 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 (igd_passthru) > > + return 0xFF; > > + return grp_reg->grp_size; > > +} > > > > Here instead we are adding the new group and forcing the offset to be > > PCI_INTEL_OPREGION, below. It looks functionally equivalent, but nicer. > > > > But wouldn't it be even better to have find_cap_offset return the right offset > > for Intel IGD Opregion group? Why do we need to manually set reg_grp_offset > > to PCI_INTEL_OPREGION? > > > > > > > > > @@ -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) { > > Actually in this emulated case we still call find_cap_offset() to get reg_grp_offset. > > > > if (xen_pt_hide_dev_cap(&s->real_device, > > > > > xen_pt_emu_reg_grps[i].grp_id)) { > > > continue; > > > @@ -1819,6 +1858,10 @@ int xen_pt_config_init(XenPCIPassthroughState > > *s) > > > } > > > } > > > > > > + if (xen_pt_emu_reg_grps[i].grp_id == PCI_INTEL_OPREGION) { > > > + reg_grp_offset = PCI_INTEL_OPREGION; > > > + } > > > + > > But for this pass through scenario, we have to set 0xfc manually since we need to trap 0xfc completely in that comment: > > + /* > + ** 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. > + */ OK. Can you please keep this comment in your patch? Thanks!