Xen devel - Nov 2011 - [PATCH] qemu-xen: Intel GPU passthrough, fix 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).

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@eu.citrix.com>
---
 hw/pass-through.c |    8 ++--
 hw/pass-through.h |    4 ++
 hw/pt-graphics.c  |   96 ++++++++++++++++++++++++++++++++++++++++++----------
 3 files changed, 85 insertions(+), 23 deletions(-)



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

On 27/11 04:23, 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.
> 
Ian/Allen,

The pending change to hvmloader has been applied already to xen-unstable.
Is the change alright with you?

Jean

Jean Guyader writes ("[Xen-devel] [PATCH] qemu-xen: Intel GPU passthrough,
fix OpRegion mapping."):
> The OpRegion shouldn''t be mapped 1:1 because the address in the
host
> can''t be used in the guest directly.
I confess that I don''t understand this area of the code at all.  Would
anyone like to comment on this patch ?

Ian.

On Sun, 27 Nov 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@eu.citrix.com>
> ---
>  hw/pass-through.c |    8 ++--
>  hw/pass-through.h |    4 ++
>  hw/pt-graphics.c  |   96
++++++++++++++++++++++++++++++++++++++++++----------
>  3 files changed, 85 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/pass-through.c b/hw/pass-through.c
> index 919937f..976d28f 100644
> --- a/hw/pass-through.c
> +++ b/hw/pass-through.c
> @@ -1455,8 +1455,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;
> @@ -1637,7 +1636,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;
> @@ -4191,7 +4190,8 @@ static struct pt_dev * register_real_device(PCIBus
*e_bus,
>      /* Register device */
>      assigned_device = (struct pt_dev *) pci_register_device(e_bus,
e_dev_name,
>                                  sizeof(struct pt_dev), e_devfn,
> -                                pt_pci_read_config, pt_pci_write_config);
> +                                gfx_passthru ? vgapt_pci_read_config :
pt_pci_read_config,
> +                                gfx_passthru ? vgapt_pci_write_config :
pt_pci_write_config);
Would it be possible to call the two vgapt_* functions from
pt_pci_read/write_config? Maybe hooking them into pt_emu_reg_grp_tbl?

>      if ( assigned_device == NULL )
>      {
>          PT_LOG("Error: couldn''t register real
device\n");
> diff --git a/hw/pass-through.h b/hw/pass-through.h
> index 884139c..c898c7c 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);
> +void vgapt_pci_write_config(PCIDevice *pci_dev, uint32_t config_addr,
uint32_t val, int len);
> +uint32_t vgapt_pci_read_config(PCIDevice *pci_dev, uint32_t config_addr,
int len);
> +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..33cbff5 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,77 @@ void intel_pch_init(PCIBus *bus)
>                          pch_map_irq, "intel_bridge_1f");
>  }
>  
> +static void igd_write_opregion(PCIDevice *pci_dev, uint32_t val, int len)
> +{
> +    struct pt_dev *real_dev = (struct pt_dev *)pci_dev;
> +    uint32_t host_opregion = 0;
> +    int ret;
> +
> +    if ( igd_guest_opregion || len != 4 )
> +        return;
You should print a warning here.

> +    host_opregion = pt_pci_host_read(real_dev->pci_dev,
> +            PCI_INTEL_OPREGION, len);
> +    igd_guest_opregion = (val & ~0xfff) | (host_opregion & 0xfff);
> +    PT_LOG("Map OpRegion: %x -> %x\n", host_opregion,
igd_guest_opregion);
> +
> +    ret = xc_domain_memory_mapping(xc_handle, domid,
> +            igd_guest_opregion >> XC_PAGE_SHIFT,
> +            host_opregion >> XC_PAGE_SHIFT,
> +            2,
> +            DPCI_ADD_MAPPING);
> +
> +    if ( ret != 0 )
> +    {
> +        PT_LOG("Error: Can''t map opregion\n");
> +        igd_guest_opregion = 0;
> +    }
> +}
> +
> +void vgapt_pci_write_config(PCIDevice *pci_dev, uint32_t config_addr,
uint32_t val, int len)
> +{
> +#ifdef PT_DEBUG_PCI_CONFIG_ACCESS
> +    PT_LOG("vgapt_pci_write_config: %x:%x.%x: addr=%x len=%x
val=%x\n",
> +            pci_bus_num(pci_dev->bus), PCI_SLOT(pci_dev->devfn),
> +            PCI_FUNC(pci_dev->devfn), config_addr, len, val);
> +#endif
> +
> +    if ( igd_passthru )
> +    {
> +        switch ( config_addr )
> +        {
> +            case 0xfc : /* Opregion */
> +                igd_write_opregion(pci_dev, val, len);
> +                return;
> +        }
> +    }
Shouldn''t you be calling igd_write_opregion only if vendor_id
=PCI_VENDOR_ID_INTEL?

> +    pt_pci_write_config(pci_dev, config_addr, val, len);
> +}
> +
> +uint32_t vgapt_pci_read_config(PCIDevice *pci_dev, uint32_t config_addr,
int len)
> +{
> +    uint32_t val;
> +
> +    val = pt_pci_read_config(pci_dev, config_addr, len);
> +
> +    if ( igd_passthru )
> +    {
> +        switch ( config_addr )
> +        {
> +            case 0xfc: /* OpRegion */
> +                if ( igd_guest_opregion )
> +                    val = igd_guest_opregion;
> +                break;
> +        }
> +    }
Shouldn''t you call pt_pci_read_config only at the end, in case it is
not
igd_passthru or it is not config_addr == 0xfc?

> +#ifdef PT_DEBUG_PCI_CONFIG_ACCESS
> +    PT_LOG_DEV(pci_dev, "vgapt_pci_read_config: %x:%x.%x: addr=%x
len=%x val=%x\n",
> +            config_addr, len, val);
> +#endif
> +    return val;
> +}
> +
>  void igd_pci_write(PCIDevice *pci_dev, uint32_t config_addr, uint32_t val,
int len)
>  {
>      struct pci_dev *pci_dev_host_bridge = pt_pci_get_dev(0, 0, 0);
> @@ -99,7 +172,6 @@ uint32_t igd_pci_read(PCIDevice *pci_dev, uint32_t
config_addr, int len)
>  int register_vga_regions(struct pt_dev *real_device)
>  {
>      u16 vendor_id;
> -    int igd_opregion;
>      int ret = 0;
>  
>      if ( !gfx_passthru || real_device->pci_dev->device_class !=
0x0300 )
> @@ -117,19 +189,6 @@ int register_vga_regions(struct pt_dev *real_device)
>              0x20,
>              DPCI_ADD_MAPPING);
>  
> -    /* 1:1 map ASL Storage register value */
> -    vendor_id = pt_pci_host_read(real_device->pci_dev, 0, 2);
> -    igd_opregion = pt_pci_host_read(real_device->pci_dev,
PCI_INTEL_OPREGION, 4);
> -    if ( (vendor_id == PCI_VENDOR_ID_INTEL ) && igd_opregion )
> -    {
> -        ret |= xc_domain_memory_mapping(xc_handle, domid,
> -                igd_opregion >> XC_PAGE_SHIFT,
> -                igd_opregion >> XC_PAGE_SHIFT,
> -                2,
> -                DPCI_ADD_MAPPING);
> -        PT_LOG("register_vga: igd_opregion = %x\n",
igd_opregion);
> -    }
> -
>      if ( ret != 0 )
>          PT_LOG("VGA region mapping failed\n");
>  
> @@ -141,7 +200,7 @@ int register_vga_regions(struct pt_dev *real_device)
>   */
>  int unregister_vga_regions(struct pt_dev *real_device)
>  {
> -    u32 vendor_id, igd_opregion;
> +    u32 vendor_id;
>      int ret = 0;
>  
>      if ( !gfx_passthru || real_device->pci_dev->device_class !=
0x0300 )
> @@ -160,12 +219,11 @@ int unregister_vga_regions(struct pt_dev
*real_device)
>              DPCI_REMOVE_MAPPING);
>  
>      vendor_id = pt_pci_host_read(real_device->pci_dev, PCI_VENDOR_ID,
2);
> -    igd_opregion = pt_pci_host_read(real_device->pci_dev,
PCI_INTEL_OPREGION, 4);
> -    if ( (vendor_id == PCI_VENDOR_ID_INTEL) && igd_opregion )
> +    if ( (vendor_id == PCI_VENDOR_ID_INTEL) && igd_guest_opregion
)
>      {
>          ret |= xc_domain_memory_mapping(xc_handle, domid,
> -                igd_opregion >> XC_PAGE_SHIFT,
> -                igd_opregion >> XC_PAGE_SHIFT,
> +                igd_guest_opregion >> XC_PAGE_SHIFT,
> +                igd_guest_opregion >> XC_PAGE_SHIFT,
>                  2,
>                  DPCI_REMOVE_MAPPING);
>      }

On Sun, Nov 27, 2011 at 04:23:26PM +0000, 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@eu.citrix.com>
> ---
>  hw/pass-through.c |    8 ++--
>  hw/pass-through.h |    4 ++
>  hw/pt-graphics.c  |   96
++++++++++++++++++++++++++++++++++++++++++----------
>  3 files changed, 85 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/pass-through.c b/hw/pass-through.c
> index 919937f..976d28f 100644
> --- a/hw/pass-through.c
> +++ b/hw/pass-through.c
> @@ -1455,8 +1455,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;
> @@ -1637,7 +1636,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;
> @@ -4191,7 +4190,8 @@ static struct pt_dev * register_real_device(PCIBus
*e_bus,
>      /* Register device */
>      assigned_device = (struct pt_dev *) pci_register_device(e_bus,
e_dev_name,
>                                  sizeof(struct pt_dev), e_devfn,
> -                                pt_pci_read_config, pt_pci_write_config);
> +                                gfx_passthru ? vgapt_pci_read_config :
pt_pci_read_config,
> +                                gfx_passthru ? vgapt_pci_write_config :
pt_pci_write_config);
>      if ( assigned_device == NULL )
>      {
>          PT_LOG("Error: couldn''t register real
device\n");
> diff --git a/hw/pass-through.h b/hw/pass-through.h
> index 884139c..c898c7c 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);
> +void vgapt_pci_write_config(PCIDevice *pci_dev, uint32_t config_addr,
uint32_t val, int len);
> +uint32_t vgapt_pci_read_config(PCIDevice *pci_dev, uint32_t config_addr,
int len);
> +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..33cbff5 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,77 @@ void intel_pch_init(PCIBus *bus)
>                          pch_map_irq, "intel_bridge_1f");
>  }
>  
> +static void igd_write_opregion(PCIDevice *pci_dev, uint32_t val, int len)
> +{
> +    struct pt_dev *real_dev = (struct pt_dev *)pci_dev;
> +    uint32_t host_opregion = 0;
> +    int ret;
> +
> +    if ( igd_guest_opregion || len != 4 )
> +        return;
> +
> +    host_opregion = pt_pci_host_read(real_dev->pci_dev,
> +            PCI_INTEL_OPREGION, len);
The tabs here are a bit weird.
> +    igd_guest_opregion = (val & ~0xfff) | (host_opregion & 0xfff);
> +    PT_LOG("Map OpRegion: %x -> %x\n", host_opregion,
igd_guest_opregion);
> +
> +    ret = xc_domain_memory_mapping(xc_handle, domid,
> +            igd_guest_opregion >> XC_PAGE_SHIFT,
> +            host_opregion >> XC_PAGE_SHIFT,
> +            2,
> +            DPCI_ADD_MAPPING);
> +
Shouldn''t you unmap the older region first?
> +    if ( ret != 0 )
> +    {
> +        PT_LOG("Error: Can''t map opregion\n");
> +        igd_guest_opregion = 0;
> +    }
> +}
> +
> +void vgapt_pci_write_config(PCIDevice *pci_dev, uint32_t config_addr,
uint32_t val, int len)
> +{
> +#ifdef PT_DEBUG_PCI_CONFIG_ACCESS
> +    PT_LOG("vgapt_pci_write_config: %x:%x.%x: addr=%x len=%x
val=%x\n",
> +            pci_bus_num(pci_dev->bus), PCI_SLOT(pci_dev->devfn),
> +            PCI_FUNC(pci_dev->devfn), config_addr, len, val);
> +#endif
> +
> +    if ( igd_passthru )
> +    {
> +        switch ( config_addr )
> +        {
> +            case 0xfc : /* Opregion */
> +                igd_write_opregion(pci_dev, val, len);
> +                return;
No default case? I would think the compiler would throw a fit for that.
> +        }
> +    }
> +
> +    pt_pci_write_config(pci_dev, config_addr, val, len);
> +}
> +
> +uint32_t vgapt_pci_read_config(PCIDevice *pci_dev, uint32_t config_addr,
int len)
> +{
> +    uint32_t val;
> +
> +    val = pt_pci_read_config(pci_dev, config_addr, len);
> +
> +    if ( igd_passthru )
> +    {
> +        switch ( config_addr )
> +        {
> +            case 0xfc: /* OpRegion */
> +                if ( igd_guest_opregion )
> +                    val = igd_guest_opregion;
> +                break;
> +        }
> +    }
> +#ifdef PT_DEBUG_PCI_CONFIG_ACCESS
> +    PT_LOG_DEV(pci_dev, "vgapt_pci_read_config: %x:%x.%x: addr=%x
len=%x val=%x\n",
> +            config_addr, len, val);
> +#endif
> +    return val;
> +}
> +
>  void igd_pci_write(PCIDevice *pci_dev, uint32_t config_addr, uint32_t val,
int len)
>  {
>      struct pci_dev *pci_dev_host_bridge = pt_pci_get_dev(0, 0, 0);
> @@ -99,7 +172,6 @@ uint32_t igd_pci_read(PCIDevice *pci_dev, uint32_t
config_addr, int len)
>  int register_vga_regions(struct pt_dev *real_device)
>  {
>      u16 vendor_id;
> -    int igd_opregion;
>      int ret = 0;
>  
>      if ( !gfx_passthru || real_device->pci_dev->device_class !=
0x0300 )
> @@ -117,19 +189,6 @@ int register_vga_regions(struct pt_dev *real_device)
>              0x20,
>              DPCI_ADD_MAPPING);
>  
> -    /* 1:1 map ASL Storage register value */
> -    vendor_id = pt_pci_host_read(real_device->pci_dev, 0, 2);
> -    igd_opregion = pt_pci_host_read(real_device->pci_dev,
PCI_INTEL_OPREGION, 4);
> -    if ( (vendor_id == PCI_VENDOR_ID_INTEL ) && igd_opregion )
> -    {
> -        ret |= xc_domain_memory_mapping(xc_handle, domid,
> -                igd_opregion >> XC_PAGE_SHIFT,
> -                igd_opregion >> XC_PAGE_SHIFT,
> -                2,
> -                DPCI_ADD_MAPPING);
> -        PT_LOG("register_vga: igd_opregion = %x\n",
igd_opregion);
> -    }
> -
>      if ( ret != 0 )
>          PT_LOG("VGA region mapping failed\n");
>  
> @@ -141,7 +200,7 @@ int register_vga_regions(struct pt_dev *real_device)
>   */
>  int unregister_vga_regions(struct pt_dev *real_device)
>  {
> -    u32 vendor_id, igd_opregion;
> +    u32 vendor_id;
>      int ret = 0;
>  
>      if ( !gfx_passthru || real_device->pci_dev->device_class !=
0x0300 )
> @@ -160,12 +219,11 @@ int unregister_vga_regions(struct pt_dev
*real_device)
>              DPCI_REMOVE_MAPPING);
>  
>      vendor_id = pt_pci_host_read(real_device->pci_dev, PCI_VENDOR_ID,
2);
> -    igd_opregion = pt_pci_host_read(real_device->pci_dev,
PCI_INTEL_OPREGION, 4);
> -    if ( (vendor_id == PCI_VENDOR_ID_INTEL) && igd_opregion )
> +    if ( (vendor_id == PCI_VENDOR_ID_INTEL) && igd_guest_opregion
)
>      {
>          ret |= xc_domain_memory_mapping(xc_handle, domid,
> -                igd_opregion >> XC_PAGE_SHIFT,
> -                igd_opregion >> XC_PAGE_SHIFT,
> +                igd_guest_opregion >> XC_PAGE_SHIFT,
> +                igd_guest_opregion >> XC_PAGE_SHIFT,
>                  2,
>                  DPCI_REMOVE_MAPPING);
>      }
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

On 12 December 2011 16:32, Konrad Rzeszutek Wilk <konrad@darnok.org>
wrote:
> On Sun, Nov 27, 2011 at 04:23:26PM +0000, 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@eu.citrix.com>
>> ---
>>  hw/pass-through.c |    8 ++--
>>  hw/pass-through.h |    4 ++
>>  hw/pt-graphics.c  |   96
++++++++++++++++++++++++++++++++++++++++++----------
>>  3 files changed, 85 insertions(+), 23 deletions(-)
>>
>
>> diff --git a/hw/pass-through.c b/hw/pass-through.c
>> index 919937f..976d28f 100644
>> --- a/hw/pass-through.c
>> +++ b/hw/pass-through.c
>> @@ -1455,8 +1455,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;
>> @@ -1637,7 +1636,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;
>> @@ -4191,7 +4190,8 @@ static struct pt_dev *
register_real_device(PCIBus *e_bus,
>>      /* Register device */
>>      assigned_device = (struct pt_dev *) pci_register_device(e_bus,
e_dev_name,
>>                                  sizeof(struct pt_dev), e_devfn,
>> -                                pt_pci_read_config,
pt_pci_write_config);
>> +                                gfx_passthru ? vgapt_pci_read_config :
pt_pci_read_config,
>> +                                gfx_passthru ? vgapt_pci_write_config
: pt_pci_write_config);
>>      if ( assigned_device == NULL )
>>      {
>>          PT_LOG("Error: couldn''t register real
device\n");
>> diff --git a/hw/pass-through.h b/hw/pass-through.h
>> index 884139c..c898c7c 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);
>> +void vgapt_pci_write_config(PCIDevice *pci_dev, uint32_t config_addr,
uint32_t val, int len);
>> +uint32_t vgapt_pci_read_config(PCIDevice *pci_dev, uint32_t
config_addr, int len);
>> +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..33cbff5 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,77 @@ void intel_pch_init(PCIBus *bus)
>>                          pch_map_irq, "intel_bridge_1f");
>>  }
>>
>> +static void igd_write_opregion(PCIDevice *pci_dev, uint32_t val, int
len)
>> +{
>> +    struct pt_dev *real_dev = (struct pt_dev *)pci_dev;
>> +    uint32_t host_opregion = 0;
>> +    int ret;
>> +
>> +    if ( igd_guest_opregion || len != 4 )
>> +        return;
>> +
>> +    host_opregion = pt_pci_host_read(real_dev->pci_dev,
>> +            PCI_INTEL_OPREGION, len);
>
> The tabs here are a bit weird.
>> +    igd_guest_opregion = (val & ~0xfff) | (host_opregion &
0xfff);
>> +    PT_LOG("Map OpRegion: %x -> %x\n", host_opregion,
igd_guest_opregion);
>> +
>> +    ret = xc_domain_memory_mapping(xc_handle, domid,
>> +            igd_guest_opregion >> XC_PAGE_SHIFT,
>> +            host_opregion >> XC_PAGE_SHIFT,
>> +            2,
>> +            DPCI_ADD_MAPPING);
>> +
>
> Shouldn''t you unmap the older region first?
>
We don''t really need to. This region can''t be remapped.

The trick I used here will only work once. Once the page has been mapped
by hvmloader it will stay where it is.

Jean
>> +    if ( ret != 0 )
>> +    {
>> +        PT_LOG("Error: Can''t map opregion\n");
>> +        igd_guest_opregion = 0;
>> +    }
>> +}
>> +
>> +void vgapt_pci_write_config(PCIDevice *pci_dev, uint32_t config_addr,
uint32_t val, int len)
>> +{
>> +#ifdef PT_DEBUG_PCI_CONFIG_ACCESS
>> +    PT_LOG("vgapt_pci_write_config: %x:%x.%x: addr=%x len=%x
val=%x\n",
>> +            pci_bus_num(pci_dev->bus), PCI_SLOT(pci_dev->devfn),
>> +            PCI_FUNC(pci_dev->devfn), config_addr, len, val);
>> +#endif
>> +
>> +    if ( igd_passthru )
>> +    {
>> +        switch ( config_addr )
>> +        {
>> +            case 0xfc : /* Opregion */
>> +                igd_write_opregion(pci_dev, val, len);
>> +                return;
>
> No default case? I would think the compiler would throw a fit for that.
>
>> +        }
>> +    }
>> +
>> +    pt_pci_write_config(pci_dev, config_addr, val, len);
>> +}
>> +
>> +uint32_t vgapt_pci_read_config(PCIDevice *pci_dev, uint32_t
config_addr, int len)
>> +{
>> +    uint32_t val;
>> +
>> +    val = pt_pci_read_config(pci_dev, config_addr, len);
>> +
>> +    if ( igd_passthru )
>> +    {
>> +        switch ( config_addr )
>> +        {
>> +            case 0xfc: /* OpRegion */
>> +                if ( igd_guest_opregion )
>> +                    val = igd_guest_opregion;
>> +                break;
>> +        }
>> +    }
>> +#ifdef PT_DEBUG_PCI_CONFIG_ACCESS
>> +    PT_LOG_DEV(pci_dev, "vgapt_pci_read_config: %x:%x.%x: addr=%x
len=%x val=%x\n",
>> +            config_addr, len, val);
>> +#endif
>> +    return val;
>> +}
>> +
>>  void igd_pci_write(PCIDevice *pci_dev, uint32_t config_addr, uint32_t
val, int len)
>>  {
>>      struct pci_dev *pci_dev_host_bridge = pt_pci_get_dev(0, 0, 0);
>> @@ -99,7 +172,6 @@ uint32_t igd_pci_read(PCIDevice *pci_dev, uint32_t
config_addr, int len)
>>  int register_vga_regions(struct pt_dev *real_device)
>>  {
>>      u16 vendor_id;
>> -    int igd_opregion;
>>      int ret = 0;
>>
>>      if ( !gfx_passthru || real_device->pci_dev->device_class !=
0x0300 )
>> @@ -117,19 +189,6 @@ int register_vga_regions(struct pt_dev
*real_device)
>>              0x20,
>>              DPCI_ADD_MAPPING);
>>
>> -    /* 1:1 map ASL Storage register value */
>> -    vendor_id = pt_pci_host_read(real_device->pci_dev, 0, 2);
>> -    igd_opregion = pt_pci_host_read(real_device->pci_dev,
PCI_INTEL_OPREGION, 4);
>> -    if ( (vendor_id == PCI_VENDOR_ID_INTEL ) && igd_opregion )
>> -    {
>> -        ret |= xc_domain_memory_mapping(xc_handle, domid,
>> -                igd_opregion >> XC_PAGE_SHIFT,
>> -                igd_opregion >> XC_PAGE_SHIFT,
>> -                2,
>> -                DPCI_ADD_MAPPING);
>> -        PT_LOG("register_vga: igd_opregion = %x\n",
igd_opregion);
>> -    }
>> -
>>      if ( ret != 0 )
>>          PT_LOG("VGA region mapping failed\n");
>>
>> @@ -141,7 +200,7 @@ int register_vga_regions(struct pt_dev
*real_device)
>>   */
>>  int unregister_vga_regions(struct pt_dev *real_device)
>>  {
>> -    u32 vendor_id, igd_opregion;
>> +    u32 vendor_id;
>>      int ret = 0;
>>
>>      if ( !gfx_passthru || real_device->pci_dev->device_class !=
0x0300 )
>> @@ -160,12 +219,11 @@ int unregister_vga_regions(struct pt_dev
*real_device)
>>              DPCI_REMOVE_MAPPING);
>>
>>      vendor_id = pt_pci_host_read(real_device->pci_dev,
PCI_VENDOR_ID, 2);
>> -    igd_opregion = pt_pci_host_read(real_device->pci_dev,
PCI_INTEL_OPREGION, 4);
>> -    if ( (vendor_id == PCI_VENDOR_ID_INTEL) && igd_opregion )
>> +    if ( (vendor_id == PCI_VENDOR_ID_INTEL) &&
igd_guest_opregion )
>>      {
>>          ret |= xc_domain_memory_mapping(xc_handle, domid,
>> -                igd_opregion >> XC_PAGE_SHIFT,
>> -                igd_opregion >> XC_PAGE_SHIFT,
>> +                igd_guest_opregion >> XC_PAGE_SHIFT,
>> +                igd_guest_opregion >> XC_PAGE_SHIFT,
>>                  2,
>>                  DPCI_REMOVE_MAPPING);
>>      }
>