[v9,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>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
v9:

* Move is_igd_vga_passthrough(dev)) from xen_igd_passthrough_isa_bridge_create()
  into xen_pt_initfn().

 hw/xen/xen_pt.c      | 21 +++++++++++++++++++++
 include/hw/xen/xen.h |  1 +
 2 files changed, 22 insertions(+)

On Tue, 7 Jul 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>
> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
> v9:
> 
> * Move is_igd_vga_passthrough(dev)) from xen_igd_passthrough_isa_bridge_create()
>   into xen_pt_initfn().
> 
>  hw/xen/xen_pt.c      | 21 +++++++++++++++++++++
>  include/hw/xen/xen.h |  1 +
>  2 files changed, 22 insertions(+)
> 
> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> index 15b02cb..f764131 100644
> --- a/hw/xen/xen_pt.c
> +++ b/hw/xen/xen_pt.c
> @@ -684,6 +684,17 @@ 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;
> +
> +    gpu_dev_id = dev->device_id;
> +    igd_passthrough_isa_bridge_create(d->bus, gpu_dev_id);
> +}
> +
>  /* init */
>  
>  static int xen_pt_initfn(PCIDevice *d)
> @@ -728,11 +739,21 @@ static int xen_pt_initfn(PCIDevice *d)
>      /* Setup VGA bios for passthrough GFX */
>      if ((s->real_device.domain == 0) && (s->real_device.bus == 0) &&
>          (s->real_device.dev == 2) && (s->real_device.func == 0)) {
> +        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;
> +        }

This is OK now, thanks.


>          if (xen_pt_setup_vga(s, &s->real_device) < 0) {
>              XEN_PT_ERR(d, "Setup VGA BIOS of passthrough GFX failed!\n");
>              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 */

You are right that I was confused on my previous comment, but it is true
that it shouldn't be part of this patch: it should be part of patch
7/10. Also the declaration should probably not be in the xen.h header.

Michael, where do you think should go?

On Mon, Jul 13, 2015 at 05:14:08PM +0100, Stefano Stabellini wrote:
> On Tue, 7 Jul 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>
> > Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > ---
> > v9:
> > 
> > * Move is_igd_vga_passthrough(dev)) from xen_igd_passthrough_isa_bridge_create()
> >   into xen_pt_initfn().
> > 
> >  hw/xen/xen_pt.c      | 21 +++++++++++++++++++++
> >  include/hw/xen/xen.h |  1 +
> >  2 files changed, 22 insertions(+)
> > 
> > diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> > index 15b02cb..f764131 100644
> > --- a/hw/xen/xen_pt.c
> > +++ b/hw/xen/xen_pt.c
> > @@ -684,6 +684,17 @@ 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;
> > +
> > +    gpu_dev_id = dev->device_id;
> > +    igd_passthrough_isa_bridge_create(d->bus, gpu_dev_id);
> > +}
> > +
> >  /* init */
> >  
> >  static int xen_pt_initfn(PCIDevice *d)
> > @@ -728,11 +739,21 @@ static int xen_pt_initfn(PCIDevice *d)
> >      /* Setup VGA bios for passthrough GFX */
> >      if ((s->real_device.domain == 0) && (s->real_device.bus == 0) &&
> >          (s->real_device.dev == 2) && (s->real_device.func == 0)) {
> > +        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;
> > +        }
> 
> This is OK now, thanks.
> 
> 
> >          if (xen_pt_setup_vga(s, &s->real_device) < 0) {
> >              XEN_PT_ERR(d, "Setup VGA BIOS of passthrough GFX failed!\n");
> >              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 */
> 
> You are right that I was confused on my previous comment, but it is true
> that it shouldn't be part of this patch: it should be part of patch
> 7/10. Also the declaration should probably not be in the xen.h header.
> 
> Michael, where do you think should go?

Donnu - add a header?

>>> 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 */
>>
>> You are right that I was confused on my previous comment, but it is true
>> that it shouldn't be part of this patch: it should be part of patch

Right.

>> 7/10. Also the declaration should probably not be in the xen.h header.
>>
>> Michael, where do you think should go?
>
> Donnu - add a header?
>

What about the include/hw/i386/pc.h file?

Thanks
Tiejun

On Tue, Jul 14, 2015 at 04:42:44PM +0800, Chen, Tiejun wrote:
> >>>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 */
> >>
> >>You are right that I was confused on my previous comment, but it is true
> >>that it shouldn't be part of this patch: it should be part of patch
> 
> Right.
> 
> >>7/10. Also the declaration should probably not be in the xen.h header.
> >>
> >>Michael, where do you think should go?
> >
> >Donnu - add a header?
> >
> 
> What about the include/hw/i386/pc.h file?
> 
> Thanks
> Tiejun

Somewhat ugly, but OK I guess.