Skip to contentSkip to conversation

xen: introduce new machine for IGD passthrough

PATCH Superseded Tiejun Chen 2 participants 17 messages Jul 31, 2014 – Aug 4, 2014

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Tiejun Chen

author
#0 lore raw

v2 cover xen: introduce new machine for IGD passthrough

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

author
#1 lore raw

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

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

Signed-off-by Tiejun Chen <[email protected]>
---
 hw/i386/pc_piix.c | 117 +++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 93 insertions(+), 24 deletions(-)

v2:

* Fix some coding style

hw/i386/pc_piix.c +93 -24
index 9694f88..ab380d5 100644
@@ -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);
Signature · 1 line
1.9.1
author
#2 lore raw

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

In reply to Tiejun Chen

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

Signed-off-by Tiejun Chen <[email protected]>
---
 hw/pci-host/piix.c | 91 ++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 75 insertions(+), 16 deletions(-)

v2:

* New patch to separate i440fx_init

hw/pci-host/piix.c +75 -16
index e0e0946..a9a5570 100644
@@ -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,
Signature · 1 line
1.9.1
#3 lore raw

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

Quoted 4 lines · Tiejun Chen

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

Signed-off-by Tiejun Chen <[email protected]>

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 <[email protected]>

--

include/hw/i386/pc.h +2 -1
index be8fdfe..72e612b 100644
@@ -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,
hw/i386/pc_piix.c +2 -1
index 31125b7..5fefde6 100644
@@ -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,
hw/pci-host/piix.c +3 -2
index e0e0946..f0c5b5d 100644
@@ -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);
author
#4 lore raw

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

Quoted 8 lines · Michael S. Tsirkin
Quoted 4 lines · Tiejun Chen

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

Signed-off-by Tiejun Chen <[email protected]>

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

Quoted 65 lines

-->

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 <[email protected]>

--

include/hw/i386/pc.h +2 -1
index be8fdfe..72e612b 100644
@@ -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,
hw/i386/pc_piix.c +2 -1
index 31125b7..5fefde6 100644
@@ -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,
hw/pci-host/piix.c +3 -2
index e0e0946..f0c5b5d 100644
@@ -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);
#5 lore raw

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

Quoted 5 lines · Tiejun
Quoted 4 lines · Michael S. Tsirkin
Quoted 3 lines · Tiejun Chen

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

Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Signed-off-by Tiejun Chen <[email protected]>

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 ...

Quoted 8 lines

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 <[email protected]>

-->

include/hw/i386/pc.h +5 -1
index be8fdfe..86f295a 100644
@@ -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,
hw/i386/pc_piix.c +3 -1
index 31125b7..e0979cd 100644
@@ -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,
hw/pci-host/piix.c +4 -5
index e0e0946..0cd82b8 100644
@@ -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;
author
#6 lore raw

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

Quoted 8 lines · Michael S. Tsirkin
Quoted 7 lines · Tiejun
Quoted 6 lines · Michael S. Tsirkin
Quoted 4 lines · Tiejun Chen

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

Signed-off-by Tiejun Chen <[email protected]>
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

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;
}
Quoted 4 lines
Quoted 4 lines

so how to distinguish them? Are you saying I should check the type like this?

Quoted 8 lines
Quoted 5 lines

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

Quoted 93 lines

----
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 <[email protected]>

-->

include/hw/i386/pc.h +5 -1
index be8fdfe..86f295a 100644
@@ -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,
hw/i386/pc_piix.c +3 -1
index 31125b7..e0979cd 100644
@@ -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,
hw/pci-host/piix.c +4 -5
index e0e0946..0cd82b8 100644
@@ -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;
author
#7 lore raw

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

Quoted 56 lines · Tiejun
Quoted 8 lines · Michael S. Tsirkin
Quoted 7 lines · Tiejun
Quoted 6 lines · Michael S. Tsirkin
Quoted 4 lines · Tiejun Chen

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

Signed-off-by Tiejun Chen <[email protected]>
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

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;
}
Quoted 10 lines
Quoted 9 lines

so how to distinguish them? Are you saying I should check the type like this?

if(Xen-Type)
{}
else
{}

Quoted 6 lines
Quoted 2 lines

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

#8 lore raw

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

Quoted 64 lines · Tiejun
Quoted 56 lines · Tiejun
Quoted 8 lines · Michael S. Tsirkin
Quoted 7 lines · Tiejun
Quoted 6 lines · Michael S. Tsirkin
Quoted 4 lines · Tiejun Chen

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

Signed-off-by Tiejun Chen <[email protected]>
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

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;
}
Quoted 10 lines
Quoted 9 lines

so how to distinguish them? Are you saying I should check the type like this?

if(Xen-Type)
{}
else
{}

Quoted 6 lines
Quoted 2 lines

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?

Quoted 7 lines

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.

Signature · 1 line
MST
author
#9 lore raw

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

Quoted 70 lines · Michael S. Tsirkin
Quoted 69 lines · Tiejun
Quoted 56 lines · Tiejun
Quoted 8 lines · Michael S. Tsirkin
Quoted 7 lines · Tiejun
Quoted 6 lines · Michael S. Tsirkin
Quoted 4 lines · Tiejun Chen

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

Signed-off-by Tiejun Chen <[email protected]>
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

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;
}
Quoted 10 lines
Quoted 9 lines

so how to distinguish them? Are you saying I should check the type like this?

if(Xen-Type)
{}
else
{}

Quoted 6 lines
Quoted 2 lines

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.

Quoted 8 lines
Quoted 2 lines

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

#10 lore raw

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

In reply to Chen, Tiejun
Quoted 15 lines · Tiejun
Quoted 14 lines · Michael S. Tsirkin
Quoted 12 lines · Tiejun
Quoted 8 lines · Michael S. Tsirkin
Quoted 4 lines · Tiejun Chen

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

Signed-off-by Tiejun Chen <[email protected]>

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,

Quoted 8 lines
Quoted 2 lines

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?

Quoted 5 lines

So it may be like this,

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

Quoted 14 lines
	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.

Quoted 13 lines
	return 0;
}
Quoted 10 lines
Quoted 9 lines

so how to distinguish them? Are you saying I should check the type like this?

if(Xen-Type)
{}
else
{}

Quoted 7 lines
Quoted 2 lines

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.

Quoted 93 lines
Quoted 93 lines

----
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 <[email protected]>

-->

include/hw/i386/pc.h +5 -1
index be8fdfe..86f295a 100644
@@ -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,
hw/i386/pc_piix.c +3 -1
index 31125b7..e0979cd 100644
@@ -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,
hw/pci-host/piix.c +4 -5
index e0e0946..0cd82b8 100644
@@ -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;
author
#11 lore raw

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

Quoted 22 lines · Michael S. Tsirkin
Quoted 21 lines · Tiejun
Quoted 8 lines · Michael S. Tsirkin
Quoted 7 lines · Tiejun
Quoted 6 lines · Michael S. Tsirkin
Quoted 4 lines · Tiejun Chen

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

Signed-off-by Tiejun Chen <[email protected]>
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

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.

Quoted 6 lines
Quoted 2 lines

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

Quoted 144 lines
Quoted 7 lines

So it may be like this,

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

Quoted 14 lines
	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.

Quoted 13 lines
	return 0;
}
Quoted 10 lines
Quoted 9 lines

so how to distinguish them? Are you saying I should check the type like this?

if(Xen-Type)
{}
else
{}

Quoted 7 lines
Quoted 2 lines

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.

Quoted 93 lines
Quoted 93 lines

----
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 <[email protected]>

-->

include/hw/i386/pc.h +5 -1
index be8fdfe..86f295a 100644
@@ -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,
hw/i386/pc_piix.c +3 -1
index 31125b7..e0979cd 100644
@@ -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,
hw/pci-host/piix.c +4 -5
index e0e0946..0cd82b8 100644
@@ -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;
author
#12 lore raw

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

In reply to Michael S. Tsirkin
Quoted 29 lines · Michael S. Tsirkin
Quoted 5 lines · Tiejun
Quoted 4 lines · Michael S. Tsirkin
Quoted 3 lines · Tiejun Chen

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

Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Signed-off-by Tiejun Chen <[email protected]>

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 ...

Quoted 8 lines

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:

Quoted 7 lines

----
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 <[email protected]>

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

Quoted 85 lines

-->

include/hw/i386/pc.h +5 -1
index be8fdfe..86f295a 100644
@@ -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,
hw/i386/pc_piix.c +3 -1
index 31125b7..e0979cd 100644
@@ -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,
hw/pci-host/piix.c +4 -5
index e0e0946..0cd82b8 100644
@@ -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;
author
#13 lore raw

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

In reply to 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 <[email protected]>
---
 hw/pci-host/piix.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

v2:

* Just add prefix with XEN_IGD_PASSTHROUGH/xen_igd_passthrough

hw/pci-host/piix.c +42 -0
index a9a5570..8414864 100644
@@ -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);
Signature · 1 line
1.9.1
author
#14 lore raw

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

In reply to 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 <[email protected]>
---
 hw/pci-host/piix.c   | 32 ++++++++++++++++++++++++++++++++
 include/hw/i386/pc.h | 11 +++++++++++
 2 files changed, 43 insertions(+)

v2:

* Based on patch #2 to regenerate

hw/pci-host/piix.c +32 -0
index 8414864..5977841 100644
@@ -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,
include/hw/i386/pc.h +11 -0
index f4b9b2b..65b75a2 100644
@@ -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;
Signature · 1 line
1.9.1
author
#15 lore raw

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

In reply to 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 <[email protected]>
---
 hw/i386/pc_piix.c | 105 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 105 insertions(+)

v2:

* Unify prefix with XEN_IGD_PASSTHROUGH/xen_igd_passthrough like patch #3

hw/i386/pc_piix.c +105 -0
index ab380d5..8d1c2bb 100644
@@ -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
 }
Signature · 1 line
1.9.1
#16 lore raw
In reply to Tiejun Chen
Quoted 9 lines · 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

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

Quoted 18 lines

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