Re: [Xen-devel] [PATCH] qemu-xen: Intel GPU passthrough, fix OpRegion mapping. (v2)

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Fri, 2 Dec 2011, Jean Guyader 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).
> 
> To work correctly this patch needs a change in hvmloader.
> 
> HVMloader will allocate 2 pages for the OpRegion and write this address
> on the config space of the Intel GPU. Qemu will trap and map the host
> OpRegion to the guest. Any write to this offset after that won't have
> any effect. Any read of this config space offset will return the address
> in the guest.
> 
> Signed-off-by: Jean Guyader <jean.guyader@xxxxxxxxxxxxx>
> ---
>  hw/pass-through.c |   52 ++++++++++++++++++++++++++++++++++---
>  hw/pass-through.h |    4 +++
>  hw/pt-graphics.c  |   73 
> +++++++++++++++++++++++++++++++++++++++--------------
>  3 files changed, 105 insertions(+), 24 deletions(-)
>
> diff --git a/hw/pass-through.c b/hw/pass-through.c
> index 919937f..1378022 100644
> --- a/hw/pass-through.c
> +++ b/hw/pass-through.c
> @@ -237,6 +237,14 @@ static int pt_bar_reg_restore(struct pt_dev *ptdev,
>  static int pt_exp_rom_bar_reg_restore(struct pt_dev *ptdev,
>      struct pt_reg_tbl *cfg_entry,
>      uint32_t real_offset, uint32_t dev_value, uint32_t *value);
> +static int pt_intel_opregion_read(struct pt_dev *ptdev,
> +        struct pt_reg_tbl *cfg_entry,
> +        uint32_t *value, uint32_t valid_mask);
> +static int pt_intel_opregion_write(struct pt_dev *ptdev,
> +        struct pt_reg_tbl *cfg_entry,
> +        uint32_t *value, uint32_t dev_value, uint32_t valid_mask);
> +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);
>  
>  /* pt_reg_info_tbl declaration
>   * - only for emulated register (either a part or whole bit).
> @@ -443,6 +451,18 @@ static struct pt_reg_info_tbl pt_emu_reg_header0_tbl[] = 
> {
>          .u.dw.write = pt_exp_rom_bar_reg_write,
>          .u.dw.restore = pt_exp_rom_bar_reg_restore,
>      },
> +    /* Vendor ID reg */
> +    {
> +        .offset     = PCI_INTEL_OPREGION,
> +        .size       = 4,
> +        .init_val   = 0x0000,
> +        .ro_mask    = 0xFFFFFFFF,
> +        .emu_mask   = 0xFFFFFFFF,
> +        .init       = pt_common_reg_init,
> +        .u.dw.read   = pt_intel_opregion_read,
> +        .u.dw.write  = pt_intel_opregion_write,
> +        .u.dw.restore  = NULL,
> +    },
>      {
>          .size = 0,
>      },
> @@ -736,7 +756,7 @@ static const struct pt_reg_grp_info_tbl 
> pt_emu_reg_grp_tbl[] = {
>          .grp_id     = 0xFF,
>          .grp_type   = GRP_TYPE_EMU,
>          .grp_size   = 0x40,
> -        .size_init  = pt_reg_grp_size_init,
> +        .size_init  = pt_reg_grp_header0_size_init,
>          .emu_reg_tbl= pt_emu_reg_header0_tbl,
>      },
>      /* PCI PowerManagement Capability reg group */
> @@ -1400,7 +1420,7 @@ static struct pt_reg_grp_tbl* pt_find_reg_grp(struct 
> pt_dev *ptdev,
>      {
>          /* check address */
>          if ((reg_grp_entry->base_offset <= address) &&
> -            ((reg_grp_entry->base_offset + reg_grp_entry->size) > address))
> +            ((reg_grp_entry->base_offset + reg_grp_entry->size) >= address))
>              goto out;
>      }
>      /* group entry not found */

I think that this change is wrong


> @@ -1455,8 +1475,7 @@ out:
>      return index;
>  }
>  
> -static void pt_pci_write_config(PCIDevice *d, uint32_t address, uint32_t val,
> -                                int len)
> +void pt_pci_write_config(PCIDevice *d, uint32_t address, uint32_t val, int 
> len)
>  {
>      struct pt_dev *assigned_device = (struct pt_dev *)d;
>      struct pci_dev *pci_dev = assigned_device->pci_dev;

why are you removing static here?


> @@ -1637,7 +1656,7 @@ exit:
>      return;
>  }
>  
> -static uint32_t pt_pci_read_config(PCIDevice *d, uint32_t address, int len)
> +uint32_t pt_pci_read_config(PCIDevice *d, uint32_t address, int len)
>  {
>      struct pt_dev *assigned_device = (struct pt_dev *)d;
>      struct pci_dev *pci_dev = assigned_device->pci_dev;

I think you probably can get away without making this function
externally visible too, see below


> @@ -2965,6 +2984,14 @@ static uint32_t pt_msixctrl_reg_init(struct pt_dev 
> *ptdev,
>      return reg->init_val;
>  }
>  
> +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)
> +{
> +    if (igd_passthru)
> +        return 0xFF;
> +    return grp_reg->grp_size;
> +}

Can you please add a comment on why you need to do that?


>  /* get register group size */
>  static uint8_t pt_reg_grp_size_init(struct pt_dev *ptdev,
>          struct pt_reg_grp_info_tbl *grp_reg, uint32_t base_offset)
> @@ -4113,6 +4140,21 @@ static int pt_pmcsr_reg_restore(struct pt_dev *ptdev,
>      return 0;
>  }
>  
> +static int pt_intel_opregion_read(struct pt_dev *ptdev,
> +        struct pt_reg_tbl *cfg_entry,
> +        uint32_t *value, uint32_t valid_mask)
> +{
> +    *value = igd_read_opregion(ptdev);
> +    return 0;
> +}
> +
> +static int pt_intel_opregion_write(struct pt_dev *ptdev,
> +        struct pt_reg_tbl *cfg_entry,
> +        uint32_t *value, uint32_t dev_value, uint32_t valid_mask)
> +{
> +    igd_write_opregion(ptdev, *value);
> +    return 0;
> +}
>  static struct pt_dev * register_real_device(PCIBus *e_bus,
>          const char *e_dev_name, int e_devfn, uint8_t r_bus, uint8_t r_dev,
>          uint8_t r_func, uint32_t machine_irq, struct pci_access *pci_access,
> diff --git a/hw/pass-through.h b/hw/pass-through.h
> index 884139c..9ab9638 100644
> --- a/hw/pass-through.h
> +++ b/hw/pass-through.h
> @@ -422,6 +422,10 @@ PCIBus *intel_pci_bridge_init(PCIBus *bus, int devfn, 
> uint16_t vid,
>             uint16_t did, const char *name, uint16_t revision);
>  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(struct pt_dev *pci_dev);
> +void igd_write_opregion(struct pt_dev *real_dev, uint32_t val);
> +uint32_t pt_pci_read_config(PCIDevice *d, uint32_t address, int len);
> +void pt_pci_write_config(PCIDevice *d, uint32_t address, uint32_t val, int 
> len);
>  
>  #endif /* __PASSTHROUGH_H__ */
>  
> diff --git a/hw/pt-graphics.c b/hw/pt-graphics.c
> index fec7390..2e1dc44 100644
> --- a/hw/pt-graphics.c
> +++ b/hw/pt-graphics.c
> @@ -13,6 +13,8 @@
>  extern int gfx_passthru;
>  extern int igd_passthru;
>  
> +static uint32_t igd_guest_opregion = 0;
> +
>  static int pch_map_irq(PCIDevice *pci_dev, int irq_num)
>  {
>      PT_LOG("pch_map_irq called\n");
> @@ -37,6 +39,54 @@ void intel_pch_init(PCIBus *bus)
>                          pch_map_irq, "intel_bridge_1f");
>  }
>  
> +uint32_t igd_read_opregion(struct pt_dev *pci_dev)
> +{
> +    uint32_t val = -1;
> +
> +    if ( igd_guest_opregion )
> +        val = pt_pci_read_config((PCIDevice*)pci_dev, PCI_INTEL_OPREGION, 4);
> +
> +#ifdef PT_DEBUG_PCI_CONFIG_ACCESS
> +    PT_LOG_DEV((PCIDevice*)pci_dev, "addr=%x len=%x val=%x\n",
> +            PCI_INTEL_OPREGION, 4, val);
> +#endif
> +    return val;
> +}

Can you rearrange this function is a way that you perform the
pt_pci_read_config directly in pt_intel_opregion_read?



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel