[Qemu-devel] [v2][PATCH 0/5] xen: introduce new machine for IGD passthrough

[Qemu-devel] [v2][PATCH 0/5] xen: introduce new machine for IGD passthrough

[Tiejun Chen]

v2:

* Fix some coding style
* New patch to separate i440fx_init
* Just add prefix with XEN_IGD_PASSTHROUGH/xen_igd_passthrough
* Based on patch #2 to regenerate
* Unify prefix with XEN_IGD_PASSTHROUGH/xen_igd_passthrough like patch #3
* Test: boot with a preinstalled ubuntu 14.04
  ./i386-softmmu/qemu-system-i386 -hda test.img -m 2560 -boot c -machine pc

As we discussed we need to create a separate machine to support current
IGD passthrough.

----------------------------------------------------------------
Tiejun Chen (5):
      hw:i386:pc_piix: split pc_init1()
      hw:pci-host:piix: split i440fx_init
      xen:hw:pci-host:piix: create host bridge to passthrough
      xen:hw:pci-host:piix: introduce xen_igd_passthrough_i440fx_init
      xen:hw:i386:pc_piix: introduce new machine for IGD passthrough

 hw/i386/pc_piix.c    | 222 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------
 hw/pci-host/piix.c   | 165 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------
 include/hw/i386/pc.h |  11 +++++++++
 3 files changed, 358 insertions(+), 40 deletions(-)

Thanks
Tiejun

[v2][PATCH 0/5] xen: introduce new machine for IGD passthrough

[Tiejun Chen]

v2:

* Fix some coding style
* New patch to separate i440fx_init
* Just add prefix with XEN_IGD_PASSTHROUGH/xen_igd_passthrough
* Based on patch #2 to regenerate
* Unify prefix with XEN_IGD_PASSTHROUGH/xen_igd_passthrough like patch #3
* Test: boot with a preinstalled ubuntu 14.04
  ./i386-softmmu/qemu-system-i386 -hda test.img -m 2560 -boot c -machine pc

As we discussed we need to create a separate machine to support current
IGD passthrough.

----------------------------------------------------------------
Tiejun Chen (5):
      hw:i386:pc_piix: split pc_init1()
      hw:pci-host:piix: split i440fx_init
      xen:hw:pci-host:piix: create host bridge to passthrough
      xen:hw:pci-host:piix: introduce xen_igd_passthrough_i440fx_init
      xen:hw:i386:pc_piix: introduce new machine for IGD passthrough

 hw/i386/pc_piix.c    | 222 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------
 hw/pci-host/piix.c   | 165 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------
 include/hw/i386/pc.h |  11 +++++++++
 3 files changed, 358 insertions(+), 40 deletions(-)

Thanks
Tiejun

[Qemu-devel] [v2][PATCH 1/5] hw:i386:pc_piix: split pc_init1()

[Tiejun Chen]

We'd like to split pc_init1 and then we can share something
with other stuff.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 hw/i386/pc_piix.c | 117 +++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 93 insertions(+), 24 deletions(-)

v2:

* Fix some coding style

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 9694f88..ab380d5 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -71,36 +71,29 @@ static bool smbios_legacy_mode;
 static bool gigabyte_align = true;
 static bool has_reserved_memory = true;
 
-/* PC hardware initialisation */
-static void pc_init1(MachineState *machine,
-                     int pci_enabled,
-                     int kvmclock_enabled)
+static ram_addr_t below_4g_mem_size;
+static ram_addr_t above_4g_mem_size;
+static void pc_machine_base_init(MachineState *machine,
+                                 int pci_enabled,
+                                 int kvmclock_enabled,
+                                 DeviceState **pc_icc_bridge,
+                                 MemoryRegion **pc_ram_memory,
+                                 MemoryRegion **pc_pci_memory,
+                                 qemu_irq **pc_gsi,
+                                 GSIState **pc_gsi_state,
+                                 FWCfgState **pc_fw_cfg)
 {
     PCMachineState *pc_machine = PC_MACHINE(machine);
     MemoryRegion *system_memory = get_system_memory();
-    MemoryRegion *system_io = get_system_io();
-    int i;
-    ram_addr_t below_4g_mem_size, above_4g_mem_size;
-    PCIBus *pci_bus;
-    ISABus *isa_bus;
-    PCII440FXState *i440fx_state;
-    int piix3_devfn = -1;
-    qemu_irq *cpu_irq;
-    qemu_irq *gsi;
-    qemu_irq *i8259;
-    qemu_irq *smi_irq;
-    GSIState *gsi_state;
-    DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
-    BusState *idebus[MAX_IDE_BUS];
-    ISADevice *rtc_state;
-    ISADevice *floppy;
-    MemoryRegion *ram_memory;
-    MemoryRegion *pci_memory;
     MemoryRegion *rom_memory;
-    DeviceState *icc_bridge;
-    FWCfgState *fw_cfg = NULL;
     PcGuestInfo *guest_info;
     ram_addr_t lowmem;
+    DeviceState *icc_bridge;
+    MemoryRegion *ram_memory;
+    MemoryRegion *pci_memory;
+    qemu_irq *gsi;
+    GSIState *gsi_state;
+    FWCfgState *fw_cfg;
 
     /* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory).
      * If it doesn't, we need to split it in chunks below and above 4G.
@@ -193,6 +186,31 @@ static void pc_init1(MachineState *machine,
         gsi = qemu_allocate_irqs(gsi_handler, gsi_state, GSI_NUM_PINS);
     }
 
+    *pc_icc_bridge = icc_bridge;
+    *pc_ram_memory = ram_memory;
+    *pc_pci_memory = pci_memory;
+    *pc_gsi = gsi;
+    *pc_gsi_state = gsi_state;
+    *pc_fw_cfg = fw_cfg;
+}
+
+static void pc_machine_pci_bus_init(MachineState *machine,
+                                    int pci_enabled,
+                                    PCII440FXState **pc_i440fx_state,
+                                    int *pc_piix3_devfn,
+                                    PCIBus **pc_pci_bus,
+                                    ISABus **pc_isa_bus,
+                                    qemu_irq *gsi,
+                                    MemoryRegion *pci_memory,
+                                    MemoryRegion *ram_memory)
+{
+    MemoryRegion *system_memory = get_system_memory();
+    MemoryRegion *system_io = get_system_io();
+    PCII440FXState *i440fx_state;
+    int piix3_devfn;
+    PCIBus *pci_bus;
+    ISABus *isa_bus;
+
     if (pci_enabled) {
         pci_bus = i440fx_init(&i440fx_state, &piix3_devfn, &isa_bus, gsi,
                               system_memory, system_io, machine->ram_size,
@@ -205,6 +223,33 @@ static void pc_init1(MachineState *machine,
         isa_bus = isa_bus_new(NULL, system_io);
         no_hpet = 1;
     }
+
+    *pc_i440fx_state = i440fx_state;
+    *pc_piix3_devfn = piix3_devfn;
+    *pc_pci_bus = pci_bus;
+    *pc_isa_bus = isa_bus;
+}
+
+static void pc_machine_device_init(MachineState *machine,
+                                   int pci_enabled,
+                                   GSIState *gsi_state,
+                                   DeviceState *icc_bridge,
+                                   int piix3_devfn,
+                                   FWCfgState *fw_cfg,
+                                   PCIBus *pci_bus,
+                                   ISABus *isa_bus,
+                                   qemu_irq *gsi)
+{
+    int i;
+    DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
+    BusState *idebus[MAX_IDE_BUS];
+    qemu_irq *smi_irq;
+    PCMachineState *pc_machine = PC_MACHINE(machine);
+    qemu_irq *cpu_irq;
+    qemu_irq *i8259;
+    ISADevice *rtc_state;
+    ISADevice *floppy;
+
     isa_bus_irqs(isa_bus, gsi);
 
     if (kvm_irqchip_in_kernel()) {
@@ -292,6 +337,30 @@ static void pc_init1(MachineState *machine,
     }
 }
 
+/* PC hardware initialisation */
+static void pc_init1(MachineState *machine,
+                     int pci_enabled,
+                     int kvmclock_enabled)
+{
+    PCIBus *pci_bus = NULL;
+    ISABus *isa_bus = NULL;
+    PCII440FXState *i440fx_state = NULL;
+    int piix3_devfn = -1;
+    qemu_irq *gsi = NULL;
+    GSIState *gsi_state = NULL;
+    MemoryRegion *ram_memory = NULL;
+    MemoryRegion *pci_memory = NULL;
+    DeviceState *icc_bridge = NULL;
+    FWCfgState *fw_cfg = NULL;
+
+    pc_machine_base_init(machine, pci_enabled, kvmclock_enabled, &icc_bridge,
+                         &ram_memory, &pci_memory, &gsi, &gsi_state, &fw_cfg);
+    pc_machine_pci_bus_init(machine, pci_enabled, &i440fx_state, &piix3_devfn,
+                            &pci_bus, &isa_bus, gsi, pci_memory, ram_memory);
+    pc_machine_device_init(machine, pci_enabled, gsi_state, icc_bridge,
+                           piix3_devfn, fw_cfg, pci_bus, isa_bus, gsi);
+}
+
 static void pc_init_pci(MachineState *machine)
 {
     pc_init1(machine, 1, 1);
-- 
1.9.1

[v2][PATCH 1/5] hw:i386:pc_piix: split pc_init1()

[Tiejun Chen]

We'd like to split pc_init1 and then we can share something
with other stuff.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 hw/i386/pc_piix.c | 117 +++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 93 insertions(+), 24 deletions(-)

v2:

* Fix some coding style

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 9694f88..ab380d5 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -71,36 +71,29 @@ static bool smbios_legacy_mode;
 static bool gigabyte_align = true;
 static bool has_reserved_memory = true;
 
-/* PC hardware initialisation */
-static void pc_init1(MachineState *machine,
-                     int pci_enabled,
-                     int kvmclock_enabled)
+static ram_addr_t below_4g_mem_size;
+static ram_addr_t above_4g_mem_size;
+static void pc_machine_base_init(MachineState *machine,
+                                 int pci_enabled,
+                                 int kvmclock_enabled,
+                                 DeviceState **pc_icc_bridge,
+                                 MemoryRegion **pc_ram_memory,
+                                 MemoryRegion **pc_pci_memory,
+                                 qemu_irq **pc_gsi,
+                                 GSIState **pc_gsi_state,
+                                 FWCfgState **pc_fw_cfg)
 {
     PCMachineState *pc_machine = PC_MACHINE(machine);
     MemoryRegion *system_memory = get_system_memory();
-    MemoryRegion *system_io = get_system_io();
-    int i;
-    ram_addr_t below_4g_mem_size, above_4g_mem_size;
-    PCIBus *pci_bus;
-    ISABus *isa_bus;
-    PCII440FXState *i440fx_state;
-    int piix3_devfn = -1;
-    qemu_irq *cpu_irq;
-    qemu_irq *gsi;
-    qemu_irq *i8259;
-    qemu_irq *smi_irq;
-    GSIState *gsi_state;
-    DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
-    BusState *idebus[MAX_IDE_BUS];
-    ISADevice *rtc_state;
-    ISADevice *floppy;
-    MemoryRegion *ram_memory;
-    MemoryRegion *pci_memory;
     MemoryRegion *rom_memory;
-    DeviceState *icc_bridge;
-    FWCfgState *fw_cfg = NULL;
     PcGuestInfo *guest_info;
     ram_addr_t lowmem;
+    DeviceState *icc_bridge;
+    MemoryRegion *ram_memory;
+    MemoryRegion *pci_memory;
+    qemu_irq *gsi;
+    GSIState *gsi_state;
+    FWCfgState *fw_cfg;
 
     /* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory).
      * If it doesn't, we need to split it in chunks below and above 4G.
@@ -193,6 +186,31 @@ static void pc_init1(MachineState *machine,
         gsi = qemu_allocate_irqs(gsi_handler, gsi_state, GSI_NUM_PINS);
     }
 
+    *pc_icc_bridge = icc_bridge;
+    *pc_ram_memory = ram_memory;
+    *pc_pci_memory = pci_memory;
+    *pc_gsi = gsi;
+    *pc_gsi_state = gsi_state;
+    *pc_fw_cfg = fw_cfg;
+}
+
+static void pc_machine_pci_bus_init(MachineState *machine,
+                                    int pci_enabled,
+                                    PCII440FXState **pc_i440fx_state,
+                                    int *pc_piix3_devfn,
+                                    PCIBus **pc_pci_bus,
+                                    ISABus **pc_isa_bus,
+                                    qemu_irq *gsi,
+                                    MemoryRegion *pci_memory,
+                                    MemoryRegion *ram_memory)
+{
+    MemoryRegion *system_memory = get_system_memory();
+    MemoryRegion *system_io = get_system_io();
+    PCII440FXState *i440fx_state;
+    int piix3_devfn;
+    PCIBus *pci_bus;
+    ISABus *isa_bus;
+
     if (pci_enabled) {
         pci_bus = i440fx_init(&i440fx_state, &piix3_devfn, &isa_bus, gsi,
                               system_memory, system_io, machine->ram_size,
@@ -205,6 +223,33 @@ static void pc_init1(MachineState *machine,
         isa_bus = isa_bus_new(NULL, system_io);
         no_hpet = 1;
     }
+
+    *pc_i440fx_state = i440fx_state;
+    *pc_piix3_devfn = piix3_devfn;
+    *pc_pci_bus = pci_bus;
+    *pc_isa_bus = isa_bus;
+}
+
+static void pc_machine_device_init(MachineState *machine,
+                                   int pci_enabled,
+                                   GSIState *gsi_state,
+                                   DeviceState *icc_bridge,
+                                   int piix3_devfn,
+                                   FWCfgState *fw_cfg,
+                                   PCIBus *pci_bus,
+                                   ISABus *isa_bus,
+                                   qemu_irq *gsi)
+{
+    int i;
+    DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
+    BusState *idebus[MAX_IDE_BUS];
+    qemu_irq *smi_irq;
+    PCMachineState *pc_machine = PC_MACHINE(machine);
+    qemu_irq *cpu_irq;
+    qemu_irq *i8259;
+    ISADevice *rtc_state;
+    ISADevice *floppy;
+
     isa_bus_irqs(isa_bus, gsi);
 
     if (kvm_irqchip_in_kernel()) {
@@ -292,6 +337,30 @@ static void pc_init1(MachineState *machine,
     }
 }
 
+/* PC hardware initialisation */
+static void pc_init1(MachineState *machine,
+                     int pci_enabled,
+                     int kvmclock_enabled)
+{
+    PCIBus *pci_bus = NULL;
+    ISABus *isa_bus = NULL;
+    PCII440FXState *i440fx_state = NULL;
+    int piix3_devfn = -1;
+    qemu_irq *gsi = NULL;
+    GSIState *gsi_state = NULL;
+    MemoryRegion *ram_memory = NULL;
+    MemoryRegion *pci_memory = NULL;
+    DeviceState *icc_bridge = NULL;
+    FWCfgState *fw_cfg = NULL;
+
+    pc_machine_base_init(machine, pci_enabled, kvmclock_enabled, &icc_bridge,
+                         &ram_memory, &pci_memory, &gsi, &gsi_state, &fw_cfg);
+    pc_machine_pci_bus_init(machine, pci_enabled, &i440fx_state, &piix3_devfn,
+                            &pci_bus, &isa_bus, gsi, pci_memory, ram_memory);
+    pc_machine_device_init(machine, pci_enabled, gsi_state, icc_bridge,
+                           piix3_devfn, fw_cfg, pci_bus, isa_bus, gsi);
+}
+
 static void pc_init_pci(MachineState *machine)
 {
     pc_init1(machine, 1, 1);
-- 
1.9.1

[Qemu-devel] [v2][PATCH 2/5] hw:pci-host:piix: split i440fx_init

[Tiejun Chen]

We'd like to split i440fx_init and then we can share something
with other stuff.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 hw/pci-host/piix.c | 91 ++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 75 insertions(+), 16 deletions(-)

v2:

* New patch to separate i440fx_init

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index e0e0946..a9a5570 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -305,25 +305,14 @@ static int i440fx_initfn(PCIDevice *dev)
     return 0;
 }
 
-PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
-                    int *piix3_devfn,
-                    ISABus **isa_bus, qemu_irq *pic,
-                    MemoryRegion *address_space_mem,
-                    MemoryRegion *address_space_io,
-                    ram_addr_t ram_size,
-                    ram_addr_t below_4g_mem_size,
-                    ram_addr_t above_4g_mem_size,
-                    MemoryRegion *pci_address_space,
-                    MemoryRegion *ram_memory)
+static void i440fx_pci_host_create(DeviceState **dev_bridge,
+                                   PCIBus **bus_bridge,
+                                   MemoryRegion *address_space_io,
+                                   MemoryRegion *pci_address_space)
 {
     DeviceState *dev;
     PCIBus *b;
-    PCIDevice *d;
     PCIHostState *s;
-    PIIX3State *piix3;
-    PCII440FXState *f;
-    unsigned i;
-    I440FXState *i440fx;
 
     dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST_BRIDGE);
     s = PCI_HOST_BRIDGE(dev);
@@ -333,8 +322,48 @@ 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);
+    *bus_bridge = b;
+    *dev_bridge = dev;
+}
+
+static PCIDevice *i440fx_pci0_create(PCIBus **bus_bridge,
+                                     const char *type,
+                                     PCII440FXState **pi440fx_state)
+{
+    PCIBus *b;
+    PCIDevice *d;
+
+    b = *bus_bridge;
+
+    d = pci_create_simple(b, 0, type);
     *pi440fx_state = I440FX_PCI_DEVICE(d);
+    return d;
+}
+
+static PCIBus *i440fx_pci_setup(DeviceState **dev_bridge,
+                                PCIBus **bus_bridge,
+                                PCIDevice *d,
+                                PCII440FXState **pi440fx_state,
+                                int *piix3_devfn,
+                                ISABus **isa_bus, qemu_irq *pic,
+                                MemoryRegion *address_space_mem,
+                                MemoryRegion *address_space_io,
+                                ram_addr_t ram_size,
+                                ram_addr_t below_4g_mem_size,
+                                ram_addr_t above_4g_mem_size,
+                                MemoryRegion *pci_address_space,
+                                MemoryRegion *ram_memory)
+{
+    DeviceState *dev;
+    PCIBus *b;
+    PCII440FXState *f;
+    I440FXState *i440fx;
+    unsigned i;
+    PIIX3State *piix3;
+
+    b = *bus_bridge;
+    dev = *dev_bridge;
+
     f = *pi440fx_state;
     f->system_memory = address_space_mem;
     f->pci_address_space = pci_address_space;
@@ -392,6 +421,36 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
     return b;
 }
 
+PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
+                    int *piix3_devfn,
+                    ISABus **isa_bus,
+                    qemu_irq *pic,
+                    MemoryRegion *address_space_mem,
+                    MemoryRegion *address_space_io,
+                    ram_addr_t ram_size,
+                    ram_addr_t below_4g_mem_size,
+                    ram_addr_t above_4g_mem_size,
+                    MemoryRegion *pci_address_space,
+                    MemoryRegion *ram_memory)
+{
+    PCIDevice *d;
+    DeviceState *dev_bridge;
+    PCIBus *bus_bridge;
+
+    i440fx_pci_host_create(&dev_bridge, &bus_bridge, address_space_io,
+                           pci_address_space);
+
+    d = i440fx_pci0_create(&bus_bridge, TYPE_I440FX_PCI_DEVICE, pi440fx_state);
+
+    bus_bridge = i440fx_pci_setup(&dev_bridge, &bus_bridge, d, pi440fx_state,
+                                  piix3_devfn, isa_bus, pic, address_space_mem,
+                                  address_space_io, ram_size, below_4g_mem_size,
+                                  above_4g_mem_size, pci_address_space,
+                                  ram_memory);
+
+    return bus_bridge;
+}
+
 PCIBus *find_i440fx(void)
 {
     PCIHostState *s = OBJECT_CHECK(PCIHostState,
-- 
1.9.1

[v2][PATCH 2/5] hw:pci-host:piix: split i440fx_init

[Tiejun Chen]

We'd like to split i440fx_init and then we can share something
with other stuff.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 hw/pci-host/piix.c | 91 ++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 75 insertions(+), 16 deletions(-)

v2:

* New patch to separate i440fx_init

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index e0e0946..a9a5570 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -305,25 +305,14 @@ static int i440fx_initfn(PCIDevice *dev)
     return 0;
 }
 
-PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
-                    int *piix3_devfn,
-                    ISABus **isa_bus, qemu_irq *pic,
-                    MemoryRegion *address_space_mem,
-                    MemoryRegion *address_space_io,
-                    ram_addr_t ram_size,
-                    ram_addr_t below_4g_mem_size,
-                    ram_addr_t above_4g_mem_size,
-                    MemoryRegion *pci_address_space,
-                    MemoryRegion *ram_memory)
+static void i440fx_pci_host_create(DeviceState **dev_bridge,
+                                   PCIBus **bus_bridge,
+                                   MemoryRegion *address_space_io,
+                                   MemoryRegion *pci_address_space)
 {
     DeviceState *dev;
     PCIBus *b;
-    PCIDevice *d;
     PCIHostState *s;
-    PIIX3State *piix3;
-    PCII440FXState *f;
-    unsigned i;
-    I440FXState *i440fx;
 
     dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST_BRIDGE);
     s = PCI_HOST_BRIDGE(dev);
@@ -333,8 +322,48 @@ 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);
+    *bus_bridge = b;
+    *dev_bridge = dev;
+}
+
+static PCIDevice *i440fx_pci0_create(PCIBus **bus_bridge,
+                                     const char *type,
+                                     PCII440FXState **pi440fx_state)
+{
+    PCIBus *b;
+    PCIDevice *d;
+
+    b = *bus_bridge;
+
+    d = pci_create_simple(b, 0, type);
     *pi440fx_state = I440FX_PCI_DEVICE(d);
+    return d;
+}
+
+static PCIBus *i440fx_pci_setup(DeviceState **dev_bridge,
+                                PCIBus **bus_bridge,
+                                PCIDevice *d,
+                                PCII440FXState **pi440fx_state,
+                                int *piix3_devfn,
+                                ISABus **isa_bus, qemu_irq *pic,
+                                MemoryRegion *address_space_mem,
+                                MemoryRegion *address_space_io,
+                                ram_addr_t ram_size,
+                                ram_addr_t below_4g_mem_size,
+                                ram_addr_t above_4g_mem_size,
+                                MemoryRegion *pci_address_space,
+                                MemoryRegion *ram_memory)
+{
+    DeviceState *dev;
+    PCIBus *b;
+    PCII440FXState *f;
+    I440FXState *i440fx;
+    unsigned i;
+    PIIX3State *piix3;
+
+    b = *bus_bridge;
+    dev = *dev_bridge;
+
     f = *pi440fx_state;
     f->system_memory = address_space_mem;
     f->pci_address_space = pci_address_space;
@@ -392,6 +421,36 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
     return b;
 }
 
+PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
+                    int *piix3_devfn,
+                    ISABus **isa_bus,
+                    qemu_irq *pic,
+                    MemoryRegion *address_space_mem,
+                    MemoryRegion *address_space_io,
+                    ram_addr_t ram_size,
+                    ram_addr_t below_4g_mem_size,
+                    ram_addr_t above_4g_mem_size,
+                    MemoryRegion *pci_address_space,
+                    MemoryRegion *ram_memory)
+{
+    PCIDevice *d;
+    DeviceState *dev_bridge;
+    PCIBus *bus_bridge;
+
+    i440fx_pci_host_create(&dev_bridge, &bus_bridge, address_space_io,
+                           pci_address_space);
+
+    d = i440fx_pci0_create(&bus_bridge, TYPE_I440FX_PCI_DEVICE, pi440fx_state);
+
+    bus_bridge = i440fx_pci_setup(&dev_bridge, &bus_bridge, d, pi440fx_state,
+                                  piix3_devfn, isa_bus, pic, address_space_mem,
+                                  address_space_io, ram_size, below_4g_mem_size,
+                                  above_4g_mem_size, pci_address_space,
+                                  ram_memory);
+
+    return bus_bridge;
+}
+
 PCIBus *find_i440fx(void)
 {
     PCIHostState *s = OBJECT_CHECK(PCIHostState,
-- 
1.9.1

Re: [Qemu-devel] [v2][PATCH 2/5] hw:pci-host:piix: split i440fx_init

[Michael S. Tsirkin]

On Thu, Jul 31, 2014 at 02:31:36PM +0800, Tiejun Chen wrote:
> We'd like to split i440fx_init and then we can share something
> with other stuff.
> 
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>

I think this is too much work for very little benefit.
Just pass const char *type to i440fx_init.

-->

i440fx: make type configurable at run-time

Xen wants to supply a different pci host device,
inheriting i440fx. Make this configurable.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

--

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index be8fdfe..72e612b 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -230,7 +230,8 @@ extern int no_hpet;
 struct PCII440FXState;
 typedef struct PCII440FXState PCII440FXState;
 
-PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
+PCIBus *i440fx_init(const char *type,
+                    PCII440FXState **pi440fx_state, int *piix_devfn,
                     ISABus **isa_bus, qemu_irq *pic,
                     MemoryRegion *address_space_mem,
                     MemoryRegion *address_space_io,
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 31125b7..5fefde6 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -194,7 +194,8 @@ static void pc_init1(MachineState *machine,
     }
 
     if (pci_enabled) {
-        pci_bus = i440fx_init(&i440fx_state, &piix3_devfn, &isa_bus, gsi,
+        pci_bus = i440fx_init(TYPE_I440FX_PCI_HOST_BRIDGE,
+                              &i440fx_state, &piix3_devfn, &isa_bus, gsi,
                               system_memory, system_io, machine->ram_size,
                               below_4g_mem_size,
                               above_4g_mem_size,
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index e0e0946..f0c5b5d 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -305,7 +305,8 @@ static int i440fx_initfn(PCIDevice *dev)
     return 0;
 }
 
-PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
+PCIBus *i440fx_init(const char *type,
+                    PCII440FXState **pi440fx_state,
                     int *piix3_devfn,
                     ISABus **isa_bus, qemu_irq *pic,
                     MemoryRegion *address_space_mem,
@@ -325,7 +326,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
     unsigned i;
     I440FXState *i440fx;
 
-    dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST_BRIDGE);
+    dev = qdev_create(NULL, type);
     s = PCI_HOST_BRIDGE(dev);
     b = pci_bus_new(dev, NULL, pci_address_space,
                     address_space_io, 0, TYPE_PCI_BUS);

Re: [v2][PATCH 2/5] hw:pci-host:piix: split i440fx_init

[Michael S. Tsirkin]

On Thu, Jul 31, 2014 at 02:31:36PM +0800, Tiejun Chen wrote:
> We'd like to split i440fx_init and then we can share something
> with other stuff.
> 
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>

I think this is too much work for very little benefit.
Just pass const char *type to i440fx_init.

-->

i440fx: make type configurable at run-time

Xen wants to supply a different pci host device,
inheriting i440fx. Make this configurable.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

--

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index be8fdfe..72e612b 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -230,7 +230,8 @@ extern int no_hpet;
 struct PCII440FXState;
 typedef struct PCII440FXState PCII440FXState;
 
-PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
+PCIBus *i440fx_init(const char *type,
+                    PCII440FXState **pi440fx_state, int *piix_devfn,
                     ISABus **isa_bus, qemu_irq *pic,
                     MemoryRegion *address_space_mem,
                     MemoryRegion *address_space_io,
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 31125b7..5fefde6 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -194,7 +194,8 @@ static void pc_init1(MachineState *machine,
     }
 
     if (pci_enabled) {
-        pci_bus = i440fx_init(&i440fx_state, &piix3_devfn, &isa_bus, gsi,
+        pci_bus = i440fx_init(TYPE_I440FX_PCI_HOST_BRIDGE,
+                              &i440fx_state, &piix3_devfn, &isa_bus, gsi,
                               system_memory, system_io, machine->ram_size,
                               below_4g_mem_size,
                               above_4g_mem_size,
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index e0e0946..f0c5b5d 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -305,7 +305,8 @@ static int i440fx_initfn(PCIDevice *dev)
     return 0;
 }
 
-PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
+PCIBus *i440fx_init(const char *type,
+                    PCII440FXState **pi440fx_state,
                     int *piix3_devfn,
                     ISABus **isa_bus, qemu_irq *pic,
                     MemoryRegion *address_space_mem,
@@ -325,7 +326,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
     unsigned i;
     I440FXState *i440fx;
 
-    dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST_BRIDGE);
+    dev = qdev_create(NULL, type);
     s = PCI_HOST_BRIDGE(dev);
     b = pci_bus_new(dev, NULL, pci_address_space,
                     address_space_io, 0, TYPE_PCI_BUS);

Re: [Qemu-devel] [v2][PATCH 2/5] hw:pci-host:piix: split i440fx_init

[Chen, Tiejun]

On 2014/7/31 17:10, Michael S. Tsirkin wrote:
> On Thu, Jul 31, 2014 at 02:31:36PM +0800, Tiejun Chen wrote:
>> We'd like to split i440fx_init and then we can share something
>> with other stuff.
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>
> I think this is too much work for very little benefit.
> Just pass const char *type to i440fx_init.

You know we will introduce that faked PCIe device represented that PCH 
later, so how to distinguish them? Are you saying I should check the 
type like this?

if(Xen-Type)
{}
else
{}

If so, actually this is mostly same as my original implementation,

"
@@ -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;
"

But as I said to you last time, Paolo doesn't like we walk the common 
codes :)

Tiejun

>
> -->
>
> i440fx: make type configurable at run-time
>
> Xen wants to supply a different pci host device,
> inheriting i440fx. Make this configurable.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> --
>
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index be8fdfe..72e612b 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -230,7 +230,8 @@ extern int no_hpet;
>   struct PCII440FXState;
>   typedef struct PCII440FXState PCII440FXState;
>
> -PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
> +PCIBus *i440fx_init(const char *type,
> +                    PCII440FXState **pi440fx_state, int *piix_devfn,
>                       ISABus **isa_bus, qemu_irq *pic,
>                       MemoryRegion *address_space_mem,
>                       MemoryRegion *address_space_io,
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 31125b7..5fefde6 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -194,7 +194,8 @@ static void pc_init1(MachineState *machine,
>       }
>
>       if (pci_enabled) {
> -        pci_bus = i440fx_init(&i440fx_state, &piix3_devfn, &isa_bus, gsi,
> +        pci_bus = i440fx_init(TYPE_I440FX_PCI_HOST_BRIDGE,
> +                              &i440fx_state, &piix3_devfn, &isa_bus, gsi,
>                                 system_memory, system_io, machine->ram_size,
>                                 below_4g_mem_size,
>                                 above_4g_mem_size,
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index e0e0946..f0c5b5d 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -305,7 +305,8 @@ static int i440fx_initfn(PCIDevice *dev)
>       return 0;
>   }
>
> -PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> +PCIBus *i440fx_init(const char *type,
> +                    PCII440FXState **pi440fx_state,
>                       int *piix3_devfn,
>                       ISABus **isa_bus, qemu_irq *pic,
>                       MemoryRegion *address_space_mem,
> @@ -325,7 +326,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
>       unsigned i;
>       I440FXState *i440fx;
>
> -    dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST_BRIDGE);
> +    dev = qdev_create(NULL, type);
>       s = PCI_HOST_BRIDGE(dev);
>       b = pci_bus_new(dev, NULL, pci_address_space,
>                       address_space_io, 0, TYPE_PCI_BUS);
>
>

Re: [v2][PATCH 2/5] hw:pci-host:piix: split i440fx_init

[Chen, Tiejun]

On 2014/7/31 17:10, Michael S. Tsirkin wrote:
> On Thu, Jul 31, 2014 at 02:31:36PM +0800, Tiejun Chen wrote:
>> We'd like to split i440fx_init and then we can share something
>> with other stuff.
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>
> I think this is too much work for very little benefit.
> Just pass const char *type to i440fx_init.

You know we will introduce that faked PCIe device represented that PCH 
later, so how to distinguish them? Are you saying I should check the 
type like this?

if(Xen-Type)
{}
else
{}

If so, actually this is mostly same as my original implementation,

"
@@ -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;
"

But as I said to you last time, Paolo doesn't like we walk the common 
codes :)

Tiejun

>
> -->
>
> i440fx: make type configurable at run-time
>
> Xen wants to supply a different pci host device,
> inheriting i440fx. Make this configurable.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> --
>
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index be8fdfe..72e612b 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -230,7 +230,8 @@ extern int no_hpet;
>   struct PCII440FXState;
>   typedef struct PCII440FXState PCII440FXState;
>
> -PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
> +PCIBus *i440fx_init(const char *type,
> +                    PCII440FXState **pi440fx_state, int *piix_devfn,
>                       ISABus **isa_bus, qemu_irq *pic,
>                       MemoryRegion *address_space_mem,
>                       MemoryRegion *address_space_io,
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 31125b7..5fefde6 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -194,7 +194,8 @@ static void pc_init1(MachineState *machine,
>       }
>
>       if (pci_enabled) {
> -        pci_bus = i440fx_init(&i440fx_state, &piix3_devfn, &isa_bus, gsi,
> +        pci_bus = i440fx_init(TYPE_I440FX_PCI_HOST_BRIDGE,
> +                              &i440fx_state, &piix3_devfn, &isa_bus, gsi,
>                                 system_memory, system_io, machine->ram_size,
>                                 below_4g_mem_size,
>                                 above_4g_mem_size,
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index e0e0946..f0c5b5d 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -305,7 +305,8 @@ static int i440fx_initfn(PCIDevice *dev)
>       return 0;
>   }
>
> -PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> +PCIBus *i440fx_init(const char *type,
> +                    PCII440FXState **pi440fx_state,
>                       int *piix3_devfn,
>                       ISABus **isa_bus, qemu_irq *pic,
>                       MemoryRegion *address_space_mem,
> @@ -325,7 +326,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
>       unsigned i;
>       I440FXState *i440fx;
>
> -    dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST_BRIDGE);
> +    dev = qdev_create(NULL, type);
>       s = PCI_HOST_BRIDGE(dev);
>       b = pci_bus_new(dev, NULL, pci_address_space,
>                       address_space_io, 0, TYPE_PCI_BUS);
>
>

Re: [Qemu-devel] [v2][PATCH 2/5] hw:pci-host:piix: split i440fx_init

[Michael S. Tsirkin]

On Thu, Jul 31, 2014 at 05:26:41PM +0800, Chen, Tiejun wrote:
> On 2014/7/31 17:10, Michael S. Tsirkin wrote:
> >On Thu, Jul 31, 2014 at 02:31:36PM +0800, Tiejun Chen wrote:
> >>We'd like to split i440fx_init and then we can share something
> >>with other stuff.
> >>
> >>Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> >
> >I think this is too much work for very little benefit.
> >Just pass const char *type to i440fx_init.
> 
> You know we will introduce that faked PCIe device represented that PCH
> later,

Later when? On top of this patch series? Would like to see it all
before applying this ...

> so how to distinguish them? Are you saying I should check the type
> like this?
> 
> if(Xen-Type)
> {}
> else
> {}
>

No! Put the code in init function for the respective class,
pass type as an argument:

----
i440fx: make types configurable at run-time

Xen wants to supply a different pci and host devices,
inheriting i440fx devices. Make types configurable.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

-->
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index be8fdfe..86f295a 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -230,7 +230,11 @@ extern int no_hpet;
 struct PCII440FXState;
 typedef struct PCII440FXState PCII440FXState;
 
-PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
+#define TYPE_I440FX_PCI_HOST_BRIDGE "i440FX-pcihost"
+#define TYPE_I440FX_PCI_DEVICE "i440FX"
+
+PCIBus *i440fx_init(const char *host_type, const char *pci_type,
+                    PCII440FXState **pi440fx_state, int *piix_devfn,
                     ISABus **isa_bus, qemu_irq *pic,
                     MemoryRegion *address_space_mem,
                     MemoryRegion *address_space_io,
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 31125b7..e0979cd 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -194,7 +194,9 @@ static void pc_init1(MachineState *machine,
     }
 
     if (pci_enabled) {
-        pci_bus = i440fx_init(&i440fx_state, &piix3_devfn, &isa_bus, gsi,
+        pci_bus = i440fx_init(TYPE_I440FX_PCI_HOST_BRIDGE,
+                              TYPE_I440FX_PCI_DEVICE,
+                              &i440fx_state, &piix3_devfn, &isa_bus, gsi,
                               system_memory, system_io, machine->ram_size,
                               below_4g_mem_size,
                               above_4g_mem_size,
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index e0e0946..0cd82b8 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -40,7 +40,6 @@
  * http://download.intel.com/design/chipsets/datashts/29054901.pdf
  */
 
-#define TYPE_I440FX_PCI_HOST_BRIDGE "i440FX-pcihost"
 #define I440FX_PCI_HOST_BRIDGE(obj) \
     OBJECT_CHECK(I440FXState, (obj), TYPE_I440FX_PCI_HOST_BRIDGE)
 
@@ -91,7 +90,6 @@ typedef struct PIIX3State {
     MemoryRegion rcr_mem;
 } PIIX3State;
 
-#define TYPE_I440FX_PCI_DEVICE "i440FX"
 #define I440FX_PCI_DEVICE(obj) \
     OBJECT_CHECK(PCII440FXState, (obj), TYPE_I440FX_PCI_DEVICE)
 
@@ -305,7 +303,8 @@ static int i440fx_initfn(PCIDevice *dev)
     return 0;
 }
 
-PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
+PCIBus *i440fx_init(const char *host_type, const char *pci_type,
+                    PCII440FXState **pi440fx_state,
                     int *piix3_devfn,
                     ISABus **isa_bus, qemu_irq *pic,
                     MemoryRegion *address_space_mem,
@@ -325,7 +324,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
     unsigned i;
     I440FXState *i440fx;
 
-    dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST_BRIDGE);
+    dev = qdev_create(NULL, host_type);
     s = PCI_HOST_BRIDGE(dev);
     b = pci_bus_new(dev, NULL, pci_address_space,
                     address_space_io, 0, TYPE_PCI_BUS);
@@ -333,7 +332,7 @@ 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);
+    d = pci_create_simple(b, 0, pci_type);
     *pi440fx_state = I440FX_PCI_DEVICE(d);
     f = *pi440fx_state;
     f->system_memory = address_space_mem;

Re: [v2][PATCH 2/5] hw:pci-host:piix: split i440fx_init

[Michael S. Tsirkin]

On Thu, Jul 31, 2014 at 05:26:41PM +0800, Chen, Tiejun wrote:
> On 2014/7/31 17:10, Michael S. Tsirkin wrote:
> >On Thu, Jul 31, 2014 at 02:31:36PM +0800, Tiejun Chen wrote:
> >>We'd like to split i440fx_init and then we can share something
> >>with other stuff.
> >>
> >>Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> >
> >I think this is too much work for very little benefit.
> >Just pass const char *type to i440fx_init.
> 
> You know we will introduce that faked PCIe device represented that PCH
> later,

Later when? On top of this patch series? Would like to see it all
before applying this ...

> so how to distinguish them? Are you saying I should check the type
> like this?
> 
> if(Xen-Type)
> {}
> else
> {}
>

No! Put the code in init function for the respective class,
pass type as an argument:

----
i440fx: make types configurable at run-time

Xen wants to supply a different pci and host devices,
inheriting i440fx devices. Make types configurable.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

-->
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index be8fdfe..86f295a 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -230,7 +230,11 @@ extern int no_hpet;
 struct PCII440FXState;
 typedef struct PCII440FXState PCII440FXState;
 
-PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
+#define TYPE_I440FX_PCI_HOST_BRIDGE "i440FX-pcihost"
+#define TYPE_I440FX_PCI_DEVICE "i440FX"
+
+PCIBus *i440fx_init(const char *host_type, const char *pci_type,
+                    PCII440FXState **pi440fx_state, int *piix_devfn,
                     ISABus **isa_bus, qemu_irq *pic,
                     MemoryRegion *address_space_mem,
                     MemoryRegion *address_space_io,
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 31125b7..e0979cd 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -194,7 +194,9 @@ static void pc_init1(MachineState *machine,
     }
 
     if (pci_enabled) {
-        pci_bus = i440fx_init(&i440fx_state, &piix3_devfn, &isa_bus, gsi,
+        pci_bus = i440fx_init(TYPE_I440FX_PCI_HOST_BRIDGE,
+                              TYPE_I440FX_PCI_DEVICE,
+                              &i440fx_state, &piix3_devfn, &isa_bus, gsi,
                               system_memory, system_io, machine->ram_size,
                               below_4g_mem_size,
                               above_4g_mem_size,
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index e0e0946..0cd82b8 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -40,7 +40,6 @@
  * http://download.intel.com/design/chipsets/datashts/29054901.pdf
  */
 
-#define TYPE_I440FX_PCI_HOST_BRIDGE "i440FX-pcihost"
 #define I440FX_PCI_HOST_BRIDGE(obj) \
     OBJECT_CHECK(I440FXState, (obj), TYPE_I440FX_PCI_HOST_BRIDGE)
 
@@ -91,7 +90,6 @@ typedef struct PIIX3State {
     MemoryRegion rcr_mem;
 } PIIX3State;
 
-#define TYPE_I440FX_PCI_DEVICE "i440FX"
 #define I440FX_PCI_DEVICE(obj) \
     OBJECT_CHECK(PCII440FXState, (obj), TYPE_I440FX_PCI_DEVICE)
 
@@ -305,7 +303,8 @@ static int i440fx_initfn(PCIDevice *dev)
     return 0;
 }
 
-PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
+PCIBus *i440fx_init(const char *host_type, const char *pci_type,
+                    PCII440FXState **pi440fx_state,
                     int *piix3_devfn,
                     ISABus **isa_bus, qemu_irq *pic,
                     MemoryRegion *address_space_mem,
@@ -325,7 +324,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
     unsigned i;
     I440FXState *i440fx;
 
-    dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST_BRIDGE);
+    dev = qdev_create(NULL, host_type);
     s = PCI_HOST_BRIDGE(dev);
     b = pci_bus_new(dev, NULL, pci_address_space,
                     address_space_io, 0, TYPE_PCI_BUS);
@@ -333,7 +332,7 @@ 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);
+    d = pci_create_simple(b, 0, pci_type);
     *pi440fx_state = I440FX_PCI_DEVICE(d);
     f = *pi440fx_state;
     f->system_memory = address_space_mem;

Re: [Qemu-devel] [v2][PATCH 2/5] hw:pci-host:piix: split i440fx_init

[Chen, Tiejun]

On 2014/7/31 17:53, Michael S. Tsirkin wrote:
> On Thu, Jul 31, 2014 at 05:26:41PM +0800, Chen, Tiejun wrote:
>> On 2014/7/31 17:10, Michael S. Tsirkin wrote:
>>> On Thu, Jul 31, 2014 at 02:31:36PM +0800, Tiejun Chen wrote:
>>>> We'd like to split i440fx_init and then we can share something
>>>> with other stuff.
>>>>
>>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>>
>>> I think this is too much work for very little benefit.
>>> Just pass const char *type to i440fx_init.
>>
>> You know we will introduce that faked PCIe device represented that PCH
>> later,
>
> Later when? On top of this patch series? Would like to see it all
> before applying this ...

I will send this with other IGD stuff after this series is fine to you 
since its just creating a simple PCIe device.

I think you should know this whole story since as you guys discussed we 
don't fix that PCH at 1f.0. So it may be like this,

static int create_pseudo_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice *hdev)
{

	struct PCIDevice *dev;

	char rid;

	/* We havt to use a simple PCI device to fake this ISA bridge
	 * to avoid making some confusion to BIOS and ACPI.
	 */
	dev = pci_create(bus, -1, "pseudo-intel-pch-isa-bridge");

	qdev_init_nofail(&dev->qdev);
	pci_config_set_vendor_id(dev->config, XEN_SUBSYSTEM_ID));
						^
				I don't remember this exactly.
	pci_config_set_device_id(dev->config, hdev->device_id);

	return 0;
}

>
>> so how to distinguish them? Are you saying I should check the type
>> like this?
>>
>> if(Xen-Type)
>> {}
>> else
>> {}
>>
>
> No! Put the code in init function for the respective class,
> pass type as an argument:

If you mean we don't introduce any "if/else", I still don't understand 
how to insert such that function above, could you show this exactly?

Tiejun

>
> ----
> i440fx: make types configurable at run-time
>
> Xen wants to supply a different pci and host devices,
> inheriting i440fx devices. Make types configurable.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> -->
>
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index be8fdfe..86f295a 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -230,7 +230,11 @@ extern int no_hpet;
>   struct PCII440FXState;
>   typedef struct PCII440FXState PCII440FXState;
>
> -PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
> +#define TYPE_I440FX_PCI_HOST_BRIDGE "i440FX-pcihost"
> +#define TYPE_I440FX_PCI_DEVICE "i440FX"
> +
> +PCIBus *i440fx_init(const char *host_type, const char *pci_type,
> +                    PCII440FXState **pi440fx_state, int *piix_devfn,
>                       ISABus **isa_bus, qemu_irq *pic,
>                       MemoryRegion *address_space_mem,
>                       MemoryRegion *address_space_io,
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 31125b7..e0979cd 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -194,7 +194,9 @@ static void pc_init1(MachineState *machine,
>       }
>
>       if (pci_enabled) {
> -        pci_bus = i440fx_init(&i440fx_state, &piix3_devfn, &isa_bus, gsi,
> +        pci_bus = i440fx_init(TYPE_I440FX_PCI_HOST_BRIDGE,
> +                              TYPE_I440FX_PCI_DEVICE,
> +                              &i440fx_state, &piix3_devfn, &isa_bus, gsi,
>                                 system_memory, system_io, machine->ram_size,
>                                 below_4g_mem_size,
>                                 above_4g_mem_size,
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index e0e0946..0cd82b8 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -40,7 +40,6 @@
>    * http://download.intel.com/design/chipsets/datashts/29054901.pdf
>    */
>
> -#define TYPE_I440FX_PCI_HOST_BRIDGE "i440FX-pcihost"
>   #define I440FX_PCI_HOST_BRIDGE(obj) \
>       OBJECT_CHECK(I440FXState, (obj), TYPE_I440FX_PCI_HOST_BRIDGE)
>
> @@ -91,7 +90,6 @@ typedef struct PIIX3State {
>       MemoryRegion rcr_mem;
>   } PIIX3State;
>
> -#define TYPE_I440FX_PCI_DEVICE "i440FX"
>   #define I440FX_PCI_DEVICE(obj) \
>       OBJECT_CHECK(PCII440FXState, (obj), TYPE_I440FX_PCI_DEVICE)
>
> @@ -305,7 +303,8 @@ static int i440fx_initfn(PCIDevice *dev)
>       return 0;
>   }
>
> -PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> +PCIBus *i440fx_init(const char *host_type, const char *pci_type,
> +                    PCII440FXState **pi440fx_state,
>                       int *piix3_devfn,
>                       ISABus **isa_bus, qemu_irq *pic,
>                       MemoryRegion *address_space_mem,
> @@ -325,7 +324,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
>       unsigned i;
>       I440FXState *i440fx;
>
> -    dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST_BRIDGE);
> +    dev = qdev_create(NULL, host_type);
>       s = PCI_HOST_BRIDGE(dev);
>       b = pci_bus_new(dev, NULL, pci_address_space,
>                       address_space_io, 0, TYPE_PCI_BUS);
> @@ -333,7 +332,7 @@ 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);
> +    d = pci_create_simple(b, 0, pci_type);
>       *pi440fx_state = I440FX_PCI_DEVICE(d);
>       f = *pi440fx_state;
>       f->system_memory = address_space_mem;
>
>

Re: [v2][PATCH 2/5] hw:pci-host:piix: split i440fx_init

[Chen, Tiejun]

On 2014/7/31 17:53, Michael S. Tsirkin wrote:
> On Thu, Jul 31, 2014 at 05:26:41PM +0800, Chen, Tiejun wrote:
>> On 2014/7/31 17:10, Michael S. Tsirkin wrote:
>>> On Thu, Jul 31, 2014 at 02:31:36PM +0800, Tiejun Chen wrote:
>>>> We'd like to split i440fx_init and then we can share something
>>>> with other stuff.
>>>>
>>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>>
>>> I think this is too much work for very little benefit.
>>> Just pass const char *type to i440fx_init.
>>
>> You know we will introduce that faked PCIe device represented that PCH
>> later,
>
> Later when? On top of this patch series? Would like to see it all
> before applying this ...

I will send this with other IGD stuff after this series is fine to you 
since its just creating a simple PCIe device.

I think you should know this whole story since as you guys discussed we 
don't fix that PCH at 1f.0. So it may be like this,

static int create_pseudo_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice *hdev)
{

	struct PCIDevice *dev;

	char rid;

	/* We havt to use a simple PCI device to fake this ISA bridge
	 * to avoid making some confusion to BIOS and ACPI.
	 */
	dev = pci_create(bus, -1, "pseudo-intel-pch-isa-bridge");

	qdev_init_nofail(&dev->qdev);
	pci_config_set_vendor_id(dev->config, XEN_SUBSYSTEM_ID));
						^
				I don't remember this exactly.
	pci_config_set_device_id(dev->config, hdev->device_id);

	return 0;
}

>
>> so how to distinguish them? Are you saying I should check the type
>> like this?
>>
>> if(Xen-Type)
>> {}
>> else
>> {}
>>
>
> No! Put the code in init function for the respective class,
> pass type as an argument:

If you mean we don't introduce any "if/else", I still don't understand 
how to insert such that function above, could you show this exactly?

Tiejun

>
> ----
> i440fx: make types configurable at run-time
>
> Xen wants to supply a different pci and host devices,
> inheriting i440fx devices. Make types configurable.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> -->
>
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index be8fdfe..86f295a 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -230,7 +230,11 @@ extern int no_hpet;
>   struct PCII440FXState;
>   typedef struct PCII440FXState PCII440FXState;
>
> -PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
> +#define TYPE_I440FX_PCI_HOST_BRIDGE "i440FX-pcihost"
> +#define TYPE_I440FX_PCI_DEVICE "i440FX"
> +
> +PCIBus *i440fx_init(const char *host_type, const char *pci_type,
> +                    PCII440FXState **pi440fx_state, int *piix_devfn,
>                       ISABus **isa_bus, qemu_irq *pic,
>                       MemoryRegion *address_space_mem,
>                       MemoryRegion *address_space_io,
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 31125b7..e0979cd 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -194,7 +194,9 @@ static void pc_init1(MachineState *machine,
>       }
>
>       if (pci_enabled) {
> -        pci_bus = i440fx_init(&i440fx_state, &piix3_devfn, &isa_bus, gsi,
> +        pci_bus = i440fx_init(TYPE_I440FX_PCI_HOST_BRIDGE,
> +                              TYPE_I440FX_PCI_DEVICE,
> +                              &i440fx_state, &piix3_devfn, &isa_bus, gsi,
>                                 system_memory, system_io, machine->ram_size,
>                                 below_4g_mem_size,
>                                 above_4g_mem_size,
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index e0e0946..0cd82b8 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -40,7 +40,6 @@
>    * http://download.intel.com/design/chipsets/datashts/29054901.pdf
>    */
>
> -#define TYPE_I440FX_PCI_HOST_BRIDGE "i440FX-pcihost"
>   #define I440FX_PCI_HOST_BRIDGE(obj) \
>       OBJECT_CHECK(I440FXState, (obj), TYPE_I440FX_PCI_HOST_BRIDGE)
>
> @@ -91,7 +90,6 @@ typedef struct PIIX3State {
>       MemoryRegion rcr_mem;
>   } PIIX3State;
>
> -#define TYPE_I440FX_PCI_DEVICE "i440FX"
>   #define I440FX_PCI_DEVICE(obj) \
>       OBJECT_CHECK(PCII440FXState, (obj), TYPE_I440FX_PCI_DEVICE)
>
> @@ -305,7 +303,8 @@ static int i440fx_initfn(PCIDevice *dev)
>       return 0;
>   }
>
> -PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> +PCIBus *i440fx_init(const char *host_type, const char *pci_type,
> +                    PCII440FXState **pi440fx_state,
>                       int *piix3_devfn,
>                       ISABus **isa_bus, qemu_irq *pic,
>                       MemoryRegion *address_space_mem,
> @@ -325,7 +324,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
>       unsigned i;
>       I440FXState *i440fx;
>
> -    dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST_BRIDGE);
> +    dev = qdev_create(NULL, host_type);
>       s = PCI_HOST_BRIDGE(dev);
>       b = pci_bus_new(dev, NULL, pci_address_space,
>                       address_space_io, 0, TYPE_PCI_BUS);
> @@ -333,7 +332,7 @@ 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);
> +    d = pci_create_simple(b, 0, pci_type);
>       *pi440fx_state = I440FX_PCI_DEVICE(d);
>       f = *pi440fx_state;
>       f->system_memory = address_space_mem;
>
>

Re: [Qemu-devel] [v2][PATCH 2/5] hw:pci-host:piix: split i440fx_init

[Chen, Tiejun]

On 2014/7/31 18:12, Chen, Tiejun wrote:
> On 2014/7/31 17:53, Michael S. Tsirkin wrote:
>> On Thu, Jul 31, 2014 at 05:26:41PM +0800, Chen, Tiejun wrote:
>>> On 2014/7/31 17:10, Michael S. Tsirkin wrote:
>>>> On Thu, Jul 31, 2014 at 02:31:36PM +0800, Tiejun Chen wrote:
>>>>> We'd like to split i440fx_init and then we can share something
>>>>> with other stuff.
>>>>>
>>>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>>>
>>>> I think this is too much work for very little benefit.
>>>> Just pass const char *type to i440fx_init.
>>>
>>> You know we will introduce that faked PCIe device represented that PCH
>>> later,
>>
>> Later when? On top of this patch series? Would like to see it all
>> before applying this ...
>
> I will send this with other IGD stuff after this series is fine to you
> since its just creating a simple PCIe device.
>
> I think you should know this whole story since as you guys discussed we
> don't fix that PCH at 1f.0. So it may be like this,
>
> static int create_pseudo_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice
> *hdev)
> {
>
>      struct PCIDevice *dev;
>
>      char rid;
>
>      /* We havt to use a simple PCI device to fake this ISA bridge
>       * to avoid making some confusion to BIOS and ACPI.
>       */
>      dev = pci_create(bus, -1, "pseudo-intel-pch-isa-bridge");
>
>      qdev_init_nofail(&dev->qdev);
>      pci_config_set_vendor_id(dev->config, XEN_SUBSYSTEM_ID));
>                          ^
>                  I don't remember this exactly.
>      pci_config_set_device_id(dev->config, hdev->device_id);
>
>      return 0;
> }
>
>>
>>> so how to distinguish them? Are you saying I should check the type
>>> like this?
>>>
>>> if(Xen-Type)
>>> {}
>>> else
>>> {}
>>>
>>
>> No! Put the code in init function for the respective class,
>> pass type as an argument:
>
> If you mean we don't introduce any "if/else", I still don't understand
> how to insert such that function above, could you show this exactly?
>

I think I can do this inside that into 
xen_igd_passthrough_pc_machine_pci_bus_init(). There, we can just follow 
pci_bus = i440fx_init() since create_pseudo_pch_isa_bridge() just needs 
pci_bus as a parameter.

Thanks
Tiejun

Re: [v2][PATCH 2/5] hw:pci-host:piix: split i440fx_init

[Chen, Tiejun]

On 2014/7/31 18:12, Chen, Tiejun wrote:
> On 2014/7/31 17:53, Michael S. Tsirkin wrote:
>> On Thu, Jul 31, 2014 at 05:26:41PM +0800, Chen, Tiejun wrote:
>>> On 2014/7/31 17:10, Michael S. Tsirkin wrote:
>>>> On Thu, Jul 31, 2014 at 02:31:36PM +0800, Tiejun Chen wrote:
>>>>> We'd like to split i440fx_init and then we can share something
>>>>> with other stuff.
>>>>>
>>>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>>>
>>>> I think this is too much work for very little benefit.
>>>> Just pass const char *type to i440fx_init.
>>>
>>> You know we will introduce that faked PCIe device represented that PCH
>>> later,
>>
>> Later when? On top of this patch series? Would like to see it all
>> before applying this ...
>
> I will send this with other IGD stuff after this series is fine to you
> since its just creating a simple PCIe device.
>
> I think you should know this whole story since as you guys discussed we
> don't fix that PCH at 1f.0. So it may be like this,
>
> static int create_pseudo_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice
> *hdev)
> {
>
>      struct PCIDevice *dev;
>
>      char rid;
>
>      /* We havt to use a simple PCI device to fake this ISA bridge
>       * to avoid making some confusion to BIOS and ACPI.
>       */
>      dev = pci_create(bus, -1, "pseudo-intel-pch-isa-bridge");
>
>      qdev_init_nofail(&dev->qdev);
>      pci_config_set_vendor_id(dev->config, XEN_SUBSYSTEM_ID));
>                          ^
>                  I don't remember this exactly.
>      pci_config_set_device_id(dev->config, hdev->device_id);
>
>      return 0;
> }
>
>>
>>> so how to distinguish them? Are you saying I should check the type
>>> like this?
>>>
>>> if(Xen-Type)
>>> {}
>>> else
>>> {}
>>>
>>
>> No! Put the code in init function for the respective class,
>> pass type as an argument:
>
> If you mean we don't introduce any "if/else", I still don't understand
> how to insert such that function above, could you show this exactly?
>

I think I can do this inside that into 
xen_igd_passthrough_pc_machine_pci_bus_init(). There, we can just follow 
pci_bus = i440fx_init() since create_pseudo_pch_isa_bridge() just needs 
pci_bus as a parameter.

Thanks
Tiejun

Re: [Qemu-devel] [v2][PATCH 2/5] hw:pci-host:piix: split i440fx_init

[Michael S. Tsirkin]

On Thu, Jul 31, 2014 at 08:10:53PM +0800, Chen, Tiejun wrote:
> On 2014/7/31 18:12, Chen, Tiejun wrote:
> >On 2014/7/31 17:53, Michael S. Tsirkin wrote:
> >>On Thu, Jul 31, 2014 at 05:26:41PM +0800, Chen, Tiejun wrote:
> >>>On 2014/7/31 17:10, Michael S. Tsirkin wrote:
> >>>>On Thu, Jul 31, 2014 at 02:31:36PM +0800, Tiejun Chen wrote:
> >>>>>We'd like to split i440fx_init and then we can share something
> >>>>>with other stuff.
> >>>>>
> >>>>>Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> >>>>
> >>>>I think this is too much work for very little benefit.
> >>>>Just pass const char *type to i440fx_init.
> >>>
> >>>You know we will introduce that faked PCIe device represented that PCH
> >>>later,
> >>
> >>Later when? On top of this patch series? Would like to see it all
> >>before applying this ...
> >
> >I will send this with other IGD stuff after this series is fine to you
> >since its just creating a simple PCIe device.
> >
> >I think you should know this whole story since as you guys discussed we
> >don't fix that PCH at 1f.0. So it may be like this,
> >
> >static int create_pseudo_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice
> >*hdev)
> >{
> >
> >     struct PCIDevice *dev;
> >
> >     char rid;
> >
> >     /* We havt to use a simple PCI device to fake this ISA bridge
> >      * to avoid making some confusion to BIOS and ACPI.
> >      */
> >     dev = pci_create(bus, -1, "pseudo-intel-pch-isa-bridge");
> >
> >     qdev_init_nofail(&dev->qdev);
> >     pci_config_set_vendor_id(dev->config, XEN_SUBSYSTEM_ID));
> >                         ^
> >                 I don't remember this exactly.
> >     pci_config_set_device_id(dev->config, hdev->device_id);
> >
> >     return 0;
> >}
> >
> >>
> >>>so how to distinguish them? Are you saying I should check the type
> >>>like this?
> >>>
> >>>if(Xen-Type)
> >>>{}
> >>>else
> >>>{}
> >>>
> >>
> >>No! Put the code in init function for the respective class,
> >>pass type as an argument:
> >
> >If you mean we don't introduce any "if/else", I still don't understand
> >how to insert such that function above, could you show this exactly?
> >
> 
> I think I can do this inside that into
> xen_igd_passthrough_pc_machine_pci_bus_init(). There, we can just follow
> pci_bus = i440fx_init() since create_pseudo_pch_isa_bridge() just needs
> pci_bus as a parameter.
> 
> Thanks
> Tiejun

Do it in xen_igd_i440fx_class_init: constructor for your class.
This device is not hot-pluggable right?
If so I would prefer not to make this xen specific.
Instead, find the device and tweak the id when you
initialize the card.

-- 
MST

Re: [v2][PATCH 2/5] hw:pci-host:piix: split i440fx_init

[Michael S. Tsirkin]

On Thu, Jul 31, 2014 at 08:10:53PM +0800, Chen, Tiejun wrote:
> On 2014/7/31 18:12, Chen, Tiejun wrote:
> >On 2014/7/31 17:53, Michael S. Tsirkin wrote:
> >>On Thu, Jul 31, 2014 at 05:26:41PM +0800, Chen, Tiejun wrote:
> >>>On 2014/7/31 17:10, Michael S. Tsirkin wrote:
> >>>>On Thu, Jul 31, 2014 at 02:31:36PM +0800, Tiejun Chen wrote:
> >>>>>We'd like to split i440fx_init and then we can share something
> >>>>>with other stuff.
> >>>>>
> >>>>>Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> >>>>
> >>>>I think this is too much work for very little benefit.
> >>>>Just pass const char *type to i440fx_init.
> >>>
> >>>You know we will introduce that faked PCIe device represented that PCH
> >>>later,
> >>
> >>Later when? On top of this patch series? Would like to see it all
> >>before applying this ...
> >
> >I will send this with other IGD stuff after this series is fine to you
> >since its just creating a simple PCIe device.
> >
> >I think you should know this whole story since as you guys discussed we
> >don't fix that PCH at 1f.0. So it may be like this,
> >
> >static int create_pseudo_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice
> >*hdev)
> >{
> >
> >     struct PCIDevice *dev;
> >
> >     char rid;
> >
> >     /* We havt to use a simple PCI device to fake this ISA bridge
> >      * to avoid making some confusion to BIOS and ACPI.
> >      */
> >     dev = pci_create(bus, -1, "pseudo-intel-pch-isa-bridge");
> >
> >     qdev_init_nofail(&dev->qdev);
> >     pci_config_set_vendor_id(dev->config, XEN_SUBSYSTEM_ID));
> >                         ^
> >                 I don't remember this exactly.
> >     pci_config_set_device_id(dev->config, hdev->device_id);
> >
> >     return 0;
> >}
> >
> >>
> >>>so how to distinguish them? Are you saying I should check the type
> >>>like this?
> >>>
> >>>if(Xen-Type)
> >>>{}
> >>>else
> >>>{}
> >>>
> >>
> >>No! Put the code in init function for the respective class,
> >>pass type as an argument:
> >
> >If you mean we don't introduce any "if/else", I still don't understand
> >how to insert such that function above, could you show this exactly?
> >
> 
> I think I can do this inside that into
> xen_igd_passthrough_pc_machine_pci_bus_init(). There, we can just follow
> pci_bus = i440fx_init() since create_pseudo_pch_isa_bridge() just needs
> pci_bus as a parameter.
> 
> Thanks
> Tiejun

Do it in xen_igd_i440fx_class_init: constructor for your class.
This device is not hot-pluggable right?
If so I would prefer not to make this xen specific.
Instead, find the device and tweak the id when you
initialize the card.

-- 
MST

Re: [Qemu-devel] [v2][PATCH 2/5] hw:pci-host:piix: split i440fx_init

[Chen, Tiejun]

On 2014/7/31 23:47, Michael S. Tsirkin wrote:
> On Thu, Jul 31, 2014 at 08:10:53PM +0800, Chen, Tiejun wrote:
>> On 2014/7/31 18:12, Chen, Tiejun wrote:
>>> On 2014/7/31 17:53, Michael S. Tsirkin wrote:
>>>> On Thu, Jul 31, 2014 at 05:26:41PM +0800, Chen, Tiejun wrote:
>>>>> On 2014/7/31 17:10, Michael S. Tsirkin wrote:
>>>>>> On Thu, Jul 31, 2014 at 02:31:36PM +0800, Tiejun Chen wrote:
>>>>>>> We'd like to split i440fx_init and then we can share something
>>>>>>> with other stuff.
>>>>>>>
>>>>>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>>>>>
>>>>>> I think this is too much work for very little benefit.
>>>>>> Just pass const char *type to i440fx_init.
>>>>>
>>>>> You know we will introduce that faked PCIe device represented that PCH
>>>>> later,
>>>>
>>>> Later when? On top of this patch series? Would like to see it all
>>>> before applying this ...
>>>
>>> I will send this with other IGD stuff after this series is fine to you
>>> since its just creating a simple PCIe device.
>>>
>>> I think you should know this whole story since as you guys discussed we
>>> don't fix that PCH at 1f.0. So it may be like this,
>>>
>>> static int create_pseudo_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice
>>> *hdev)
>>> {
>>>
>>>      struct PCIDevice *dev;
>>>
>>>      char rid;
>>>
>>>      /* We havt to use a simple PCI device to fake this ISA bridge
>>>       * to avoid making some confusion to BIOS and ACPI.
>>>       */
>>>      dev = pci_create(bus, -1, "pseudo-intel-pch-isa-bridge");
>>>
>>>      qdev_init_nofail(&dev->qdev);
>>>      pci_config_set_vendor_id(dev->config, XEN_SUBSYSTEM_ID));
>>>                          ^
>>>                  I don't remember this exactly.
>>>      pci_config_set_device_id(dev->config, hdev->device_id);
>>>
>>>      return 0;
>>> }
>>>
>>>>
>>>>> so how to distinguish them? Are you saying I should check the type
>>>>> like this?
>>>>>
>>>>> if(Xen-Type)
>>>>> {}
>>>>> else
>>>>> {}
>>>>>
>>>>
>>>> No! Put the code in init function for the respective class,
>>>> pass type as an argument:
>>>
>>> If you mean we don't introduce any "if/else", I still don't understand
>>> how to insert such that function above, could you show this exactly?
>>>
>>
>> I think I can do this inside that into
>> xen_igd_passthrough_pc_machine_pci_bus_init(). There, we can just follow
>> pci_bus = i440fx_init() since create_pseudo_pch_isa_bridge() just needs
>> pci_bus as a parameter.
>>
>> Thanks
>> Tiejun
>
> Do it in xen_igd_i440fx_class_init: constructor for your class.
> This device is not hot-pluggable right?
> If so I would prefer not to make this xen specific.
> Instead, find the device and tweak the id when you
> initialize the card.

Michael,

I mean in v3 patches series to do something like,

xen_igd_passthrough_pc_machine_pci_bus_init()
{
	if (pci_enabled) {
		pci_bus = i440fx_init();
		create_pseudo_pch_isa_bridge()
	}

Thanks
Tiejun

Re: [v2][PATCH 2/5] hw:pci-host:piix: split i440fx_init

[Chen, Tiejun]

On 2014/7/31 23:47, Michael S. Tsirkin wrote:
> On Thu, Jul 31, 2014 at 08:10:53PM +0800, Chen, Tiejun wrote:
>> On 2014/7/31 18:12, Chen, Tiejun wrote:
>>> On 2014/7/31 17:53, Michael S. Tsirkin wrote:
>>>> On Thu, Jul 31, 2014 at 05:26:41PM +0800, Chen, Tiejun wrote:
>>>>> On 2014/7/31 17:10, Michael S. Tsirkin wrote:
>>>>>> On Thu, Jul 31, 2014 at 02:31:36PM +0800, Tiejun Chen wrote:
>>>>>>> We'd like to split i440fx_init and then we can share something
>>>>>>> with other stuff.
>>>>>>>
>>>>>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>>>>>
>>>>>> I think this is too much work for very little benefit.
>>>>>> Just pass const char *type to i440fx_init.
>>>>>
>>>>> You know we will introduce that faked PCIe device represented that PCH
>>>>> later,
>>>>
>>>> Later when? On top of this patch series? Would like to see it all
>>>> before applying this ...
>>>
>>> I will send this with other IGD stuff after this series is fine to you
>>> since its just creating a simple PCIe device.
>>>
>>> I think you should know this whole story since as you guys discussed we
>>> don't fix that PCH at 1f.0. So it may be like this,
>>>
>>> static int create_pseudo_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice
>>> *hdev)
>>> {
>>>
>>>      struct PCIDevice *dev;
>>>
>>>      char rid;
>>>
>>>      /* We havt to use a simple PCI device to fake this ISA bridge
>>>       * to avoid making some confusion to BIOS and ACPI.
>>>       */
>>>      dev = pci_create(bus, -1, "pseudo-intel-pch-isa-bridge");
>>>
>>>      qdev_init_nofail(&dev->qdev);
>>>      pci_config_set_vendor_id(dev->config, XEN_SUBSYSTEM_ID));
>>>                          ^
>>>                  I don't remember this exactly.
>>>      pci_config_set_device_id(dev->config, hdev->device_id);
>>>
>>>      return 0;
>>> }
>>>
>>>>
>>>>> so how to distinguish them? Are you saying I should check the type
>>>>> like this?
>>>>>
>>>>> if(Xen-Type)
>>>>> {}
>>>>> else
>>>>> {}
>>>>>
>>>>
>>>> No! Put the code in init function for the respective class,
>>>> pass type as an argument:
>>>
>>> If you mean we don't introduce any "if/else", I still don't understand
>>> how to insert such that function above, could you show this exactly?
>>>
>>
>> I think I can do this inside that into
>> xen_igd_passthrough_pc_machine_pci_bus_init(). There, we can just follow
>> pci_bus = i440fx_init() since create_pseudo_pch_isa_bridge() just needs
>> pci_bus as a parameter.
>>
>> Thanks
>> Tiejun
>
> Do it in xen_igd_i440fx_class_init: constructor for your class.
> This device is not hot-pluggable right?
> If so I would prefer not to make this xen specific.
> Instead, find the device and tweak the id when you
> initialize the card.

Michael,

I mean in v3 patches series to do something like,

xen_igd_passthrough_pc_machine_pci_bus_init()
{
	if (pci_enabled) {
		pci_bus = i440fx_init();
		create_pseudo_pch_isa_bridge()
	}

Thanks
Tiejun

Re: [Qemu-devel] [v2][PATCH 2/5] hw:pci-host:piix: split i440fx_init

[Michael S. Tsirkin]

On Thu, Jul 31, 2014 at 06:12:32PM +0800, Chen, Tiejun wrote:
> On 2014/7/31 17:53, Michael S. Tsirkin wrote:
> >On Thu, Jul 31, 2014 at 05:26:41PM +0800, Chen, Tiejun wrote:
> >>On 2014/7/31 17:10, Michael S. Tsirkin wrote:
> >>>On Thu, Jul 31, 2014 at 02:31:36PM +0800, Tiejun Chen wrote:
> >>>>We'd like to split i440fx_init and then we can share something
> >>>>with other stuff.
> >>>>
> >>>>Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> >>>
> >>>I think this is too much work for very little benefit.
> >>>Just pass const char *type to i440fx_init.
> >>
> >>You know we will introduce that faked PCIe device represented that PCH
> >>later,
> >
> >Later when? On top of this patch series? Would like to see it all
> >before applying this ...
> 
> I will send this with other IGD stuff after this series is fine to you since
> its just creating a simple PCIe device.
> 
> I think you should know this whole story since as you guys discussed we
> don't fix that PCH at 1f.0.


And this works with all legacy drivers and with windows
drivers?


> So it may be like this,
> 
> static int create_pseudo_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice *hdev)
> {
> 
> 	struct PCIDevice *dev;
> 
> 	char rid;
> 
> 	/* We havt to use a simple PCI device to fake this ISA bridge
> 	 * to avoid making some confusion to BIOS and ACPI.
> 	 */
> 	dev = pci_create(bus, -1, "pseudo-intel-pch-isa-bridge");
> 
> 	qdev_init_nofail(&dev->qdev);
> 	pci_config_set_vendor_id(dev->config, XEN_SUBSYSTEM_ID));
> 						^
> 				I don't remember this exactly.
> 	pci_config_set_device_id(dev->config, hdev->device_id);

This is a hack anyway, how about reverse-decoding
required device id from the supplied card instead of
poking at the host?
You can do it when card is initialized.

> 
> 	return 0;
> }
> >
> >>so how to distinguish them? Are you saying I should check the type
> >>like this?
> >>
> >>if(Xen-Type)
> >>{}
> >>else
> >>{}
> >>
> >
> >No! Put the code in init function for the respective class,
> >pass type as an argument:
> 
> If you mean we don't introduce any "if/else", I still don't understand how
> to insert such that function above, could you show this exactly?
> 
> Tiejun

Stick it in the constructor for your xen pt device.

> >
> >----
> >i440fx: make types configurable at run-time
> >
> >Xen wants to supply a different pci and host devices,
> >inheriting i440fx devices. Make types configurable.
> >
> >Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> >-->
> >
> >diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> >index be8fdfe..86f295a 100644
> >--- a/include/hw/i386/pc.h
> >+++ b/include/hw/i386/pc.h
> >@@ -230,7 +230,11 @@ extern int no_hpet;
> >  struct PCII440FXState;
> >  typedef struct PCII440FXState PCII440FXState;
> >
> >-PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
> >+#define TYPE_I440FX_PCI_HOST_BRIDGE "i440FX-pcihost"
> >+#define TYPE_I440FX_PCI_DEVICE "i440FX"
> >+
> >+PCIBus *i440fx_init(const char *host_type, const char *pci_type,
> >+                    PCII440FXState **pi440fx_state, int *piix_devfn,
> >                      ISABus **isa_bus, qemu_irq *pic,
> >                      MemoryRegion *address_space_mem,
> >                      MemoryRegion *address_space_io,
> >diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> >index 31125b7..e0979cd 100644
> >--- a/hw/i386/pc_piix.c
> >+++ b/hw/i386/pc_piix.c
> >@@ -194,7 +194,9 @@ static void pc_init1(MachineState *machine,
> >      }
> >
> >      if (pci_enabled) {
> >-        pci_bus = i440fx_init(&i440fx_state, &piix3_devfn, &isa_bus, gsi,
> >+        pci_bus = i440fx_init(TYPE_I440FX_PCI_HOST_BRIDGE,
> >+                              TYPE_I440FX_PCI_DEVICE,
> >+                              &i440fx_state, &piix3_devfn, &isa_bus, gsi,
> >                                system_memory, system_io, machine->ram_size,
> >                                below_4g_mem_size,
> >                                above_4g_mem_size,
> >diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> >index e0e0946..0cd82b8 100644
> >--- a/hw/pci-host/piix.c
> >+++ b/hw/pci-host/piix.c
> >@@ -40,7 +40,6 @@
> >   * http://download.intel.com/design/chipsets/datashts/29054901.pdf
> >   */
> >
> >-#define TYPE_I440FX_PCI_HOST_BRIDGE "i440FX-pcihost"
> >  #define I440FX_PCI_HOST_BRIDGE(obj) \
> >      OBJECT_CHECK(I440FXState, (obj), TYPE_I440FX_PCI_HOST_BRIDGE)
> >
> >@@ -91,7 +90,6 @@ typedef struct PIIX3State {
> >      MemoryRegion rcr_mem;
> >  } PIIX3State;
> >
> >-#define TYPE_I440FX_PCI_DEVICE "i440FX"
> >  #define I440FX_PCI_DEVICE(obj) \
> >      OBJECT_CHECK(PCII440FXState, (obj), TYPE_I440FX_PCI_DEVICE)
> >
> >@@ -305,7 +303,8 @@ static int i440fx_initfn(PCIDevice *dev)
> >      return 0;
> >  }
> >
> >-PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> >+PCIBus *i440fx_init(const char *host_type, const char *pci_type,
> >+                    PCII440FXState **pi440fx_state,
> >                      int *piix3_devfn,
> >                      ISABus **isa_bus, qemu_irq *pic,
> >                      MemoryRegion *address_space_mem,
> >@@ -325,7 +324,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> >      unsigned i;
> >      I440FXState *i440fx;
> >
> >-    dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST_BRIDGE);
> >+    dev = qdev_create(NULL, host_type);
> >      s = PCI_HOST_BRIDGE(dev);
> >      b = pci_bus_new(dev, NULL, pci_address_space,
> >                      address_space_io, 0, TYPE_PCI_BUS);
> >@@ -333,7 +332,7 @@ 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);
> >+    d = pci_create_simple(b, 0, pci_type);
> >      *pi440fx_state = I440FX_PCI_DEVICE(d);
> >      f = *pi440fx_state;
> >      f->system_memory = address_space_mem;
> >
> >

Re: [v2][PATCH 2/5] hw:pci-host:piix: split i440fx_init

[Michael S. Tsirkin]

On Thu, Jul 31, 2014 at 06:12:32PM +0800, Chen, Tiejun wrote:
> On 2014/7/31 17:53, Michael S. Tsirkin wrote:
> >On Thu, Jul 31, 2014 at 05:26:41PM +0800, Chen, Tiejun wrote:
> >>On 2014/7/31 17:10, Michael S. Tsirkin wrote:
> >>>On Thu, Jul 31, 2014 at 02:31:36PM +0800, Tiejun Chen wrote:
> >>>>We'd like to split i440fx_init and then we can share something
> >>>>with other stuff.
> >>>>
> >>>>Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> >>>
> >>>I think this is too much work for very little benefit.
> >>>Just pass const char *type to i440fx_init.
> >>
> >>You know we will introduce that faked PCIe device represented that PCH
> >>later,
> >
> >Later when? On top of this patch series? Would like to see it all
> >before applying this ...
> 
> I will send this with other IGD stuff after this series is fine to you since
> its just creating a simple PCIe device.
> 
> I think you should know this whole story since as you guys discussed we
> don't fix that PCH at 1f.0.


And this works with all legacy drivers and with windows
drivers?


> So it may be like this,
> 
> static int create_pseudo_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice *hdev)
> {
> 
> 	struct PCIDevice *dev;
> 
> 	char rid;
> 
> 	/* We havt to use a simple PCI device to fake this ISA bridge
> 	 * to avoid making some confusion to BIOS and ACPI.
> 	 */
> 	dev = pci_create(bus, -1, "pseudo-intel-pch-isa-bridge");
> 
> 	qdev_init_nofail(&dev->qdev);
> 	pci_config_set_vendor_id(dev->config, XEN_SUBSYSTEM_ID));
> 						^
> 				I don't remember this exactly.
> 	pci_config_set_device_id(dev->config, hdev->device_id);

This is a hack anyway, how about reverse-decoding
required device id from the supplied card instead of
poking at the host?
You can do it when card is initialized.

> 
> 	return 0;
> }
> >
> >>so how to distinguish them? Are you saying I should check the type
> >>like this?
> >>
> >>if(Xen-Type)
> >>{}
> >>else
> >>{}
> >>
> >
> >No! Put the code in init function for the respective class,
> >pass type as an argument:
> 
> If you mean we don't introduce any "if/else", I still don't understand how
> to insert such that function above, could you show this exactly?
> 
> Tiejun

Stick it in the constructor for your xen pt device.

> >
> >----
> >i440fx: make types configurable at run-time
> >
> >Xen wants to supply a different pci and host devices,
> >inheriting i440fx devices. Make types configurable.
> >
> >Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> >-->
> >
> >diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> >index be8fdfe..86f295a 100644
> >--- a/include/hw/i386/pc.h
> >+++ b/include/hw/i386/pc.h
> >@@ -230,7 +230,11 @@ extern int no_hpet;
> >  struct PCII440FXState;
> >  typedef struct PCII440FXState PCII440FXState;
> >
> >-PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
> >+#define TYPE_I440FX_PCI_HOST_BRIDGE "i440FX-pcihost"
> >+#define TYPE_I440FX_PCI_DEVICE "i440FX"
> >+
> >+PCIBus *i440fx_init(const char *host_type, const char *pci_type,
> >+                    PCII440FXState **pi440fx_state, int *piix_devfn,
> >                      ISABus **isa_bus, qemu_irq *pic,
> >                      MemoryRegion *address_space_mem,
> >                      MemoryRegion *address_space_io,
> >diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> >index 31125b7..e0979cd 100644
> >--- a/hw/i386/pc_piix.c
> >+++ b/hw/i386/pc_piix.c
> >@@ -194,7 +194,9 @@ static void pc_init1(MachineState *machine,
> >      }
> >
> >      if (pci_enabled) {
> >-        pci_bus = i440fx_init(&i440fx_state, &piix3_devfn, &isa_bus, gsi,
> >+        pci_bus = i440fx_init(TYPE_I440FX_PCI_HOST_BRIDGE,
> >+                              TYPE_I440FX_PCI_DEVICE,
> >+                              &i440fx_state, &piix3_devfn, &isa_bus, gsi,
> >                                system_memory, system_io, machine->ram_size,
> >                                below_4g_mem_size,
> >                                above_4g_mem_size,
> >diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> >index e0e0946..0cd82b8 100644
> >--- a/hw/pci-host/piix.c
> >+++ b/hw/pci-host/piix.c
> >@@ -40,7 +40,6 @@
> >   * http://download.intel.com/design/chipsets/datashts/29054901.pdf
> >   */
> >
> >-#define TYPE_I440FX_PCI_HOST_BRIDGE "i440FX-pcihost"
> >  #define I440FX_PCI_HOST_BRIDGE(obj) \
> >      OBJECT_CHECK(I440FXState, (obj), TYPE_I440FX_PCI_HOST_BRIDGE)
> >
> >@@ -91,7 +90,6 @@ typedef struct PIIX3State {
> >      MemoryRegion rcr_mem;
> >  } PIIX3State;
> >
> >-#define TYPE_I440FX_PCI_DEVICE "i440FX"
> >  #define I440FX_PCI_DEVICE(obj) \
> >      OBJECT_CHECK(PCII440FXState, (obj), TYPE_I440FX_PCI_DEVICE)
> >
> >@@ -305,7 +303,8 @@ static int i440fx_initfn(PCIDevice *dev)
> >      return 0;
> >  }
> >
> >-PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> >+PCIBus *i440fx_init(const char *host_type, const char *pci_type,
> >+                    PCII440FXState **pi440fx_state,
> >                      int *piix3_devfn,
> >                      ISABus **isa_bus, qemu_irq *pic,
> >                      MemoryRegion *address_space_mem,
> >@@ -325,7 +324,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> >      unsigned i;
> >      I440FXState *i440fx;
> >
> >-    dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST_BRIDGE);
> >+    dev = qdev_create(NULL, host_type);
> >      s = PCI_HOST_BRIDGE(dev);
> >      b = pci_bus_new(dev, NULL, pci_address_space,
> >                      address_space_io, 0, TYPE_PCI_BUS);
> >@@ -333,7 +332,7 @@ 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);
> >+    d = pci_create_simple(b, 0, pci_type);
> >      *pi440fx_state = I440FX_PCI_DEVICE(d);
> >      f = *pi440fx_state;
> >      f->system_memory = address_space_mem;
> >
> >

Re: [Qemu-devel] [v2][PATCH 2/5] hw:pci-host:piix: split i440fx_init

[Chen, Tiejun]

On 2014/7/31 23:44, Michael S. Tsirkin wrote:
> On Thu, Jul 31, 2014 at 06:12:32PM +0800, Chen, Tiejun wrote:
>> On 2014/7/31 17:53, Michael S. Tsirkin wrote:
>>> On Thu, Jul 31, 2014 at 05:26:41PM +0800, Chen, Tiejun wrote:
>>>> On 2014/7/31 17:10, Michael S. Tsirkin wrote:
>>>>> On Thu, Jul 31, 2014 at 02:31:36PM +0800, Tiejun Chen wrote:
>>>>>> We'd like to split i440fx_init and then we can share something
>>>>>> with other stuff.
>>>>>>
>>>>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>>>>
>>>>> I think this is too much work for very little benefit.
>>>>> Just pass const char *type to i440fx_init.
>>>>
>>>> You know we will introduce that faked PCIe device represented that PCH
>>>> later,
>>>
>>> Later when? On top of this patch series? Would like to see it all
>>> before applying this ...
>>
>> I will send this with other IGD stuff after this series is fine to you since
>> its just creating a simple PCIe device.
>>
>> I think you should know this whole story since as you guys discussed we
>> don't fix that PCH at 1f.0.
>
>
> And this works with all legacy drivers and with windows
> drivers?

If we want to support these drivers, we still need to fix PCH at 1f.0.

I think we can discuss this in detail when I send patch to address this 
point later. But now I don't think that block this patches series since 
this is about how to do *inside* that creating PCIe device subfunction.

This is also why I try to the whole IGD patches, respectively since they 
always bring more comments or discussion, then it will be difficult to 
cover them at the same time by myself.

I hope I can do this step by step address all your comments until you 
guys expect :)

Thanks
Tiejun

>
>
>> So it may be like this,
>>
>> static int create_pseudo_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice *hdev)
>> {
>>
>> 	struct PCIDevice *dev;
>>
>> 	char rid;
>>
>> 	/* We havt to use a simple PCI device to fake this ISA bridge
>> 	 * to avoid making some confusion to BIOS and ACPI.
>> 	 */
>> 	dev = pci_create(bus, -1, "pseudo-intel-pch-isa-bridge");
>>
>> 	qdev_init_nofail(&dev->qdev);
>> 	pci_config_set_vendor_id(dev->config, XEN_SUBSYSTEM_ID));
>> 						^
>> 				I don't remember this exactly.
>> 	pci_config_set_device_id(dev->config, hdev->device_id);
>
> This is a hack anyway, how about reverse-decoding
> required device id from the supplied card instead of
> poking at the host?
> You can do it when card is initialized.
>
>>
>> 	return 0;
>> }
>>>
>>>> so how to distinguish them? Are you saying I should check the type
>>>> like this?
>>>>
>>>> if(Xen-Type)
>>>> {}
>>>> else
>>>> {}
>>>>
>>>
>>> No! Put the code in init function for the respective class,
>>> pass type as an argument:
>>
>> If you mean we don't introduce any "if/else", I still don't understand how
>> to insert such that function above, could you show this exactly?
>>
>> Tiejun
>
> Stick it in the constructor for your xen pt device.
>
>>>
>>> ----
>>> i440fx: make types configurable at run-time
>>>
>>> Xen wants to supply a different pci and host devices,
>>> inheriting i440fx devices. Make types configurable.
>>>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>
>>> -->
>>>
>>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>>> index be8fdfe..86f295a 100644
>>> --- a/include/hw/i386/pc.h
>>> +++ b/include/hw/i386/pc.h
>>> @@ -230,7 +230,11 @@ extern int no_hpet;
>>>   struct PCII440FXState;
>>>   typedef struct PCII440FXState PCII440FXState;
>>>
>>> -PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
>>> +#define TYPE_I440FX_PCI_HOST_BRIDGE "i440FX-pcihost"
>>> +#define TYPE_I440FX_PCI_DEVICE "i440FX"
>>> +
>>> +PCIBus *i440fx_init(const char *host_type, const char *pci_type,
>>> +                    PCII440FXState **pi440fx_state, int *piix_devfn,
>>>                       ISABus **isa_bus, qemu_irq *pic,
>>>                       MemoryRegion *address_space_mem,
>>>                       MemoryRegion *address_space_io,
>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>>> index 31125b7..e0979cd 100644
>>> --- a/hw/i386/pc_piix.c
>>> +++ b/hw/i386/pc_piix.c
>>> @@ -194,7 +194,9 @@ static void pc_init1(MachineState *machine,
>>>       }
>>>
>>>       if (pci_enabled) {
>>> -        pci_bus = i440fx_init(&i440fx_state, &piix3_devfn, &isa_bus, gsi,
>>> +        pci_bus = i440fx_init(TYPE_I440FX_PCI_HOST_BRIDGE,
>>> +                              TYPE_I440FX_PCI_DEVICE,
>>> +                              &i440fx_state, &piix3_devfn, &isa_bus, gsi,
>>>                                 system_memory, system_io, machine->ram_size,
>>>                                 below_4g_mem_size,
>>>                                 above_4g_mem_size,
>>> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
>>> index e0e0946..0cd82b8 100644
>>> --- a/hw/pci-host/piix.c
>>> +++ b/hw/pci-host/piix.c
>>> @@ -40,7 +40,6 @@
>>>    * http://download.intel.com/design/chipsets/datashts/29054901.pdf
>>>    */
>>>
>>> -#define TYPE_I440FX_PCI_HOST_BRIDGE "i440FX-pcihost"
>>>   #define I440FX_PCI_HOST_BRIDGE(obj) \
>>>       OBJECT_CHECK(I440FXState, (obj), TYPE_I440FX_PCI_HOST_BRIDGE)
>>>
>>> @@ -91,7 +90,6 @@ typedef struct PIIX3State {
>>>       MemoryRegion rcr_mem;
>>>   } PIIX3State;
>>>
>>> -#define TYPE_I440FX_PCI_DEVICE "i440FX"
>>>   #define I440FX_PCI_DEVICE(obj) \
>>>       OBJECT_CHECK(PCII440FXState, (obj), TYPE_I440FX_PCI_DEVICE)
>>>
>>> @@ -305,7 +303,8 @@ static int i440fx_initfn(PCIDevice *dev)
>>>       return 0;
>>>   }
>>>
>>> -PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
>>> +PCIBus *i440fx_init(const char *host_type, const char *pci_type,
>>> +                    PCII440FXState **pi440fx_state,
>>>                       int *piix3_devfn,
>>>                       ISABus **isa_bus, qemu_irq *pic,
>>>                       MemoryRegion *address_space_mem,
>>> @@ -325,7 +324,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
>>>       unsigned i;
>>>       I440FXState *i440fx;
>>>
>>> -    dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST_BRIDGE);
>>> +    dev = qdev_create(NULL, host_type);
>>>       s = PCI_HOST_BRIDGE(dev);
>>>       b = pci_bus_new(dev, NULL, pci_address_space,
>>>                       address_space_io, 0, TYPE_PCI_BUS);
>>> @@ -333,7 +332,7 @@ 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);
>>> +    d = pci_create_simple(b, 0, pci_type);
>>>       *pi440fx_state = I440FX_PCI_DEVICE(d);
>>>       f = *pi440fx_state;
>>>       f->system_memory = address_space_mem;
>>>
>>>
>

Re: [v2][PATCH 2/5] hw:pci-host:piix: split i440fx_init

[Chen, Tiejun]

On 2014/7/31 23:44, Michael S. Tsirkin wrote:
> On Thu, Jul 31, 2014 at 06:12:32PM +0800, Chen, Tiejun wrote:
>> On 2014/7/31 17:53, Michael S. Tsirkin wrote:
>>> On Thu, Jul 31, 2014 at 05:26:41PM +0800, Chen, Tiejun wrote:
>>>> On 2014/7/31 17:10, Michael S. Tsirkin wrote:
>>>>> On Thu, Jul 31, 2014 at 02:31:36PM +0800, Tiejun Chen wrote:
>>>>>> We'd like to split i440fx_init and then we can share something
>>>>>> with other stuff.
>>>>>>
>>>>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>>>>
>>>>> I think this is too much work for very little benefit.
>>>>> Just pass const char *type to i440fx_init.
>>>>
>>>> You know we will introduce that faked PCIe device represented that PCH
>>>> later,
>>>
>>> Later when? On top of this patch series? Would like to see it all
>>> before applying this ...
>>
>> I will send this with other IGD stuff after this series is fine to you since
>> its just creating a simple PCIe device.
>>
>> I think you should know this whole story since as you guys discussed we
>> don't fix that PCH at 1f.0.
>
>
> And this works with all legacy drivers and with windows
> drivers?

If we want to support these drivers, we still need to fix PCH at 1f.0.

I think we can discuss this in detail when I send patch to address this 
point later. But now I don't think that block this patches series since 
this is about how to do *inside* that creating PCIe device subfunction.

This is also why I try to the whole IGD patches, respectively since they 
always bring more comments or discussion, then it will be difficult to 
cover them at the same time by myself.

I hope I can do this step by step address all your comments until you 
guys expect :)

Thanks
Tiejun

>
>
>> So it may be like this,
>>
>> static int create_pseudo_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice *hdev)
>> {
>>
>> 	struct PCIDevice *dev;
>>
>> 	char rid;
>>
>> 	/* We havt to use a simple PCI device to fake this ISA bridge
>> 	 * to avoid making some confusion to BIOS and ACPI.
>> 	 */
>> 	dev = pci_create(bus, -1, "pseudo-intel-pch-isa-bridge");
>>
>> 	qdev_init_nofail(&dev->qdev);
>> 	pci_config_set_vendor_id(dev->config, XEN_SUBSYSTEM_ID));
>> 						^
>> 				I don't remember this exactly.
>> 	pci_config_set_device_id(dev->config, hdev->device_id);
>
> This is a hack anyway, how about reverse-decoding
> required device id from the supplied card instead of
> poking at the host?
> You can do it when card is initialized.
>
>>
>> 	return 0;
>> }
>>>
>>>> so how to distinguish them? Are you saying I should check the type
>>>> like this?
>>>>
>>>> if(Xen-Type)
>>>> {}
>>>> else
>>>> {}
>>>>
>>>
>>> No! Put the code in init function for the respective class,
>>> pass type as an argument:
>>
>> If you mean we don't introduce any "if/else", I still don't understand how
>> to insert such that function above, could you show this exactly?
>>
>> Tiejun
>
> Stick it in the constructor for your xen pt device.
>
>>>
>>> ----
>>> i440fx: make types configurable at run-time
>>>
>>> Xen wants to supply a different pci and host devices,
>>> inheriting i440fx devices. Make types configurable.
>>>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>
>>> -->
>>>
>>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>>> index be8fdfe..86f295a 100644
>>> --- a/include/hw/i386/pc.h
>>> +++ b/include/hw/i386/pc.h
>>> @@ -230,7 +230,11 @@ extern int no_hpet;
>>>   struct PCII440FXState;
>>>   typedef struct PCII440FXState PCII440FXState;
>>>
>>> -PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
>>> +#define TYPE_I440FX_PCI_HOST_BRIDGE "i440FX-pcihost"
>>> +#define TYPE_I440FX_PCI_DEVICE "i440FX"
>>> +
>>> +PCIBus *i440fx_init(const char *host_type, const char *pci_type,
>>> +                    PCII440FXState **pi440fx_state, int *piix_devfn,
>>>                       ISABus **isa_bus, qemu_irq *pic,
>>>                       MemoryRegion *address_space_mem,
>>>                       MemoryRegion *address_space_io,
>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>>> index 31125b7..e0979cd 100644
>>> --- a/hw/i386/pc_piix.c
>>> +++ b/hw/i386/pc_piix.c
>>> @@ -194,7 +194,9 @@ static void pc_init1(MachineState *machine,
>>>       }
>>>
>>>       if (pci_enabled) {
>>> -        pci_bus = i440fx_init(&i440fx_state, &piix3_devfn, &isa_bus, gsi,
>>> +        pci_bus = i440fx_init(TYPE_I440FX_PCI_HOST_BRIDGE,
>>> +                              TYPE_I440FX_PCI_DEVICE,
>>> +                              &i440fx_state, &piix3_devfn, &isa_bus, gsi,
>>>                                 system_memory, system_io, machine->ram_size,
>>>                                 below_4g_mem_size,
>>>                                 above_4g_mem_size,
>>> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
>>> index e0e0946..0cd82b8 100644
>>> --- a/hw/pci-host/piix.c
>>> +++ b/hw/pci-host/piix.c
>>> @@ -40,7 +40,6 @@
>>>    * http://download.intel.com/design/chipsets/datashts/29054901.pdf
>>>    */
>>>
>>> -#define TYPE_I440FX_PCI_HOST_BRIDGE "i440FX-pcihost"
>>>   #define I440FX_PCI_HOST_BRIDGE(obj) \
>>>       OBJECT_CHECK(I440FXState, (obj), TYPE_I440FX_PCI_HOST_BRIDGE)
>>>
>>> @@ -91,7 +90,6 @@ typedef struct PIIX3State {
>>>       MemoryRegion rcr_mem;
>>>   } PIIX3State;
>>>
>>> -#define TYPE_I440FX_PCI_DEVICE "i440FX"
>>>   #define I440FX_PCI_DEVICE(obj) \
>>>       OBJECT_CHECK(PCII440FXState, (obj), TYPE_I440FX_PCI_DEVICE)
>>>
>>> @@ -305,7 +303,8 @@ static int i440fx_initfn(PCIDevice *dev)
>>>       return 0;
>>>   }
>>>
>>> -PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
>>> +PCIBus *i440fx_init(const char *host_type, const char *pci_type,
>>> +                    PCII440FXState **pi440fx_state,
>>>                       int *piix3_devfn,
>>>                       ISABus **isa_bus, qemu_irq *pic,
>>>                       MemoryRegion *address_space_mem,
>>> @@ -325,7 +324,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
>>>       unsigned i;
>>>       I440FXState *i440fx;
>>>
>>> -    dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST_BRIDGE);
>>> +    dev = qdev_create(NULL, host_type);
>>>       s = PCI_HOST_BRIDGE(dev);
>>>       b = pci_bus_new(dev, NULL, pci_address_space,
>>>                       address_space_io, 0, TYPE_PCI_BUS);
>>> @@ -333,7 +332,7 @@ 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);
>>> +    d = pci_create_simple(b, 0, pci_type);
>>>       *pi440fx_state = I440FX_PCI_DEVICE(d);
>>>       f = *pi440fx_state;
>>>       f->system_memory = address_space_mem;
>>>
>>>
>

Re: [Qemu-devel] [v2][PATCH 2/5] hw:pci-host:piix: split i440fx_init

[Chen, Tiejun]

On 2014/7/31 17:53, Michael S. Tsirkin wrote:
> On Thu, Jul 31, 2014 at 05:26:41PM +0800, Chen, Tiejun wrote:
>> On 2014/7/31 17:10, Michael S. Tsirkin wrote:
>>> On Thu, Jul 31, 2014 at 02:31:36PM +0800, Tiejun Chen wrote:
>>>> We'd like to split i440fx_init and then we can share something
>>>> with other stuff.
>>>>
>>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>>
>>> I think this is too much work for very little benefit.
>>> Just pass const char *type to i440fx_init.
>>
>> You know we will introduce that faked PCIe device represented that PCH
>> later,
>
> Later when? On top of this patch series? Would like to see it all
> before applying this ...
>
>> so how to distinguish them? Are you saying I should check the type
>> like this?
>>
>> if(Xen-Type)
>> {}
>> else
>> {}
>>
>
> No! Put the code in init function for the respective class,
> pass type as an argument:
>
> ----
> i440fx: make types configurable at run-time
>
> Xen wants to supply a different pci and host devices,
> inheriting i440fx devices. Make types configurable.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Looks this is not enough as well since after we use patch #3 to 
introduce that host bridge,

@@ -93,6 +93,9 @@ typedef struct PIIX3State {
  #define I440FX_PCI_DEVICE(obj) \
      OBJECT_CHECK(PCII440FXState, (obj), TYPE_I440FX_PCI_DEVICE)

+#define XEN_IGD_PASSTHROUGH_I440FX_PCI_DEVICE(obj) \
+    OBJECT_CHECK(PCII440FXState, (obj), 
TYPE_XEN_IGD_PASSTHROUGH_I440FX_PCI_DEVICE)
+
  struct PCII440FXState {
      /*< private >*/
      PCIDevice parent_obj;

Inside i440fx_init(), we still need to replace that macro, 
I440FX_PCI_DEVICE, with XEN_IGD_PASSTHROUGH_I440FX_PCI_DEVICE in xenigd 
case.

So looks we'd better split that i440fx_init() to address this point.

Thanks
Tiejun

>
> -->
>
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index be8fdfe..86f295a 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -230,7 +230,11 @@ extern int no_hpet;
>   struct PCII440FXState;
>   typedef struct PCII440FXState PCII440FXState;
>
> -PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
> +#define TYPE_I440FX_PCI_HOST_BRIDGE "i440FX-pcihost"
> +#define TYPE_I440FX_PCI_DEVICE "i440FX"
> +
> +PCIBus *i440fx_init(const char *host_type, const char *pci_type,
> +                    PCII440FXState **pi440fx_state, int *piix_devfn,
>                       ISABus **isa_bus, qemu_irq *pic,
>                       MemoryRegion *address_space_mem,
>                       MemoryRegion *address_space_io,
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 31125b7..e0979cd 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -194,7 +194,9 @@ static void pc_init1(MachineState *machine,
>       }
>
>       if (pci_enabled) {
> -        pci_bus = i440fx_init(&i440fx_state, &piix3_devfn, &isa_bus, gsi,
> +        pci_bus = i440fx_init(TYPE_I440FX_PCI_HOST_BRIDGE,
> +                              TYPE_I440FX_PCI_DEVICE,
> +                              &i440fx_state, &piix3_devfn, &isa_bus, gsi,
>                                 system_memory, system_io, machine->ram_size,
>                                 below_4g_mem_size,
>                                 above_4g_mem_size,
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index e0e0946..0cd82b8 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -40,7 +40,6 @@
>    * http://download.intel.com/design/chipsets/datashts/29054901.pdf
>    */
>
> -#define TYPE_I440FX_PCI_HOST_BRIDGE "i440FX-pcihost"
>   #define I440FX_PCI_HOST_BRIDGE(obj) \
>       OBJECT_CHECK(I440FXState, (obj), TYPE_I440FX_PCI_HOST_BRIDGE)
>
> @@ -91,7 +90,6 @@ typedef struct PIIX3State {
>       MemoryRegion rcr_mem;
>   } PIIX3State;
>
> -#define TYPE_I440FX_PCI_DEVICE "i440FX"
>   #define I440FX_PCI_DEVICE(obj) \
>       OBJECT_CHECK(PCII440FXState, (obj), TYPE_I440FX_PCI_DEVICE)
>
> @@ -305,7 +303,8 @@ static int i440fx_initfn(PCIDevice *dev)
>       return 0;
>   }
>
> -PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> +PCIBus *i440fx_init(const char *host_type, const char *pci_type,
> +                    PCII440FXState **pi440fx_state,
>                       int *piix3_devfn,
>                       ISABus **isa_bus, qemu_irq *pic,
>                       MemoryRegion *address_space_mem,
> @@ -325,7 +324,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
>       unsigned i;
>       I440FXState *i440fx;
>
> -    dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST_BRIDGE);
> +    dev = qdev_create(NULL, host_type);
>       s = PCI_HOST_BRIDGE(dev);
>       b = pci_bus_new(dev, NULL, pci_address_space,
>                       address_space_io, 0, TYPE_PCI_BUS);
> @@ -333,7 +332,7 @@ 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);
> +    d = pci_create_simple(b, 0, pci_type);
>       *pi440fx_state = I440FX_PCI_DEVICE(d);
>       f = *pi440fx_state;
>       f->system_memory = address_space_mem;
>
>

Re: [v2][PATCH 2/5] hw:pci-host:piix: split i440fx_init

[Chen, Tiejun]

On 2014/7/31 17:53, Michael S. Tsirkin wrote:
> On Thu, Jul 31, 2014 at 05:26:41PM +0800, Chen, Tiejun wrote:
>> On 2014/7/31 17:10, Michael S. Tsirkin wrote:
>>> On Thu, Jul 31, 2014 at 02:31:36PM +0800, Tiejun Chen wrote:
>>>> We'd like to split i440fx_init and then we can share something
>>>> with other stuff.
>>>>
>>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>>
>>> I think this is too much work for very little benefit.
>>> Just pass const char *type to i440fx_init.
>>
>> You know we will introduce that faked PCIe device represented that PCH
>> later,
>
> Later when? On top of this patch series? Would like to see it all
> before applying this ...
>
>> so how to distinguish them? Are you saying I should check the type
>> like this?
>>
>> if(Xen-Type)
>> {}
>> else
>> {}
>>
>
> No! Put the code in init function for the respective class,
> pass type as an argument:
>
> ----
> i440fx: make types configurable at run-time
>
> Xen wants to supply a different pci and host devices,
> inheriting i440fx devices. Make types configurable.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Looks this is not enough as well since after we use patch #3 to 
introduce that host bridge,

@@ -93,6 +93,9 @@ typedef struct PIIX3State {
  #define I440FX_PCI_DEVICE(obj) \
      OBJECT_CHECK(PCII440FXState, (obj), TYPE_I440FX_PCI_DEVICE)

+#define XEN_IGD_PASSTHROUGH_I440FX_PCI_DEVICE(obj) \
+    OBJECT_CHECK(PCII440FXState, (obj), 
TYPE_XEN_IGD_PASSTHROUGH_I440FX_PCI_DEVICE)
+
  struct PCII440FXState {
      /*< private >*/
      PCIDevice parent_obj;

Inside i440fx_init(), we still need to replace that macro, 
I440FX_PCI_DEVICE, with XEN_IGD_PASSTHROUGH_I440FX_PCI_DEVICE in xenigd 
case.

So looks we'd better split that i440fx_init() to address this point.

Thanks
Tiejun

>
> -->
>
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index be8fdfe..86f295a 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -230,7 +230,11 @@ extern int no_hpet;
>   struct PCII440FXState;
>   typedef struct PCII440FXState PCII440FXState;
>
> -PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
> +#define TYPE_I440FX_PCI_HOST_BRIDGE "i440FX-pcihost"
> +#define TYPE_I440FX_PCI_DEVICE "i440FX"
> +
> +PCIBus *i440fx_init(const char *host_type, const char *pci_type,
> +                    PCII440FXState **pi440fx_state, int *piix_devfn,
>                       ISABus **isa_bus, qemu_irq *pic,
>                       MemoryRegion *address_space_mem,
>                       MemoryRegion *address_space_io,
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 31125b7..e0979cd 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -194,7 +194,9 @@ static void pc_init1(MachineState *machine,
>       }
>
>       if (pci_enabled) {
> -        pci_bus = i440fx_init(&i440fx_state, &piix3_devfn, &isa_bus, gsi,
> +        pci_bus = i440fx_init(TYPE_I440FX_PCI_HOST_BRIDGE,
> +                              TYPE_I440FX_PCI_DEVICE,
> +                              &i440fx_state, &piix3_devfn, &isa_bus, gsi,
>                                 system_memory, system_io, machine->ram_size,
>                                 below_4g_mem_size,
>                                 above_4g_mem_size,
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index e0e0946..0cd82b8 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -40,7 +40,6 @@
>    * http://download.intel.com/design/chipsets/datashts/29054901.pdf
>    */
>
> -#define TYPE_I440FX_PCI_HOST_BRIDGE "i440FX-pcihost"
>   #define I440FX_PCI_HOST_BRIDGE(obj) \
>       OBJECT_CHECK(I440FXState, (obj), TYPE_I440FX_PCI_HOST_BRIDGE)
>
> @@ -91,7 +90,6 @@ typedef struct PIIX3State {
>       MemoryRegion rcr_mem;
>   } PIIX3State;
>
> -#define TYPE_I440FX_PCI_DEVICE "i440FX"
>   #define I440FX_PCI_DEVICE(obj) \
>       OBJECT_CHECK(PCII440FXState, (obj), TYPE_I440FX_PCI_DEVICE)
>
> @@ -305,7 +303,8 @@ static int i440fx_initfn(PCIDevice *dev)
>       return 0;
>   }
>
> -PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> +PCIBus *i440fx_init(const char *host_type, const char *pci_type,
> +                    PCII440FXState **pi440fx_state,
>                       int *piix3_devfn,
>                       ISABus **isa_bus, qemu_irq *pic,
>                       MemoryRegion *address_space_mem,
> @@ -325,7 +324,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
>       unsigned i;
>       I440FXState *i440fx;
>
> -    dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST_BRIDGE);
> +    dev = qdev_create(NULL, host_type);
>       s = PCI_HOST_BRIDGE(dev);
>       b = pci_bus_new(dev, NULL, pci_address_space,
>                       address_space_io, 0, TYPE_PCI_BUS);
> @@ -333,7 +332,7 @@ 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);
> +    d = pci_create_simple(b, 0, pci_type);
>       *pi440fx_state = I440FX_PCI_DEVICE(d);
>       f = *pi440fx_state;
>       f->system_memory = address_space_mem;
>
>

[Qemu-devel] [v2][PATCH 3/5] xen:hw:pci-host:piix: create host bridge to passthrough

[Tiejun Chen]

Implement a pci host bridge specific to passthrough. Actually
this just inherits the standard one.

This is based on http://patchwork.ozlabs.org/patch/363810/.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 hw/pci-host/piix.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

v2:

* Just add prefix with XEN_IGD_PASSTHROUGH/xen_igd_passthrough

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index a9a5570..8414864 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -95,6 +95,10 @@ typedef struct PIIX3State {
 #define I440FX_PCI_DEVICE(obj) \
     OBJECT_CHECK(PCII440FXState, (obj), TYPE_I440FX_PCI_DEVICE)
 
+#define TYPE_XEN_IGD_PASSTHROUGH_I440FX_PCI_DEVICE "xen-igd-passthrough-i440FX"
+#define XEN_IGD_PASSTHROUGH_I440FX_PCI_DEVICE(obj) \
+    OBJECT_CHECK(PCII440FXState, (obj), TYPE_XEN_IGD_PASSTHROUGH_I440FX_PCI_DEVICE)
+
 struct PCII440FXState {
     /*< private >*/
     PCIDevice parent_obj;
@@ -305,6 +309,16 @@ static int i440fx_initfn(PCIDevice *dev)
     return 0;
 }
 
+static int xen_igd_passthrough_i440fx_initfn(PCIDevice *dev)
+{
+    PCII440FXState *d = XEN_IGD_PASSTHROUGH_I440FX_PCI_DEVICE(dev);
+
+    dev->config[I440FX_SMRAM] = 0x02;
+
+    cpu_smm_register(&i440fx_set_smm, d);
+    return 0;
+}
+
 static void i440fx_pci_host_create(DeviceState **dev_bridge,
                                    PCIBus **bus_bridge,
                                    MemoryRegion *address_space_io,
@@ -763,6 +777,33 @@ static const TypeInfo i440fx_info = {
     .class_init    = i440fx_class_init,
 };
 
+static void xen_igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+    k->init = xen_igd_passthrough_i440fx_initfn;
+    k->vendor_id = PCI_VENDOR_ID_INTEL;
+    k->device_id = PCI_DEVICE_ID_INTEL_82441;
+    k->revision = 0x02;
+    k->class_id = PCI_CLASS_BRIDGE_HOST;
+    dc->desc = "IGD PT 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 xen_igd_passthrough_i440fx_info = {
+    .name          = TYPE_XEN_IGD_PASSTHROUGH_I440FX_PCI_DEVICE,
+    .parent        = TYPE_PCI_DEVICE,
+    .instance_size = sizeof(PCII440FXState),
+    .class_init    = xen_igd_passthrough_i440fx_class_init,
+};
+
 static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge,
                                                 PCIBus *rootbus)
 {
@@ -804,6 +845,7 @@ static const TypeInfo i440fx_pcihost_info = {
 static void i440fx_register_types(void)
 {
     type_register_static(&i440fx_info);
+    type_register_static(&xen_igd_passthrough_i440fx_info);
     type_register_static(&piix3_info);
     type_register_static(&piix3_xen_info);
     type_register_static(&i440fx_pcihost_info);
-- 
1.9.1

[v2][PATCH 3/5] xen:hw:pci-host:piix: create host bridge to passthrough

[Tiejun Chen]

Implement a pci host bridge specific to passthrough. Actually
this just inherits the standard one.

This is based on http://patchwork.ozlabs.org/patch/363810/.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 hw/pci-host/piix.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

v2:

* Just add prefix with XEN_IGD_PASSTHROUGH/xen_igd_passthrough

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index a9a5570..8414864 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -95,6 +95,10 @@ typedef struct PIIX3State {
 #define I440FX_PCI_DEVICE(obj) \
     OBJECT_CHECK(PCII440FXState, (obj), TYPE_I440FX_PCI_DEVICE)
 
+#define TYPE_XEN_IGD_PASSTHROUGH_I440FX_PCI_DEVICE "xen-igd-passthrough-i440FX"
+#define XEN_IGD_PASSTHROUGH_I440FX_PCI_DEVICE(obj) \
+    OBJECT_CHECK(PCII440FXState, (obj), TYPE_XEN_IGD_PASSTHROUGH_I440FX_PCI_DEVICE)
+
 struct PCII440FXState {
     /*< private >*/
     PCIDevice parent_obj;
@@ -305,6 +309,16 @@ static int i440fx_initfn(PCIDevice *dev)
     return 0;
 }
 
+static int xen_igd_passthrough_i440fx_initfn(PCIDevice *dev)
+{
+    PCII440FXState *d = XEN_IGD_PASSTHROUGH_I440FX_PCI_DEVICE(dev);
+
+    dev->config[I440FX_SMRAM] = 0x02;
+
+    cpu_smm_register(&i440fx_set_smm, d);
+    return 0;
+}
+
 static void i440fx_pci_host_create(DeviceState **dev_bridge,
                                    PCIBus **bus_bridge,
                                    MemoryRegion *address_space_io,
@@ -763,6 +777,33 @@ static const TypeInfo i440fx_info = {
     .class_init    = i440fx_class_init,
 };
 
+static void xen_igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+    k->init = xen_igd_passthrough_i440fx_initfn;
+    k->vendor_id = PCI_VENDOR_ID_INTEL;
+    k->device_id = PCI_DEVICE_ID_INTEL_82441;
+    k->revision = 0x02;
+    k->class_id = PCI_CLASS_BRIDGE_HOST;
+    dc->desc = "IGD PT 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 xen_igd_passthrough_i440fx_info = {
+    .name          = TYPE_XEN_IGD_PASSTHROUGH_I440FX_PCI_DEVICE,
+    .parent        = TYPE_PCI_DEVICE,
+    .instance_size = sizeof(PCII440FXState),
+    .class_init    = xen_igd_passthrough_i440fx_class_init,
+};
+
 static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge,
                                                 PCIBus *rootbus)
 {
@@ -804,6 +845,7 @@ static const TypeInfo i440fx_pcihost_info = {
 static void i440fx_register_types(void)
 {
     type_register_static(&i440fx_info);
+    type_register_static(&xen_igd_passthrough_i440fx_info);
     type_register_static(&piix3_info);
     type_register_static(&piix3_xen_info);
     type_register_static(&i440fx_pcihost_info);
-- 
1.9.1

[Qemu-devel] [v2][PATCH 4/5] xen:hw:pci-host:piix: introduce xen_igd_passthrough_i440fx_init

[Tiejun Chen]

This is almost same as an original i440fx_init but just
work with that xen igd host bridge to passthrough.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 hw/pci-host/piix.c   | 32 ++++++++++++++++++++++++++++++++
 include/hw/i386/pc.h | 11 +++++++++++
 2 files changed, 43 insertions(+)

v2:

* Based on patch #2 to regenerate

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 8414864..5977841 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -465,6 +465,38 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
     return bus_bridge;
 }
 
+PCIBus *xen_igd_passthrough_i440fx_init(PCII440FXState **pi440fx_state,
+                    int *piix3_devfn,
+                    ISABus **isa_bus,
+                    qemu_irq *pic,
+                    MemoryRegion *address_space_mem,
+                    MemoryRegion *address_space_io,
+                    ram_addr_t ram_size,
+                    ram_addr_t below_4g_mem_size,
+                    ram_addr_t above_4g_mem_size,
+                    MemoryRegion *pci_address_space,
+                    MemoryRegion *ram_memory)
+{
+    PCIDevice *d;
+    DeviceState *dev_bridge;
+    PCIBus *bus_bridge;
+
+    i440fx_pci_host_create(&dev_bridge, &bus_bridge, address_space_io,
+                           pci_address_space);
+
+    d = i440fx_pci0_create(&bus_bridge,
+                           TYPE_XEN_IGD_PASSTHROUGH_I440FX_PCI_DEVICE,
+                           pi440fx_state);
+
+    bus_bridge = i440fx_pci_setup(&dev_bridge, &bus_bridge, d, pi440fx_state,
+                                  piix3_devfn, isa_bus, pic, address_space_mem,
+                                  address_space_io, ram_size, below_4g_mem_size,
+                                  above_4g_mem_size, pci_address_space,
+                                  ram_memory);
+
+    return bus_bridge;
+}
+
 PCIBus *find_i440fx(void)
 {
     PCIHostState *s = OBJECT_CHECK(PCIHostState,
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index f4b9b2b..65b75a2 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -240,6 +240,17 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
                     MemoryRegion *pci_memory,
                     MemoryRegion *ram_memory);
 
+PCIBus *xen_igd_passthrough_i440fx_init(PCII440FXState **pi440fx_state,
+                                        int *piix_devfn,
+                                        ISABus **isa_bus, qemu_irq *pic,
+                                        MemoryRegion *address_space_mem,
+                                        MemoryRegion *address_space_io,
+                                        ram_addr_t ram_size,
+                                        ram_addr_t below_4g_mem_size,
+                                        ram_addr_t above_4g_mem_size,
+                                        MemoryRegion *pci_memory,
+                                        MemoryRegion *ram_memory);
+
 PCIBus *find_i440fx(void);
 /* piix4.c */
 extern PCIDevice *piix4_dev;
-- 
1.9.1

[v2][PATCH 4/5] xen:hw:pci-host:piix: introduce xen_igd_passthrough_i440fx_init

[Tiejun Chen]

This is almost same as an original i440fx_init but just
work with that xen igd host bridge to passthrough.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 hw/pci-host/piix.c   | 32 ++++++++++++++++++++++++++++++++
 include/hw/i386/pc.h | 11 +++++++++++
 2 files changed, 43 insertions(+)

v2:

* Based on patch #2 to regenerate

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 8414864..5977841 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -465,6 +465,38 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
     return bus_bridge;
 }
 
+PCIBus *xen_igd_passthrough_i440fx_init(PCII440FXState **pi440fx_state,
+                    int *piix3_devfn,
+                    ISABus **isa_bus,
+                    qemu_irq *pic,
+                    MemoryRegion *address_space_mem,
+                    MemoryRegion *address_space_io,
+                    ram_addr_t ram_size,
+                    ram_addr_t below_4g_mem_size,
+                    ram_addr_t above_4g_mem_size,
+                    MemoryRegion *pci_address_space,
+                    MemoryRegion *ram_memory)
+{
+    PCIDevice *d;
+    DeviceState *dev_bridge;
+    PCIBus *bus_bridge;
+
+    i440fx_pci_host_create(&dev_bridge, &bus_bridge, address_space_io,
+                           pci_address_space);
+
+    d = i440fx_pci0_create(&bus_bridge,
+                           TYPE_XEN_IGD_PASSTHROUGH_I440FX_PCI_DEVICE,
+                           pi440fx_state);
+
+    bus_bridge = i440fx_pci_setup(&dev_bridge, &bus_bridge, d, pi440fx_state,
+                                  piix3_devfn, isa_bus, pic, address_space_mem,
+                                  address_space_io, ram_size, below_4g_mem_size,
+                                  above_4g_mem_size, pci_address_space,
+                                  ram_memory);
+
+    return bus_bridge;
+}
+
 PCIBus *find_i440fx(void)
 {
     PCIHostState *s = OBJECT_CHECK(PCIHostState,
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index f4b9b2b..65b75a2 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -240,6 +240,17 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
                     MemoryRegion *pci_memory,
                     MemoryRegion *ram_memory);
 
+PCIBus *xen_igd_passthrough_i440fx_init(PCII440FXState **pi440fx_state,
+                                        int *piix_devfn,
+                                        ISABus **isa_bus, qemu_irq *pic,
+                                        MemoryRegion *address_space_mem,
+                                        MemoryRegion *address_space_io,
+                                        ram_addr_t ram_size,
+                                        ram_addr_t below_4g_mem_size,
+                                        ram_addr_t above_4g_mem_size,
+                                        MemoryRegion *pci_memory,
+                                        MemoryRegion *ram_memory);
+
 PCIBus *find_i440fx(void);
 /* piix4.c */
 extern PCIDevice *piix4_dev;
-- 
1.9.1

[Qemu-devel] [v2][PATCH 5/5] xen:hw:i386:pc_piix: introduce new machine for IGD passthrough

[Tiejun Chen]

Now we can introduce a new machine, xenigd, specific to IGD
passthrough. This can avoid involving other common codes.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 hw/i386/pc_piix.c | 105 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 105 insertions(+)

v2:

* Unify prefix with XEN_IGD_PASSTHROUGH/xen_igd_passthrough like patch #3

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index ab380d5..8d1c2bb 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -230,6 +230,45 @@ static void pc_machine_pci_bus_init(MachineState *machine,
     *pc_isa_bus = isa_bus;
 }
 
+#ifdef CONFIG_XEN
+static void xen_igd_passthrough_pc_machine_pci_bus_init(MachineState *machine,
+                                                        int pci_enabled,
+                                                        PCII440FXState **pc_i440fx_state,
+                                                        int *pc_piix3_devfn,
+                                                        PCIBus **pc_pci_bus,
+                                                        ISABus **pc_isa_bus,
+                                                        qemu_irq *gsi,
+                                                        MemoryRegion *pci_memory,
+                                                        MemoryRegion *ram_memory)
+{
+    MemoryRegion *system_memory = get_system_memory();
+    MemoryRegion *system_io = get_system_io();
+    PCII440FXState *i440fx_state;
+    int piix3_devfn;
+    PCIBus *pci_bus;
+    ISABus *isa_bus;
+
+    if (pci_enabled) {
+        pci_bus = xen_igd_passthrough_i440fx_init(&i440fx_state, &piix3_devfn,
+                                                  &isa_bus, gsi, system_memory,
+                                                  system_io, machine->ram_size,
+                                                  below_4g_mem_size,
+                                                  above_4g_mem_size,
+                                                  pci_memory, ram_memory);
+    } else {
+        pci_bus = NULL;
+        i440fx_state = NULL;
+        isa_bus = isa_bus_new(NULL, system_io);
+        no_hpet = 1;
+    }
+
+    *pc_i440fx_state = i440fx_state;
+    *pc_piix3_devfn = piix3_devfn;
+    *pc_pci_bus = pci_bus;
+    *pc_isa_bus = isa_bus;
+}
+#endif
+
 static void pc_machine_device_init(MachineState *machine,
                                    int pci_enabled,
                                    GSIState *gsi_state,
@@ -366,6 +405,38 @@ static void pc_init_pci(MachineState *machine)
     pc_init1(machine, 1, 1);
 }
 
+#ifdef CONFIG_XEN
+static void xen_igd_passthrough_pc_init1(MachineState *machine,
+                                         int pci_enabled,
+                                         int kvmclock_enabled)
+{
+    PCIBus *pci_bus = NULL;
+    ISABus *isa_bus = NULL;
+    PCII440FXState *i440fx_state = NULL;
+    int piix3_devfn = -1;
+    qemu_irq *gsi = NULL;
+    GSIState *gsi_state = NULL;
+    MemoryRegion *ram_memory = NULL;
+    MemoryRegion *pci_memory = NULL;
+    DeviceState *icc_bridge = NULL;
+    FWCfgState *fw_cfg = NULL;
+
+    pc_machine_base_init(machine, pci_enabled, kvmclock_enabled, &icc_bridge,
+                         &ram_memory, &pci_memory, &gsi, &gsi_state, &fw_cfg);
+    xen_igd_passthrough_pc_machine_pci_bus_init(machine, pci_enabled,
+                                                &i440fx_state, &piix3_devfn,
+                                                &pci_bus, &isa_bus, gsi,
+                                                pci_memory, ram_memory);
+    pc_machine_device_init(machine, pci_enabled, gsi_state, icc_bridge,
+                           piix3_devfn, fw_cfg, pci_bus, isa_bus, gsi);
+}
+
+static void xen_igd_passthrough_pc_init_pci(MachineState *machine)
+{
+    xen_igd_passthrough_pc_init1(machine, 1, 1);
+}
+#endif
+
 static void pc_compat_2_0(MachineState *machine)
 {
     /* This value depends on the actual DSDT and SSDT compiled into
@@ -512,6 +583,18 @@ static void pc_xen_hvm_init(MachineState *machine)
         pci_create_simple(bus, -1, "xen-platform");
     }
 }
+
+static void xen_igd_passthrough_pc_hvm_init(MachineState *machine)
+{
+    PCIBus *bus;
+
+    xen_igd_passthrough_pc_init_pci(machine);
+
+    bus = pci_find_primary_bus();
+    if (bus != NULL) {
+        pci_create_simple(bus, -1, "xen-platform");
+    }
+}
 #endif
 
 #define PC_I440FX_MACHINE_OPTIONS \
@@ -961,6 +1044,27 @@ static QEMUMachine xenfv_machine = {
         { /* end of list */ }
     },
 };
+
+static QEMUMachine xenigd_machine = {
+    PC_COMMON_MACHINE_OPTIONS,
+    .name = "xenigd",
+    .desc = "Xen Fully-virtualized PC specific to IGD",
+    .init = xen_igd_passthrough_pc_hvm_init,
+    .max_cpus = HVM_MAX_VCPUS,
+    .default_machine_opts = "accel=xen",
+    .hot_add_cpu = pc_hot_add_cpu,
+    .compat_props = (GlobalProperty[]) {
+        /* xenfv has no fwcfg and so does not load acpi from QEMU.
+         * as such new acpi features don't work.
+         */
+        {
+            .driver   = "PIIX4_PM",
+            .property = "acpi-pci-hotplug-with-bridge-support",
+            .value    = "off",
+        },
+        { /* end of list */ }
+    },
+};
 #endif
 
 static void pc_machine_init(void)
@@ -984,6 +1088,7 @@ static void pc_machine_init(void)
     qemu_register_pc_machine(&isapc_machine);
 #ifdef CONFIG_XEN
     qemu_register_pc_machine(&xenfv_machine);
+    qemu_register_pc_machine(&xenigd_machine);
 #endif
 }
 
-- 
1.9.1

[v2][PATCH 5/5] xen:hw:i386:pc_piix: introduce new machine for IGD passthrough

[Tiejun Chen]

Now we can introduce a new machine, xenigd, specific to IGD
passthrough. This can avoid involving other common codes.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 hw/i386/pc_piix.c | 105 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 105 insertions(+)

v2:

* Unify prefix with XEN_IGD_PASSTHROUGH/xen_igd_passthrough like patch #3

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index ab380d5..8d1c2bb 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -230,6 +230,45 @@ static void pc_machine_pci_bus_init(MachineState *machine,
     *pc_isa_bus = isa_bus;
 }
 
+#ifdef CONFIG_XEN
+static void xen_igd_passthrough_pc_machine_pci_bus_init(MachineState *machine,
+                                                        int pci_enabled,
+                                                        PCII440FXState **pc_i440fx_state,
+                                                        int *pc_piix3_devfn,
+                                                        PCIBus **pc_pci_bus,
+                                                        ISABus **pc_isa_bus,
+                                                        qemu_irq *gsi,
+                                                        MemoryRegion *pci_memory,
+                                                        MemoryRegion *ram_memory)
+{
+    MemoryRegion *system_memory = get_system_memory();
+    MemoryRegion *system_io = get_system_io();
+    PCII440FXState *i440fx_state;
+    int piix3_devfn;
+    PCIBus *pci_bus;
+    ISABus *isa_bus;
+
+    if (pci_enabled) {
+        pci_bus = xen_igd_passthrough_i440fx_init(&i440fx_state, &piix3_devfn,
+                                                  &isa_bus, gsi, system_memory,
+                                                  system_io, machine->ram_size,
+                                                  below_4g_mem_size,
+                                                  above_4g_mem_size,
+                                                  pci_memory, ram_memory);
+    } else {
+        pci_bus = NULL;
+        i440fx_state = NULL;
+        isa_bus = isa_bus_new(NULL, system_io);
+        no_hpet = 1;
+    }
+
+    *pc_i440fx_state = i440fx_state;
+    *pc_piix3_devfn = piix3_devfn;
+    *pc_pci_bus = pci_bus;
+    *pc_isa_bus = isa_bus;
+}
+#endif
+
 static void pc_machine_device_init(MachineState *machine,
                                    int pci_enabled,
                                    GSIState *gsi_state,
@@ -366,6 +405,38 @@ static void pc_init_pci(MachineState *machine)
     pc_init1(machine, 1, 1);
 }
 
+#ifdef CONFIG_XEN
+static void xen_igd_passthrough_pc_init1(MachineState *machine,
+                                         int pci_enabled,
+                                         int kvmclock_enabled)
+{
+    PCIBus *pci_bus = NULL;
+    ISABus *isa_bus = NULL;
+    PCII440FXState *i440fx_state = NULL;
+    int piix3_devfn = -1;
+    qemu_irq *gsi = NULL;
+    GSIState *gsi_state = NULL;
+    MemoryRegion *ram_memory = NULL;
+    MemoryRegion *pci_memory = NULL;
+    DeviceState *icc_bridge = NULL;
+    FWCfgState *fw_cfg = NULL;
+
+    pc_machine_base_init(machine, pci_enabled, kvmclock_enabled, &icc_bridge,
+                         &ram_memory, &pci_memory, &gsi, &gsi_state, &fw_cfg);
+    xen_igd_passthrough_pc_machine_pci_bus_init(machine, pci_enabled,
+                                                &i440fx_state, &piix3_devfn,
+                                                &pci_bus, &isa_bus, gsi,
+                                                pci_memory, ram_memory);
+    pc_machine_device_init(machine, pci_enabled, gsi_state, icc_bridge,
+                           piix3_devfn, fw_cfg, pci_bus, isa_bus, gsi);
+}
+
+static void xen_igd_passthrough_pc_init_pci(MachineState *machine)
+{
+    xen_igd_passthrough_pc_init1(machine, 1, 1);
+}
+#endif
+
 static void pc_compat_2_0(MachineState *machine)
 {
     /* This value depends on the actual DSDT and SSDT compiled into
@@ -512,6 +583,18 @@ static void pc_xen_hvm_init(MachineState *machine)
         pci_create_simple(bus, -1, "xen-platform");
     }
 }
+
+static void xen_igd_passthrough_pc_hvm_init(MachineState *machine)
+{
+    PCIBus *bus;
+
+    xen_igd_passthrough_pc_init_pci(machine);
+
+    bus = pci_find_primary_bus();
+    if (bus != NULL) {
+        pci_create_simple(bus, -1, "xen-platform");
+    }
+}
 #endif
 
 #define PC_I440FX_MACHINE_OPTIONS \
@@ -961,6 +1044,27 @@ static QEMUMachine xenfv_machine = {
         { /* end of list */ }
     },
 };
+
+static QEMUMachine xenigd_machine = {
+    PC_COMMON_MACHINE_OPTIONS,
+    .name = "xenigd",
+    .desc = "Xen Fully-virtualized PC specific to IGD",
+    .init = xen_igd_passthrough_pc_hvm_init,
+    .max_cpus = HVM_MAX_VCPUS,
+    .default_machine_opts = "accel=xen",
+    .hot_add_cpu = pc_hot_add_cpu,
+    .compat_props = (GlobalProperty[]) {
+        /* xenfv has no fwcfg and so does not load acpi from QEMU.
+         * as such new acpi features don't work.
+         */
+        {
+            .driver   = "PIIX4_PM",
+            .property = "acpi-pci-hotplug-with-bridge-support",
+            .value    = "off",
+        },
+        { /* end of list */ }
+    },
+};
 #endif
 
 static void pc_machine_init(void)
@@ -984,6 +1088,7 @@ static void pc_machine_init(void)
     qemu_register_pc_machine(&isapc_machine);
 #ifdef CONFIG_XEN
     qemu_register_pc_machine(&xenfv_machine);
+    qemu_register_pc_machine(&xenigd_machine);
 #endif
 }
 
-- 
1.9.1

Re: [Qemu-devel] [v2][PATCH 0/5] xen: introduce new machine for IGD passthrough

[Michael S. Tsirkin]

On Thu, Jul 31, 2014 at 02:31:34PM +0800, Tiejun Chen wrote:
> v2:
> 
> * Fix some coding style
> * New patch to separate i440fx_init
> * Just add prefix with XEN_IGD_PASSTHROUGH/xen_igd_passthrough
> * Based on patch #2 to regenerate
> * Unify prefix with XEN_IGD_PASSTHROUGH/xen_igd_passthrough like patch #3
> * Test: boot with a preinstalled ubuntu 14.04
>   ./i386-softmmu/qemu-system-i386 -hda test.img -m 2560 -boot c -machine pc

You need to test with windows guests as well, linux often can boot
on bad hardware.

> As we discussed we need to create a separate machine to support current
> IGD passthrough.
> 
> ----------------------------------------------------------------
> Tiejun Chen (5):
>       hw:i386:pc_piix: split pc_init1()
>       hw:pci-host:piix: split i440fx_init
>       xen:hw:pci-host:piix: create host bridge to passthrough
>       xen:hw:pci-host:piix: introduce xen_igd_passthrough_i440fx_init
>       xen:hw:i386:pc_piix: introduce new machine for IGD passthrough
> 
>  hw/i386/pc_piix.c    | 222 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------
>  hw/pci-host/piix.c   | 165 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------
>  include/hw/i386/pc.h |  11 +++++++++
>  3 files changed, 358 insertions(+), 40 deletions(-)
> 
> Thanks
> Tiejun

Re: [v2][PATCH 0/5] xen: introduce new machine for IGD passthrough

[Michael S. Tsirkin]

On Thu, Jul 31, 2014 at 02:31:34PM +0800, Tiejun Chen wrote:
> v2:
> 
> * Fix some coding style
> * New patch to separate i440fx_init
> * Just add prefix with XEN_IGD_PASSTHROUGH/xen_igd_passthrough
> * Based on patch #2 to regenerate
> * Unify prefix with XEN_IGD_PASSTHROUGH/xen_igd_passthrough like patch #3
> * Test: boot with a preinstalled ubuntu 14.04
>   ./i386-softmmu/qemu-system-i386 -hda test.img -m 2560 -boot c -machine pc

You need to test with windows guests as well, linux often can boot
on bad hardware.

> As we discussed we need to create a separate machine to support current
> IGD passthrough.
> 
> ----------------------------------------------------------------
> Tiejun Chen (5):
>       hw:i386:pc_piix: split pc_init1()
>       hw:pci-host:piix: split i440fx_init
>       xen:hw:pci-host:piix: create host bridge to passthrough
>       xen:hw:pci-host:piix: introduce xen_igd_passthrough_i440fx_init
>       xen:hw:i386:pc_piix: introduce new machine for IGD passthrough
> 
>  hw/i386/pc_piix.c    | 222 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------
>  hw/pci-host/piix.c   | 165 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------
>  include/hw/i386/pc.h |  11 +++++++++
>  3 files changed, 358 insertions(+), 40 deletions(-)
> 
> Thanks
> Tiejun