[v8,RESEND,08/10] xen, gfx passthrough: register a isa bridge

Currently we just register this isa bridge when we use IGD
passthrough in Xen side.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 hw/xen/xen_pt.c      | 18 ++++++++++++++++++
 include/hw/xen/xen.h |  1 +
 2 files changed, 19 insertions(+)

On Fri, 5 Jun 2015, Tiejun Chen wrote:
> Currently we just register this isa bridge when we use IGD
> passthrough in Xen side.
> 
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> ---
>  hw/xen/xen_pt.c      | 18 ++++++++++++++++++
>  include/hw/xen/xen.h |  1 +
>  2 files changed, 19 insertions(+)
> 
> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> index 3bf2233..fff169d 100644
> --- a/hw/xen/xen_pt.c
> +++ b/hw/xen/xen_pt.c
> @@ -676,6 +676,21 @@ static const MemoryListener xen_pt_io_listener = {
>      .priority = 10,
>  };
>  
> +static void
> +xen_igd_passthrough_isa_bridge_create(XenPCIPassthroughState *s,
> +                                      XenHostPCIDevice *dev)
> +{
> +    uint16_t gpu_dev_id;
> +    PCIDevice *d = &s->dev;
> +
> +    if (!is_igd_vga_passthrough(dev)) {
> +        return;
> +    }

I would rather move the if into xen_pt_initfn, otherwise reading
xen_pt_initfn it looks like we are going to create an isa bridge
regardless.


> +    gpu_dev_id = dev->device_id;
> +    igd_passthrough_isa_bridge_create(d->bus, gpu_dev_id);
> +}
> +
>  /* init */
>  
>  static int xen_pt_initfn(PCIDevice *d)
> @@ -725,6 +740,9 @@ static int xen_pt_initfn(PCIDevice *d)
>              xen_host_pci_device_put(&s->real_device);
>              return -1;
>          }
> +
> +        /* Register ISA bridge for passthrough GFX. */
> +        xen_igd_passthrough_isa_bridge_create(s, &s->real_device);
>      }
>  
>      /* Handle real device's MMIO/PIO BARs */
> diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
> index 4356af4..703148e 100644
> --- a/include/hw/xen/xen.h
> +++ b/include/hw/xen/xen.h
> @@ -51,4 +51,5 @@ void xen_register_framebuffer(struct MemoryRegion *mr);
>  #  define HVM_MAX_VCPUS 32
>  #endif
>  
> +extern void igd_passthrough_isa_bridge_create(PCIBus *bus, uint16_t gpu_dev_id);
>  #endif /* QEMU_HW_XEN_H */

Either static or extern. You probably want to drop this declaration.

>> +static void
>> +xen_igd_passthrough_isa_bridge_create(XenPCIPassthroughState *s,
>> +                                      XenHostPCIDevice *dev)
>> +{
>> +    uint16_t gpu_dev_id;
>> +    PCIDevice *d = &s->dev;
>> +
>> +    if (!is_igd_vga_passthrough(dev)) {
>> +        return;
>> +    }
>
> I would rather move the if into xen_pt_initfn, otherwise reading
> xen_pt_initfn it looks like we are going to create an isa bridge
> regardless.

This makes sense.

>
>
>> +    gpu_dev_id = dev->device_id;
>> +    igd_passthrough_isa_bridge_create(d->bus, gpu_dev_id);
>> +}
>> +
>>   /* init */
>>
>>   static int xen_pt_initfn(PCIDevice *d)
>> @@ -725,6 +740,9 @@ static int xen_pt_initfn(PCIDevice *d)

I'd like to add something like,

if (!is_igd_vga_passthrough(&s->real_device)) {
     XEN_PT_ERR(d, "Need to enable igd-passthru if you're trying"
	       " to passthrough IGD GFX.\n");
     xen_host_pci_device_put(&s->real_device); 

     return -1; 

         }

>>               xen_host_pci_device_put(&s->real_device);
>>               return -1;
>>           }
>> +
>> +        /* Register ISA bridge for passthrough GFX. */
>> +        xen_igd_passthrough_isa_bridge_create(s, &s->real_device);
>>       }
>>
>>       /* Handle real device's MMIO/PIO BARs */
>> diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
>> index 4356af4..703148e 100644
>> --- a/include/hw/xen/xen.h
>> +++ b/include/hw/xen/xen.h
>> @@ -51,4 +51,5 @@ void xen_register_framebuffer(struct MemoryRegion *mr);
>>   #  define HVM_MAX_VCPUS 32
>>   #endif
>>
>> +extern void igd_passthrough_isa_bridge_create(PCIBus *bus, uint16_t gpu_dev_id);
>>   #endif /* QEMU_HW_XEN_H */
>
> Either static or extern. You probably want to drop this declaration.
>

I just guess you're confused between igd_passthrough_isa_bridge_create() 
and xen_igd_passthrough_isa_bridge_create(), or am I wrong?

Note igd_passthrough_isa_bridge_create() is defined in the pc_piix.c file.

Thanks
Tiejun