[v4,4/5] xen, gfx passthrough: create host bridge to passthrough

Implement that pci host bridge to specific to passthrough. Actually
this just inherit the standard one.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
v4:

* Fix one typo in the patch head description.
* Use (xen_enabled() && xen_has_gfx_passthru) to make sure we only work
  in this scenario.

v3:

* Just fix this patch head description typo.

v2:

* New patch.

 hw/pci-host/piix.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 54 insertions(+), 2 deletions(-)

On Fri, May 30, 2014 at 04:59:28PM +0800, Tiejun Chen wrote:
> Implement that pci host bridge to specific to passthrough. Actually
> this just inherit the standard one.
> 
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
> v4:
> 
> * Fix one typo in the patch head description.
> * Use (xen_enabled() && xen_has_gfx_passthru) to make sure we only work
>   in this scenario.
> 
> v3:
> 
> * Just fix this patch head description typo.
> 
> v2:
> 
> * New patch.
> 
>  hw/pci-host/piix.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 54 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index ffdc853..52382f4 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -34,6 +34,7 @@
>  #include "sysemu/sysemu.h"
>  #include "hw/i386/ioapic.h"
>  #include "qapi/visitor.h"
> +#include "hw/xen/xen_pt.h"
>  
>  /*
>   * I440FX chipset data sheet.
> @@ -95,6 +96,10 @@ typedef struct PIIX3State {
>  #define I440FX_PCI_DEVICE(obj) \
>      OBJECT_CHECK(PCII440FXState, (obj), TYPE_I440FX_PCI_DEVICE)
>  
> +#define TYPE_I440FX_XEN_PCI_DEVICE "i440FX-xen"
> +#define I440FX_XEN_PCI_DEVICE(obj) \
> +    OBJECT_CHECK(PCII440FXState, (obj), TYPE_I440FX_XEN_PCI_DEVICE)
> +
>  struct PCII440FXState {
>      /*< private >*/
>      PCIDevice parent_obj;
> @@ -305,6 +310,16 @@ static int i440fx_initfn(PCIDevice *dev)
>      return 0;
>  }
>  
> +static int i440fx_xen_initfn(PCIDevice *dev)
> +{
> +    PCII440FXState *d = I440FX_XEN_PCI_DEVICE(dev);
> +
> +    dev->config[I440FX_SMRAM] = 0x02;
> +
> +    cpu_smm_register(&i440fx_set_smm, d);
> +    return 0;
> +}
> +
>  PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
>                      int *piix3_devfn,
>                      ISABus **isa_bus, qemu_irq *pic,
> @@ -333,8 +348,15 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
>      object_property_add_child(qdev_get_machine(), "i440fx", OBJECT(dev), NULL);
>      qdev_init_nofail(dev);
>  
> -    d = pci_create_simple(b, 0, TYPE_I440FX_PCI_DEVICE);
> -    *pi440fx_state = I440FX_PCI_DEVICE(d);
> +    if (xen_enabled() && xen_has_gfx_passthru) {
> +        d = pci_create_simple(b, 0, TYPE_I440FX_XEN_PCI_DEVICE);
> +        *pi440fx_state = I440FX_XEN_PCI_DEVICE(d);
> +        pci_create_pch(b);
> +    } else {
> +        d = pci_create_simple(b, 0, TYPE_I440FX_PCI_DEVICE);
> +        *pi440fx_state = I440FX_PCI_DEVICE(d);
> +    }
> +
>      f = *pi440fx_state;
>      f->system_memory = address_space_mem;
>      f->pci_address_space = pci_address_space;
> @@ -705,6 +727,35 @@ static const TypeInfo i440fx_info = {
>      .class_init    = i440fx_class_init,
>  };
>  
> +static void i440fx_xen_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +
> +    k->init = i440fx_xen_initfn;
> +    k->config_write = igd_pci_write;
> +    k->config_read = igd_pci_read;
> +    k->vendor_id = PCI_VENDOR_ID_INTEL;
> +    k->device_id = PCI_DEVICE_ID_INTEL_82441;
> +    k->revision = 0x02;
> +    k->class_id = PCI_CLASS_BRIDGE_ISA;
> +    dc->desc = "XEN Host bridge";
> +    dc->vmsd = &vmstate_i440fx;
> +    /*
> +     * PCI-facing part of the host bridge, not usable without the
> +     * host-facing part, which can't be device_add'ed, yet.
> +     */
> +    dc->cannot_instantiate_with_device_add_yet = true;
> +    dc->hotpluggable   = false;
> +}
> +
> +static const TypeInfo i440fx_xen_info = {
> +    .name          = TYPE_I440FX_XEN_PCI_DEVICE,
> +    .parent        = TYPE_PCI_DEVICE,
> +    .instance_size = sizeof(PCII440FXState),
> +    .class_init    = i440fx_xen_class_init,
> +};
> +
>  static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge,
>                                                  PCIBus *rootbus)
>  {
> @@ -746,6 +797,7 @@ static const TypeInfo i440fx_pcihost_info = {
>  static void i440fx_register_types(void)
>  {
>      type_register_static(&i440fx_info);
> +    type_register_static(&i440fx_xen_info);
>      type_register_static(&piix3_info);
>      type_register_static(&piix3_xen_info);
>      type_register_static(&i440fx_pcihost_info);
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

On Fri, 30 May 2014, Tiejun Chen wrote:
> Implement that pci host bridge to specific to passthrough. Actually
> this just inherit the standard one.
> 
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>

Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


> v4:
> 
> * Fix one typo in the patch head description.
> * Use (xen_enabled() && xen_has_gfx_passthru) to make sure we only work
>   in this scenario.
> 
> v3:
> 
> * Just fix this patch head description typo.
> 
> v2:
> 
> * New patch.
> 
>  hw/pci-host/piix.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 54 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index ffdc853..52382f4 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -34,6 +34,7 @@
>  #include "sysemu/sysemu.h"
>  #include "hw/i386/ioapic.h"
>  #include "qapi/visitor.h"
> +#include "hw/xen/xen_pt.h"
>  
>  /*
>   * I440FX chipset data sheet.
> @@ -95,6 +96,10 @@ typedef struct PIIX3State {
>  #define I440FX_PCI_DEVICE(obj) \
>      OBJECT_CHECK(PCII440FXState, (obj), TYPE_I440FX_PCI_DEVICE)
>  
> +#define TYPE_I440FX_XEN_PCI_DEVICE "i440FX-xen"
> +#define I440FX_XEN_PCI_DEVICE(obj) \
> +    OBJECT_CHECK(PCII440FXState, (obj), TYPE_I440FX_XEN_PCI_DEVICE)
> +
>  struct PCII440FXState {
>      /*< private >*/
>      PCIDevice parent_obj;
> @@ -305,6 +310,16 @@ static int i440fx_initfn(PCIDevice *dev)
>      return 0;
>  }
>  
> +static int i440fx_xen_initfn(PCIDevice *dev)
> +{
> +    PCII440FXState *d = I440FX_XEN_PCI_DEVICE(dev);
> +
> +    dev->config[I440FX_SMRAM] = 0x02;
> +
> +    cpu_smm_register(&i440fx_set_smm, d);
> +    return 0;
> +}
> +
>  PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
>                      int *piix3_devfn,
>                      ISABus **isa_bus, qemu_irq *pic,
> @@ -333,8 +348,15 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
>      object_property_add_child(qdev_get_machine(), "i440fx", OBJECT(dev), NULL);
>      qdev_init_nofail(dev);
>  
> -    d = pci_create_simple(b, 0, TYPE_I440FX_PCI_DEVICE);
> -    *pi440fx_state = I440FX_PCI_DEVICE(d);
> +    if (xen_enabled() && xen_has_gfx_passthru) {
> +        d = pci_create_simple(b, 0, TYPE_I440FX_XEN_PCI_DEVICE);
> +        *pi440fx_state = I440FX_XEN_PCI_DEVICE(d);
> +        pci_create_pch(b);
> +    } else {
> +        d = pci_create_simple(b, 0, TYPE_I440FX_PCI_DEVICE);
> +        *pi440fx_state = I440FX_PCI_DEVICE(d);
> +    }
> +
>      f = *pi440fx_state;
>      f->system_memory = address_space_mem;
>      f->pci_address_space = pci_address_space;
> @@ -705,6 +727,35 @@ static const TypeInfo i440fx_info = {
>      .class_init    = i440fx_class_init,
>  };
>  
> +static void i440fx_xen_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +
> +    k->init = i440fx_xen_initfn;
> +    k->config_write = igd_pci_write;
> +    k->config_read = igd_pci_read;
> +    k->vendor_id = PCI_VENDOR_ID_INTEL;
> +    k->device_id = PCI_DEVICE_ID_INTEL_82441;
> +    k->revision = 0x02;
> +    k->class_id = PCI_CLASS_BRIDGE_ISA;
> +    dc->desc = "XEN Host bridge";
> +    dc->vmsd = &vmstate_i440fx;
> +    /*
> +     * PCI-facing part of the host bridge, not usable without the
> +     * host-facing part, which can't be device_add'ed, yet.
> +     */
> +    dc->cannot_instantiate_with_device_add_yet = true;
> +    dc->hotpluggable   = false;
> +}
> +
> +static const TypeInfo i440fx_xen_info = {
> +    .name          = TYPE_I440FX_XEN_PCI_DEVICE,
> +    .parent        = TYPE_PCI_DEVICE,
> +    .instance_size = sizeof(PCII440FXState),
> +    .class_init    = i440fx_xen_class_init,
> +};
> +
>  static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge,
>                                                  PCIBus *rootbus)
>  {
> @@ -746,6 +797,7 @@ static const TypeInfo i440fx_pcihost_info = {
>  static void i440fx_register_types(void)
>  {
>      type_register_static(&i440fx_info);
> +    type_register_static(&i440fx_xen_info);
>      type_register_static(&piix3_info);
>      type_register_static(&piix3_xen_info);
>      type_register_static(&i440fx_pcihost_info);
> -- 
> 1.9.1
>

On Fri, May 30, 2014 at 04:59:28PM +0800, Tiejun Chen wrote:
> Implement that pci host bridge to specific to passthrough. Actually
> this just inherit the standard one.
> 
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>

Can this actually inherit TYPE_I440FX_PCI_DEVICE?
The parent is TYPE_PCI_DEVICE, but if it was TYPE_I440FX_PCI_DEVICE
then you could re-use i440fx_initfn.
And you could do *pi440fx_state = I440FX_PCI_DEVICE(d)
unconditionally.

> ---
> v4:
> 
> * Fix one typo in the patch head description.
> * Use (xen_enabled() && xen_has_gfx_passthru) to make sure we only work
>   in this scenario.
> 
> v3:
> 
> * Just fix this patch head description typo.
> 
> v2:
> 
> * New patch.
> 
>  hw/pci-host/piix.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 54 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index ffdc853..52382f4 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -34,6 +34,7 @@
>  #include "sysemu/sysemu.h"
>  #include "hw/i386/ioapic.h"
>  #include "qapi/visitor.h"
> +#include "hw/xen/xen_pt.h"
>  
>  /*
>   * I440FX chipset data sheet.
> @@ -95,6 +96,10 @@ typedef struct PIIX3State {
>  #define I440FX_PCI_DEVICE(obj) \
>      OBJECT_CHECK(PCII440FXState, (obj), TYPE_I440FX_PCI_DEVICE)
>  
> +#define TYPE_I440FX_XEN_PCI_DEVICE "i440FX-xen"
> +#define I440FX_XEN_PCI_DEVICE(obj) \
> +    OBJECT_CHECK(PCII440FXState, (obj), TYPE_I440FX_XEN_PCI_DEVICE)
> +
>  struct PCII440FXState {
>      /*< private >*/
>      PCIDevice parent_obj;
> @@ -305,6 +310,16 @@ static int i440fx_initfn(PCIDevice *dev)
>      return 0;
>  }
>  
> +static int i440fx_xen_initfn(PCIDevice *dev)
> +{
> +    PCII440FXState *d = I440FX_XEN_PCI_DEVICE(dev);
> +
> +    dev->config[I440FX_SMRAM] = 0x02;
> +
> +    cpu_smm_register(&i440fx_set_smm, d);
> +    return 0;
> +}
> +
>  PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
>                      int *piix3_devfn,
>                      ISABus **isa_bus, qemu_irq *pic,
> @@ -333,8 +348,15 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
>      object_property_add_child(qdev_get_machine(), "i440fx", OBJECT(dev), NULL);
>      qdev_init_nofail(dev);
>  
> -    d = pci_create_simple(b, 0, TYPE_I440FX_PCI_DEVICE);
> -    *pi440fx_state = I440FX_PCI_DEVICE(d);
> +    if (xen_enabled() && xen_has_gfx_passthru) {
> +        d = pci_create_simple(b, 0, TYPE_I440FX_XEN_PCI_DEVICE);
> +        *pi440fx_state = I440FX_XEN_PCI_DEVICE(d);
> +        pci_create_pch(b);
> +    } else {
> +        d = pci_create_simple(b, 0, TYPE_I440FX_PCI_DEVICE);
> +        *pi440fx_state = I440FX_PCI_DEVICE(d);
> +    }
> +
>      f = *pi440fx_state;
>      f->system_memory = address_space_mem;
>      f->pci_address_space = pci_address_space;
> @@ -705,6 +727,35 @@ static const TypeInfo i440fx_info = {
>      .class_init    = i440fx_class_init,
>  };
>  
> +static void i440fx_xen_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +
> +    k->init = i440fx_xen_initfn;
> +    k->config_write = igd_pci_write;
> +    k->config_read = igd_pci_read;
> +    k->vendor_id = PCI_VENDOR_ID_INTEL;
> +    k->device_id = PCI_DEVICE_ID_INTEL_82441;
> +    k->revision = 0x02;
> +    k->class_id = PCI_CLASS_BRIDGE_ISA;
> +    dc->desc = "XEN Host bridge";
> +    dc->vmsd = &vmstate_i440fx;
> +    /*
> +     * PCI-facing part of the host bridge, not usable without the
> +     * host-facing part, which can't be device_add'ed, yet.
> +     */
> +    dc->cannot_instantiate_with_device_add_yet = true;
> +    dc->hotpluggable   = false;
> +}
> +
> +static const TypeInfo i440fx_xen_info = {
> +    .name          = TYPE_I440FX_XEN_PCI_DEVICE,
> +    .parent        = TYPE_PCI_DEVICE,
> +    .instance_size = sizeof(PCII440FXState),
> +    .class_init    = i440fx_xen_class_init,
> +};
> +
>  static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge,
>                                                  PCIBus *rootbus)
>  {
> @@ -746,6 +797,7 @@ static const TypeInfo i440fx_pcihost_info = {
>  static void i440fx_register_types(void)
>  {
>      type_register_static(&i440fx_info);
> +    type_register_static(&i440fx_xen_info);
>      type_register_static(&piix3_info);
>      type_register_static(&piix3_xen_info);
>      type_register_static(&i440fx_pcihost_info);
> -- 
> 1.9.1

> -----Original Message-----
> From: Michael S. Tsirkin [mailto:mst@redhat.com]
> Sent: Tuesday, June 03, 2014 4:37 AM
> To: Chen, Tiejun
> Cc: anthony.perard@citrix.com; stefano.stabellini@eu.citrix.com;
> Kelly.Zytaruk@amd.com; qemu-devel@nongnu.org;
> xen-devel@lists.xensource.com; peter.maydell@linaro.org;
> anthony@codemonkey.ws; Kay, Allen M; Zhang, Yang Z
> Subject: Re: [v4][PATCH 4/5] xen, gfx passthrough: create host bridge to
> passthrough
> 
> On Fri, May 30, 2014 at 04:59:28PM +0800, Tiejun Chen wrote:
> > Implement that pci host bridge to specific to passthrough. Actually
> > this just inherit the standard one.
> >
> > Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> 
> Can this actually inherit TYPE_I440FX_PCI_DEVICE?

Yes, but

> The parent is TYPE_PCI_DEVICE, but if it was TYPE_I440FX_PCI_DEVICE then
> you could re-use i440fx_initfn.
> And you could do *pi440fx_state = I440FX_PCI_DEVICE(d) unconditionally.
> 

one of reasons to implement our own pci host bridge which just inherit that standard one, please refer to some comments from v1:

http://patchwork.ozlabs.org/patch/322443/

And I also think this separation may be flexible to extend more in the future.  

Thanks
Tiejun