[v2,8/8] xen, gfx passthrough: add opregion mapping

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 <jean.guyader@eu.citrix.com>

Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
Cc: Jean Guyader <jean.guyader@eu.citrix.com>
---
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(-)

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 <jean.guyader@eu.citrix.com>
> 
> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> Cc: Jean Guyader <jean.guyader@eu.citrix.com>
> ---
> 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) {
>              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;
> +        }
> +
>          reg_grp_entry = g_new0(XenPTRegGroup, 1);
>          QLIST_INIT(&reg_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 066bc4d..b25ecae 100644
> --- a/hw/xen/xen_pt_graphics.c
> +++ b/hw/xen/xen_pt_graphics.c
> @@ -6,6 +6,8 @@
>  #include "hw/xen/xen_backend.h"
>  #include "hw/pci/pci_bus.h"
>  
> +static int igd_guest_opregion;
> +
>  static int is_vga_passthrough(XenHostPCIDevice *dev)
>  {
>      return (xen_has_gfx_passthru
> @@ -386,3 +388,48 @@ 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) {
> +        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)
> +{
> +    uint32_t host_opregion = 0;
> +    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 *)&host_opregion, 4);
> +    igd_guest_opregion = (val & ~0xfff) | (host_opregion & 0xfff);
> +
> +    ret = xc_domain_memory_mapping(xen_xc, xen_domid,
> +            igd_guest_opregion >> XC_PAGE_SHIFT,
> +            host_opregion >> XC_PAGE_SHIFT,
> +            2,
> +            DPCI_ADD_MAPPING);
> +
> +    if (ret != 0) {
> +        XEN_PT_ERR(&s->dev, "Error: Can't map opregion\n");
> +        igd_guest_opregion = 0;
> +        return;
> +    }
> +
> +    XEN_PT_LOG(&s->dev, "Map OpRegion: %x -> %x\n", host_opregion,
> +            igd_guest_opregion);
> +}
> -- 
> 1.9.1
>

> -----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 <jean.guyader@eu.citrix.com>
> >
> > Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> > Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> > Cc: Jean Guyader <jean.guyader@eu.citrix.com>
> > ---
> > 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.
+    */

Thanks
Tiejun

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 <jean.guyader@eu.citrix.com>
> > >
> > > Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> > > Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> > > Cc: Jean Guyader <jean.guyader@eu.citrix.com>
> > > ---
> > > 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!

> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> Sent: Tuesday, May 20, 2014 6:51 PM
> To: Chen, Tiejun
> Cc: Stefano Stabellini; anthony.perard@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; Kay, Allen M; Zhang, Yang Z
> Subject: RE: [v2][PATCH 8/8] xen, gfx passthrough: add opregion mapping
> 

[snip]

> > > >
> > > > +        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!

Sure.

Thanks
Tiejun