Re: [Qemu-devel] [v5][PATCH 5/5] xen, gfx passthrough: add opregion mapping

["Michael S. Tsirkin" <mst@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 <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>
> ---
> 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(&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 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