xen:passthrough: introduce a separate machine to igd passthrough

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 | 93 +++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 70 insertions(+), 23 deletions(-)

hw/i386/pc_piix.c +51 -23

index 7081c08..2391fda 100644
@@ -70,34 +70,21 @@ 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,
+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)
+                     int kvmclock_enabled,
+                     DeviceState *icc_bridge,
+                     MemoryRegion *ram_memory,
+                     MemoryRegion *pci_memory,
+                     qemu_irq *gsi,
+                     GSIState *gsi_state,
+                     FWCfgState *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;
 

@@ -190,6 +177,20 @@ static void pc_init1(MachineState *machine,
     } else {
         gsi = qemu_allocate_irqs(gsi_handler, gsi_state, GSI_NUM_PINS);
     }
+}
+
+static void pc_machine_pci_bus_init(MachineState *machine,
+                     int pci_enabled,
+                     PCII440FXState *i440fx_state,
+                     int piix3_devfn,
+                     PCIBus *pci_bus,
+                     ISABus *isa_bus,
+                     qemu_irq *gsi,
+                     MemoryRegion *pci_memory,
+                     MemoryRegion *ram_memory)
+{
+    MemoryRegion *system_memory = get_system_memory();
+    MemoryRegion *system_io = get_system_io();
 
     if (pci_enabled) {
         pci_bus = i440fx_init(&i440fx_state, &piix3_devfn, &isa_bus, gsi,

@@ -203,6 +204,28 @@ static void pc_init1(MachineState *machine,
         isa_bus = isa_bus_new(NULL, system_io);
         no_hpet = 1;
     }
+}
+
+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()) {

@@ -290,6 +313,30 @@ static void pc_init1(MachineState *machine,
     }
 }
 
+/* PC hardware initialisation */
+static void pc_init1(MachineState *machine,
+                     int pci_enabled,
+                     int kvmclock_enabled)
+{

hw/i386/pc_piix.c +9 -0

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

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 | 93 +++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 70 insertions(+), 23 deletions(-)

hw/i386/pc_piix.c +51 -23

index 7081c08..2391fda 100644
@@ -70,34 +70,21 @@ 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,
+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)
+                     int kvmclock_enabled,
+                     DeviceState *icc_bridge,
+                     MemoryRegion *ram_memory,
+                     MemoryRegion *pci_memory,
+                     qemu_irq *gsi,
+                     GSIState *gsi_state,
+                     FWCfgState *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;
 

@@ -190,6 +177,20 @@ static void pc_init1(MachineState *machine,
     } else {
         gsi = qemu_allocate_irqs(gsi_handler, gsi_state, GSI_NUM_PINS);
     }
+}
+
+static void pc_machine_pci_bus_init(MachineState *machine,
+                     int pci_enabled,
+                     PCII440FXState *i440fx_state,
+                     int piix3_devfn,
+                     PCIBus *pci_bus,
+                     ISABus *isa_bus,
+                     qemu_irq *gsi,
+                     MemoryRegion *pci_memory,
+                     MemoryRegion *ram_memory)
+{
+    MemoryRegion *system_memory = get_system_memory();
+    MemoryRegion *system_io = get_system_io();
 
     if (pci_enabled) {
         pci_bus = i440fx_init(&i440fx_state, &piix3_devfn, &isa_bus, gsi,

@@ -203,6 +204,28 @@ static void pc_init1(MachineState *machine,
         isa_bus = isa_bus_new(NULL, system_io);
         no_hpet = 1;
     }
+}
+
+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()) {

@@ -290,6 +313,30 @@ static void pc_init1(MachineState *machine,
     }
 }
 
+/* PC hardware initialisation */
+static void pc_init1(MachineState *machine,
+                     int pci_enabled,
+                     int kvmclock_enabled)
+{

hw/i386/pc_piix.c +9 -0

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

Quoted 4 lines · Tiejun Chen

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

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

Did you test this patch? It does not look like it can work.

Quoted 113 lines

---
  hw/i386/pc_piix.c | 93 +++++++++++++++++++++++++++++++++++++++++--------------
  1 file changed, 70 insertions(+), 23 deletions(-)

hw/i386/pc_piix.c +51 -23

index 7081c08..2391fda 100644
@@ -70,34 +70,21 @@ 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,
+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)
+                     int kvmclock_enabled,
+                     DeviceState *icc_bridge,
+                     MemoryRegion *ram_memory,
+                     MemoryRegion *pci_memory,
+                     qemu_irq *gsi,
+                     GSIState *gsi_state,
+                     FWCfgState *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;
 

@@ -190,6 +177,20 @@ static void pc_init1(MachineState *machine,
      } else {
          gsi = qemu_allocate_irqs(gsi_handler, gsi_state, GSI_NUM_PINS);
      }
+}
+
+static void pc_machine_pci_bus_init(MachineState *machine,
+                     int pci_enabled,
+                     PCII440FXState *i440fx_state,
+                     int piix3_devfn,
+                     PCIBus *pci_bus,
+                     ISABus *isa_bus,
+                     qemu_irq *gsi,
+                     MemoryRegion *pci_memory,
+                     MemoryRegion *ram_memory)
+{
+    MemoryRegion *system_memory = get_system_memory();
+    MemoryRegion *system_io = get_system_io();
 
      if (pci_enabled) {
          pci_bus = i440fx_init(&i440fx_state, &piix3_devfn, &isa_bus, gsi,

@@ -203,6 +204,28 @@ static void pc_init1(MachineState *machine,
          isa_bus = isa_bus_new(NULL, system_io);
          no_hpet = 1;
      }
+}
+
+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()) {

@@ -290,6 +313,30 @@ static void pc_init1(MachineState *machine,
      }
  }
 
+/* PC hardware initialisation */
+static void pc_init1(MachineState *machine,
+                     int pci_enabled,
+                     int kvmclock_enabled)
+{

hw/i386/pc_piix.c static void pc_init1(MachineState *machine, · line 330

Quoted 10 lines

hw/i386/pc_piix.c +10 -0

+    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;

These are set to NULL here and never modified below. Why does it make sense?

Quoted 14 lines

hw/i386/pc_piix.c +9 -1

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

Quoted 4 lines · Tiejun Chen

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

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

Did you test this patch? It does not look like it can work.

Quoted 113 lines

---
  hw/i386/pc_piix.c | 93 +++++++++++++++++++++++++++++++++++++++++--------------
  1 file changed, 70 insertions(+), 23 deletions(-)

hw/i386/pc_piix.c +51 -23

index 7081c08..2391fda 100644
@@ -70,34 +70,21 @@ 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,
+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)
+                     int kvmclock_enabled,
+                     DeviceState *icc_bridge,
+                     MemoryRegion *ram_memory,
+                     MemoryRegion *pci_memory,
+                     qemu_irq *gsi,
+                     GSIState *gsi_state,
+                     FWCfgState *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;
 

@@ -190,6 +177,20 @@ static void pc_init1(MachineState *machine,
      } else {
          gsi = qemu_allocate_irqs(gsi_handler, gsi_state, GSI_NUM_PINS);
      }
+}
+
+static void pc_machine_pci_bus_init(MachineState *machine,
+                     int pci_enabled,
+                     PCII440FXState *i440fx_state,
+                     int piix3_devfn,
+                     PCIBus *pci_bus,
+                     ISABus *isa_bus,
+                     qemu_irq *gsi,
+                     MemoryRegion *pci_memory,
+                     MemoryRegion *ram_memory)
+{
+    MemoryRegion *system_memory = get_system_memory();
+    MemoryRegion *system_io = get_system_io();
 
      if (pci_enabled) {
          pci_bus = i440fx_init(&i440fx_state, &piix3_devfn, &isa_bus, gsi,

@@ -203,6 +204,28 @@ static void pc_init1(MachineState *machine,
          isa_bus = isa_bus_new(NULL, system_io);
          no_hpet = 1;
      }
+}
+
+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()) {

@@ -290,6 +313,30 @@ static void pc_init1(MachineState *machine,
      }
  }
 
+/* PC hardware initialisation */
+static void pc_init1(MachineState *machine,
+                     int pci_enabled,
+                     int kvmclock_enabled)
+{

hw/i386/pc_piix.c static void pc_init1(MachineState *machine, · line 330

Quoted 10 lines

hw/i386/pc_piix.c +10 -0

+    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;

These are set to NULL here and never modified below. Why does it make sense?

Quoted 14 lines

hw/i386/pc_piix.c +9 -1

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

Quoted 7 lines · Michael S. Tsirkin

Quoted 4 lines · Tiejun Chen

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

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

Did you test this patch? It does not look like it can work.

Just compile but I think you're right. Here something is really wrong here, so this is my fault.

I will regenerate this patch and do test to double check.

Thanks
Tiejun

Quoted 143 lines

Quoted 113 lines

---
 hw/i386/pc_piix.c | 93 +++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 70 insertions(+), 23 deletions(-)

hw/i386/pc_piix.c +51 -23

index 7081c08..2391fda 100644
@@ -70,34 +70,21 @@ 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,
+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)
+                     int kvmclock_enabled,
+                     DeviceState *icc_bridge,
+                     MemoryRegion *ram_memory,
+                     MemoryRegion *pci_memory,
+                     qemu_irq *gsi,
+                     GSIState *gsi_state,
+                     FWCfgState *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;
 

@@ -190,6 +177,20 @@ static void pc_init1(MachineState *machine,
     } else {
         gsi = qemu_allocate_irqs(gsi_handler, gsi_state, GSI_NUM_PINS);
     }
+}
+
+static void pc_machine_pci_bus_init(MachineState *machine,
+                     int pci_enabled,
+                     PCII440FXState *i440fx_state,
+                     int piix3_devfn,
+                     PCIBus *pci_bus,
+                     ISABus *isa_bus,
+                     qemu_irq *gsi,
+                     MemoryRegion *pci_memory,
+                     MemoryRegion *ram_memory)
+{
+    MemoryRegion *system_memory = get_system_memory();
+    MemoryRegion *system_io = get_system_io();
 
     if (pci_enabled) {
         pci_bus = i440fx_init(&i440fx_state, &piix3_devfn, &isa_bus, gsi,

@@ -203,6 +204,28 @@ static void pc_init1(MachineState *machine,
         isa_bus = isa_bus_new(NULL, system_io);
         no_hpet = 1;
     }
+}
+
+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()) {

@@ -290,6 +313,30 @@ static void pc_init1(MachineState *machine,
     }
 }
 
+/* PC hardware initialisation */
+static void pc_init1(MachineState *machine,
+                     int pci_enabled,
+                     int kvmclock_enabled)
+{

hw/i386/pc_piix.c static void pc_init1(MachineState *machine, · line 330

Quoted 10 lines

hw/i386/pc_piix.c +10 -0

+    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;

These are set to NULL here and never modified below. Why does it make sense?

Quoted 14 lines

hw/i386/pc_piix.c +9 -1

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

Quoted 7 lines · Michael S. Tsirkin

Quoted 4 lines · Tiejun Chen

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

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

Did you test this patch? It does not look like it can work.

Just compile but I think you're right. Here something is really wrong here, so this is my fault.

I will regenerate this patch and do test to double check.

Thanks
Tiejun

Quoted 143 lines

Quoted 113 lines

---
 hw/i386/pc_piix.c | 93 +++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 70 insertions(+), 23 deletions(-)

hw/i386/pc_piix.c +51 -23

index 7081c08..2391fda 100644
@@ -70,34 +70,21 @@ 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,
+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)
+                     int kvmclock_enabled,
+                     DeviceState *icc_bridge,
+                     MemoryRegion *ram_memory,
+                     MemoryRegion *pci_memory,
+                     qemu_irq *gsi,
+                     GSIState *gsi_state,
+                     FWCfgState *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;
 

@@ -190,6 +177,20 @@ static void pc_init1(MachineState *machine,
     } else {
         gsi = qemu_allocate_irqs(gsi_handler, gsi_state, GSI_NUM_PINS);
     }
+}
+
+static void pc_machine_pci_bus_init(MachineState *machine,
+                     int pci_enabled,
+                     PCII440FXState *i440fx_state,
+                     int piix3_devfn,
+                     PCIBus *pci_bus,
+                     ISABus *isa_bus,
+                     qemu_irq *gsi,
+                     MemoryRegion *pci_memory,
+                     MemoryRegion *ram_memory)
+{
+    MemoryRegion *system_memory = get_system_memory();
+    MemoryRegion *system_io = get_system_io();
 
     if (pci_enabled) {
         pci_bus = i440fx_init(&i440fx_state, &piix3_devfn, &isa_bus, gsi,

@@ -203,6 +204,28 @@ static void pc_init1(MachineState *machine,
         isa_bus = isa_bus_new(NULL, system_io);
         no_hpet = 1;
     }
+}
+
+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()) {

@@ -290,6 +313,30 @@ static void pc_init1(MachineState *machine,
     }
 }
 
+/* PC hardware initialisation */
+static void pc_init1(MachineState *machine,
+                     int pci_enabled,
+                     int kvmclock_enabled)
+{

hw/i386/pc_piix.c static void pc_init1(MachineState *machine, · line 330

Quoted 10 lines

hw/i386/pc_piix.c +10 -0

+    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;

These are set to NULL here and never modified below. Why does it make sense?

Quoted 14 lines

hw/i386/pc_piix.c +9 -1

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

Implement that pci

host bridge to specific

to passthrough. Actually
this just inherit

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 | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

hw/pci-host/piix.c +0 -0

index e0e0946..9feddf5 100644

hw/pci-host/piix.c +42 -0

+#define TYPE_I440FX_XEN_PCI_DEVICE "i440FX-xen"
+#define I440FX_XEN_PCI_DEVICE(obj) \
+    OBJECT_CHECK(PCII440FXState, (obj), TYPE_I440FX_XEN_PCI_DEVICE)
+
 typedef struct I440FXState {
     PCIHostState parent_obj;
     PcPciInfo pci_info;

@@ -305,6 +310,16 @@ static int i440fx_initfn(PCIDevice *dev)
     return 0;
 }
 
+static int i440fx_xen_initfn(PCIDevice *dev)
+{
+    PCII440FXState *d = I440FX_XEN_PCI_DEVICE(dev);
+
+    dev->config[I440FX_SMRAM] = 0x02;
+
+    cpu_smm_register(&i440fx_set_smm, d);
+    return 0;
+}
+
 PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
                     int *piix3_devfn,
                     ISABus **isa_bus, qemu_irq *pic,

@@ -704,6 +719,33 @@ static const TypeInfo i440fx_info = {
     .class_init    = i440fx_class_init,
 };
 
+static void i440fx_xen_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+    k->init = i440fx_xen_initfn;
+    k->vendor_id = PCI_VENDOR_ID_INTEL;
+    k->device_id = PCI_DEVICE_ID_INTEL_82441;
+    k->revision = 0x02;
+    k->class_id = PCI_CLASS_BRIDGE_ISA;
+    dc->desc = "XEN Host bridge";
+    dc->vmsd = &vmstate_i440fx;
+    /*
+     * PCI-facing part of the host bridge, not usable without the
+     * host-facing part, which can't be device_add'ed, yet.
+     */
+    dc->cannot_instantiate_with_device_add_yet = true;
+    dc->hotpluggable   = false;
+}
+
+static const TypeInfo i440fx_xen_info = {
+    .name          = TYPE_I440FX_XEN_PCI_DEVICE,
+    .parent        = TYPE_PCI_DEVICE,
+    .instance_size = sizeof(PCII440FXState),
+    .class_init    = i440fx_xen_class_init,
+};
+
 static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge,
                                                 PCIBus *rootbus)
 {

@@ -745,6 +787,7 @@ static const TypeInfo i440fx_pcihost_info = {
 static void i440fx_register_types(void)
 {
     type_register_static(&i440fx_info);
+    type_register_static(&i440fx_xen_info);
     type_register_static(&piix3_info);
     type_register_static(&piix3_xen_info);
     type_register_static(&i440fx_pcihost_info);

Signature · 1 line

1.9.1

Implement that pci

host bridge to specific

to passthrough. Actually
this just inherit

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 | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

hw/pci-host/piix.c +0 -0

index e0e0946..9feddf5 100644

hw/pci-host/piix.c +42 -0

+#define TYPE_I440FX_XEN_PCI_DEVICE "i440FX-xen"
+#define I440FX_XEN_PCI_DEVICE(obj) \
+    OBJECT_CHECK(PCII440FXState, (obj), TYPE_I440FX_XEN_PCI_DEVICE)
+
 typedef struct I440FXState {
     PCIHostState parent_obj;
     PcPciInfo pci_info;

@@ -305,6 +310,16 @@ static int i440fx_initfn(PCIDevice *dev)
     return 0;
 }
 
+static int i440fx_xen_initfn(PCIDevice *dev)
+{
+    PCII440FXState *d = I440FX_XEN_PCI_DEVICE(dev);
+
+    dev->config[I440FX_SMRAM] = 0x02;
+
+    cpu_smm_register(&i440fx_set_smm, d);
+    return 0;
+}
+
 PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
                     int *piix3_devfn,
                     ISABus **isa_bus, qemu_irq *pic,

@@ -704,6 +719,33 @@ static const TypeInfo i440fx_info = {
     .class_init    = i440fx_class_init,
 };
 
+static void i440fx_xen_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+    k->init = i440fx_xen_initfn;
+    k->vendor_id = PCI_VENDOR_ID_INTEL;
+    k->device_id = PCI_DEVICE_ID_INTEL_82441;
+    k->revision = 0x02;
+    k->class_id = PCI_CLASS_BRIDGE_ISA;
+    dc->desc = "XEN Host bridge";
+    dc->vmsd = &vmstate_i440fx;
+    /*
+     * PCI-facing part of the host bridge, not usable without the
+     * host-facing part, which can't be device_add'ed, yet.
+     */
+    dc->cannot_instantiate_with_device_add_yet = true;
+    dc->hotpluggable   = false;
+}
+
+static const TypeInfo i440fx_xen_info = {
+    .name          = TYPE_I440FX_XEN_PCI_DEVICE,
+    .parent        = TYPE_PCI_DEVICE,
+    .instance_size = sizeof(PCII440FXState),
+    .class_init    = i440fx_xen_class_init,
+};
+
 static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge,
                                                 PCIBus *rootbus)
 {

@@ -745,6 +787,7 @@ static const TypeInfo i440fx_pcihost_info = {
 static void i440fx_register_types(void)
 {
     type_register_static(&i440fx_info);
+    type_register_static(&i440fx_xen_info);
     type_register_static(&piix3_info);
     type_register_static(&piix3_xen_info);
     type_register_static(&i440fx_pcihost_info);

Signature · 1 line

1.9.1

Quoted 1 line · Tiejun Chen

Implement that pci

s/that/a/

Quoted 2 lines

host bridge to specific

s/to specific/specific/

Quoted 3 lines

to passthrough. Actually
this just inherit

s/inherit/inherits/

Quoted 10 lines

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 | 43 +++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 43 insertions(+)

hw/pci-host/piix.c line 37

Quoted 9 lines

hw/pci-host/piix.c +1 -0

index e0e0946..9feddf5 100644
@@ -34,6 +34,7 @@
  #include "sysemu/sysemu.h"
  #include "hw/i386/ioapic.h"
  #include "qapi/visitor.h"
+#include "hw/xen/xen_pt.h"

What call needs this, exactly?

hw/pci-host/piix.c line 47

Quoted 8 lines

hw/pci-host/piix.c +0 -0

  /*
   * I440FX chipset data sheet.

@@ -44,6 +45,10 @@
  #define I440FX_PCI_HOST_BRIDGE(obj) \
      OBJECT_CHECK(I440FXState, (obj), TYPE_I440FX_PCI_HOST_BRIDGE)
 

OK cool, but don't you want to put "igd" and "passthrough" somewhere in the name? Maybe "legacy" as well since future drivers will work with existing machine type, right?

Applies to functions and macro names as well.

Quoted 68 lines

hw/pci-host/piix.c +42 -1

+#define TYPE_I440FX_XEN_PCI_DEVICE "i440FX-xen"
+#define I440FX_XEN_PCI_DEVICE(obj) \
+    OBJECT_CHECK(PCII440FXState, (obj), TYPE_I440FX_XEN_PCI_DEVICE)
+
  typedef struct I440FXState {
      PCIHostState parent_obj;
      PcPciInfo pci_info;

@@ -305,6 +310,16 @@ static int i440fx_initfn(PCIDevice *dev)
      return 0;
  }
 
+static int i440fx_xen_initfn(PCIDevice *dev)
+{
+    PCII440FXState *d = I440FX_XEN_PCI_DEVICE(dev);
+
+    dev->config[I440FX_SMRAM] = 0x02;
+
+    cpu_smm_register(&i440fx_set_smm, d);
+    return 0;
+}
+
  PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
                      int *piix3_devfn,
                      ISABus **isa_bus, qemu_irq *pic,

@@ -704,6 +719,33 @@ static const TypeInfo i440fx_info = {
      .class_init    = i440fx_class_init,
  };
 
+static void i440fx_xen_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+    k->init = i440fx_xen_initfn;
+    k->vendor_id = PCI_VENDOR_ID_INTEL;
+    k->device_id = PCI_DEVICE_ID_INTEL_82441;
+    k->revision = 0x02;
+    k->class_id = PCI_CLASS_BRIDGE_ISA;
+    dc->desc = "XEN Host bridge";
+    dc->vmsd = &vmstate_i440fx;
+    /*
+     * PCI-facing part of the host bridge, not usable without the
+     * host-facing part, which can't be device_add'ed, yet.
+     */
+    dc->cannot_instantiate_with_device_add_yet = true;
+    dc->hotpluggable   = false;
+}
+
+static const TypeInfo i440fx_xen_info = {
+    .name          = TYPE_I440FX_XEN_PCI_DEVICE,
+    .parent        = TYPE_PCI_DEVICE,
+    .instance_size = sizeof(PCII440FXState),
+    .class_init    = i440fx_xen_class_init,
+};
+
  static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge,
                                                  PCIBus *rootbus)
  {

@@ -745,6 +787,7 @@ static const TypeInfo i440fx_pcihost_info = {
  static void i440fx_register_types(void)
  {
      type_register_static(&i440fx_info);
+    type_register_static(&i440fx_xen_info);
      type_register_static(&piix3_info);
      type_register_static(&piix3_xen_info);
      type_register_static(&i440fx_pcihost_info);
--

1.9.1

Quoted 1 line · Tiejun Chen

Implement that pci

s/that/a/

Quoted 2 lines

host bridge to specific

s/to specific/specific/

Quoted 3 lines

to passthrough. Actually
this just inherit

s/inherit/inherits/

Quoted 10 lines

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 | 43 +++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 43 insertions(+)

hw/pci-host/piix.c line 37

Quoted 9 lines

hw/pci-host/piix.c +1 -0

index e0e0946..9feddf5 100644
@@ -34,6 +34,7 @@
  #include "sysemu/sysemu.h"
  #include "hw/i386/ioapic.h"
  #include "qapi/visitor.h"
+#include "hw/xen/xen_pt.h"

What call needs this, exactly?

hw/pci-host/piix.c line 47

Quoted 8 lines

hw/pci-host/piix.c +0 -0

  /*
   * I440FX chipset data sheet.

@@ -44,6 +45,10 @@
  #define I440FX_PCI_HOST_BRIDGE(obj) \
      OBJECT_CHECK(I440FXState, (obj), TYPE_I440FX_PCI_HOST_BRIDGE)
 

OK cool, but don't you want to put "igd" and "passthrough" somewhere in the name? Maybe "legacy" as well since future drivers will work with existing machine type, right?

Applies to functions and macro names as well.

Quoted 68 lines

hw/pci-host/piix.c +42 -1

+#define TYPE_I440FX_XEN_PCI_DEVICE "i440FX-xen"
+#define I440FX_XEN_PCI_DEVICE(obj) \
+    OBJECT_CHECK(PCII440FXState, (obj), TYPE_I440FX_XEN_PCI_DEVICE)
+
  typedef struct I440FXState {
      PCIHostState parent_obj;
      PcPciInfo pci_info;

@@ -305,6 +310,16 @@ static int i440fx_initfn(PCIDevice *dev)
      return 0;
  }
 
+static int i440fx_xen_initfn(PCIDevice *dev)
+{
+    PCII440FXState *d = I440FX_XEN_PCI_DEVICE(dev);
+
+    dev->config[I440FX_SMRAM] = 0x02;
+
+    cpu_smm_register(&i440fx_set_smm, d);
+    return 0;
+}
+
  PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
                      int *piix3_devfn,
                      ISABus **isa_bus, qemu_irq *pic,

@@ -704,6 +719,33 @@ static const TypeInfo i440fx_info = {
      .class_init    = i440fx_class_init,
  };
 
+static void i440fx_xen_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+    k->init = i440fx_xen_initfn;
+    k->vendor_id = PCI_VENDOR_ID_INTEL;
+    k->device_id = PCI_DEVICE_ID_INTEL_82441;
+    k->revision = 0x02;
+    k->class_id = PCI_CLASS_BRIDGE_ISA;
+    dc->desc = "XEN Host bridge";
+    dc->vmsd = &vmstate_i440fx;
+    /*
+     * PCI-facing part of the host bridge, not usable without the
+     * host-facing part, which can't be device_add'ed, yet.
+     */
+    dc->cannot_instantiate_with_device_add_yet = true;
+    dc->hotpluggable   = false;
+}
+
+static const TypeInfo i440fx_xen_info = {
+    .name          = TYPE_I440FX_XEN_PCI_DEVICE,
+    .parent        = TYPE_PCI_DEVICE,
+    .instance_size = sizeof(PCII440FXState),
+    .class_init    = i440fx_xen_class_init,
+};
+
  static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge,
                                                  PCIBus *rootbus)
  {

@@ -745,6 +787,7 @@ static const TypeInfo i440fx_pcihost_info = {
  static void i440fx_register_types(void)
  {
      type_register_static(&i440fx_info);
+    type_register_static(&i440fx_xen_info);
      type_register_static(&piix3_info);
      type_register_static(&piix3_xen_info);
      type_register_static(&i440fx_pcihost_info);
--

1.9.1

Quoted 4 lines · Michael S. Tsirkin

Quoted 1 line · Tiejun Chen

Implement that pci

s/that/a/

Fixed.

Quoted 4 lines

Quoted 2 lines

host bridge to specific

s/to specific/specific/

Fixed.

Quoted 5 lines

Quoted 3 lines

to passthrough. Actually
this just inherit

s/inherit/inherits/

Fixed.

Quoted 10 lines

Quoted 10 lines

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 | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

Quoted 11 lines

hw/pci-host/piix.c line 37

Quoted 9 lines

hw/pci-host/piix.c +1 -0

index e0e0946..9feddf5 100644
@@ -34,6 +34,7 @@
 #include "sysemu/sysemu.h"
 #include "hw/i386/ioapic.h"
 #include "qapi/visitor.h"
+#include "hw/xen/xen_pt.h"

What call needs this, exactly?

Another fault when I grab something from the original patches.

Quoted 12 lines

hw/pci-host/piix.c line 47

Quoted 8 lines

hw/pci-host/piix.c +0 -0

 /*
  * I440FX chipset data sheet.

@@ -44,6 +45,10 @@
 #define I440FX_PCI_HOST_BRIDGE(obj) \
     OBJECT_CHECK(I440FXState, (obj), TYPE_I440FX_PCI_HOST_BRIDGE)
 

OK cool, but don't you want to put "igd" and "passthrough" somewhere in the name? Maybe "legacy" as well since future drivers will work with existing machine type, right?

Looks good so what about this prefix, IGD_PT_XXX/igd_pt_xxx?

Quoted 73 lines

Applies to functions and macro names as well.

Quoted 68 lines

hw/pci-host/piix.c +42 -1

+#define TYPE_I440FX_XEN_PCI_DEVICE "i440FX-xen"
+#define I440FX_XEN_PCI_DEVICE(obj) \
+    OBJECT_CHECK(PCII440FXState, (obj), TYPE_I440FX_XEN_PCI_DEVICE)
+
 typedef struct I440FXState {
     PCIHostState parent_obj;
     PcPciInfo pci_info;

@@ -305,6 +310,16 @@ static int i440fx_initfn(PCIDevice *dev)
     return 0;
 }
 
+static int i440fx_xen_initfn(PCIDevice *dev)
+{
+    PCII440FXState *d = I440FX_XEN_PCI_DEVICE(dev);
+
+    dev->config[I440FX_SMRAM] = 0x02;
+
+    cpu_smm_register(&i440fx_set_smm, d);
+    return 0;
+}
+
 PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
                     int *piix3_devfn,
                     ISABus **isa_bus, qemu_irq *pic,

@@ -704,6 +719,33 @@ static const TypeInfo i440fx_info = {
     .class_init    = i440fx_class_init,
 };
 
+static void i440fx_xen_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+    k->init = i440fx_xen_initfn;
+    k->vendor_id = PCI_VENDOR_ID_INTEL;
+    k->device_id = PCI_DEVICE_ID_INTEL_82441;
+    k->revision = 0x02;
+    k->class_id = PCI_CLASS_BRIDGE_ISA;
+    dc->desc = "XEN Host bridge";
+    dc->vmsd = &vmstate_i440fx;
+    /*
+     * PCI-facing part of the host bridge, not usable without the
+     * host-facing part, which can't be device_add'ed, yet.
+     */
+    dc->cannot_instantiate_with_device_add_yet = true;
+    dc->hotpluggable   = false;
+}
+
+static const TypeInfo i440fx_xen_info = {
+    .name          = TYPE_I440FX_XEN_PCI_DEVICE,
+    .parent        = TYPE_PCI_DEVICE,
+    .instance_size = sizeof(PCII440FXState),
+    .class_init    = i440fx_xen_class_init,
+};
+
 static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge,
                                                 PCIBus *rootbus)
 {

@@ -745,6 +787,7 @@ static const TypeInfo i440fx_pcihost_info = {
 static void i440fx_register_types(void)
 {
     type_register_static(&i440fx_info);
+    type_register_static(&i440fx_xen_info);
     type_register_static(&piix3_info);
     type_register_static(&piix3_xen_info);
     type_register_static(&i440fx_pcihost_info);
--

1.9.1

Quoted 4 lines · Michael S. Tsirkin

Quoted 1 line · Tiejun Chen

Implement that pci

s/that/a/

Fixed.

Quoted 4 lines

Quoted 2 lines

host bridge to specific

s/to specific/specific/

Fixed.

Quoted 5 lines

Quoted 3 lines

to passthrough. Actually
this just inherit

s/inherit/inherits/

Fixed.

Quoted 10 lines

Quoted 10 lines

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 | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

Quoted 11 lines

hw/pci-host/piix.c line 37

Quoted 9 lines

hw/pci-host/piix.c +1 -0

index e0e0946..9feddf5 100644
@@ -34,6 +34,7 @@
 #include "sysemu/sysemu.h"
 #include "hw/i386/ioapic.h"
 #include "qapi/visitor.h"
+#include "hw/xen/xen_pt.h"

What call needs this, exactly?

Another fault when I grab something from the original patches.

Quoted 12 lines

hw/pci-host/piix.c line 47

Quoted 8 lines

hw/pci-host/piix.c +0 -0

 /*
  * I440FX chipset data sheet.

@@ -44,6 +45,10 @@
 #define I440FX_PCI_HOST_BRIDGE(obj) \
     OBJECT_CHECK(I440FXState, (obj), TYPE_I440FX_PCI_HOST_BRIDGE)
 

OK cool, but don't you want to put "igd" and "passthrough" somewhere in the name? Maybe "legacy" as well since future drivers will work with existing machine type, right?

Looks good so what about this prefix, IGD_PT_XXX/igd_pt_xxx?

Quoted 73 lines

Applies to functions and macro names as well.

Quoted 68 lines

hw/pci-host/piix.c +42 -1

+#define TYPE_I440FX_XEN_PCI_DEVICE "i440FX-xen"
+#define I440FX_XEN_PCI_DEVICE(obj) \
+    OBJECT_CHECK(PCII440FXState, (obj), TYPE_I440FX_XEN_PCI_DEVICE)
+
 typedef struct I440FXState {
     PCIHostState parent_obj;
     PcPciInfo pci_info;

@@ -305,6 +310,16 @@ static int i440fx_initfn(PCIDevice *dev)
     return 0;
 }
 
+static int i440fx_xen_initfn(PCIDevice *dev)
+{
+    PCII440FXState *d = I440FX_XEN_PCI_DEVICE(dev);
+
+    dev->config[I440FX_SMRAM] = 0x02;
+
+    cpu_smm_register(&i440fx_set_smm, d);
+    return 0;
+}
+
 PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
                     int *piix3_devfn,
                     ISABus **isa_bus, qemu_irq *pic,

@@ -704,6 +719,33 @@ static const TypeInfo i440fx_info = {
     .class_init    = i440fx_class_init,
 };
 
+static void i440fx_xen_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+    k->init = i440fx_xen_initfn;
+    k->vendor_id = PCI_VENDOR_ID_INTEL;
+    k->device_id = PCI_DEVICE_ID_INTEL_82441;
+    k->revision = 0x02;
+    k->class_id = PCI_CLASS_BRIDGE_ISA;
+    dc->desc = "XEN Host bridge";
+    dc->vmsd = &vmstate_i440fx;
+    /*
+     * PCI-facing part of the host bridge, not usable without the
+     * host-facing part, which can't be device_add'ed, yet.
+     */
+    dc->cannot_instantiate_with_device_add_yet = true;
+    dc->hotpluggable   = false;
+}
+
+static const TypeInfo i440fx_xen_info = {
+    .name          = TYPE_I440FX_XEN_PCI_DEVICE,
+    .parent        = TYPE_PCI_DEVICE,
+    .instance_size = sizeof(PCII440FXState),
+    .class_init    = i440fx_xen_class_init,
+};
+
 static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge,
                                                 PCIBus *rootbus)
 {

@@ -745,6 +787,7 @@ static const TypeInfo i440fx_pcihost_info = {
 static void i440fx_register_types(void)
 {
     type_register_static(&i440fx_info);
+    type_register_static(&i440fx_xen_info);
     type_register_static(&piix3_info);
     type_register_static(&piix3_xen_info);
     type_register_static(&i440fx_pcihost_info);
--

1.9.1

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   | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/i386/pc.h | 10 +++++++
 2 files changed, 89 insertions(+)

hw/pci-host/piix.c +68 -0

index 9feddf5..7ef08d7 100644
@@ -407,6 +407,85 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
     return b;
 }
 
+PCIBus *xen_igd_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)
+{
+    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);
+    b = pci_bus_new(dev, NULL, pci_address_space,
+                    address_space_io, 0, TYPE_PCI_BUS);
+    s->bus = b;
+    object_property_add_child(qdev_get_machine(), "i440fx", OBJECT(dev), NULL);
+    qdev_init_nofail(dev);
+
+    d = pci_create_simple(b, 0, TYPE_I440FX_XEN_PCI_DEVICE);
+    *pi440fx_state = I440FX_XEN_PCI_DEVICE(d);
+    f = *pi440fx_state;
+    f->system_memory = address_space_mem;
+    f->pci_address_space = pci_address_space;
+    f->ram_memory = ram_memory;
+
+    i440fx = I440FX_PCI_HOST_BRIDGE(dev);
+    i440fx->pci_info.w32.begin = below_4g_mem_size;
+
+    /* setup pci memory mapping */
+    pc_pci_as_mapping_init(OBJECT(f), f->system_memory,
+                           f->pci_address_space);
+
+    memory_region_init_alias(&f->smram_region, OBJECT(d), "smram-region",
+                             f->pci_address_space, 0xa0000, 0x20000);
+    memory_region_add_subregion_overlap(f->system_memory, 0xa0000,
+                                        &f->smram_region, 1);
+    memory_region_set_enabled(&f->smram_region, false);
+    init_pam(dev, f->ram_memory, f->system_memory, f->pci_address_space,
+             &f->pam_regions[0], PAM_BIOS_BASE, PAM_BIOS_SIZE);
+    for (i = 0; i < 12; ++i) {
+        init_pam(dev, f->ram_memory, f->system_memory, f->pci_address_space,
+                 &f->pam_regions[i+1], PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE,
+                 PAM_EXPAN_SIZE);
+    }
+
+    /* Xen supports additional interrupt routes from the PCI devices to
+     * the IOAPIC: the four pins of each PCI device on the bus are also
+     * connected to the IOAPIC directly.
+     * These additional routes can be discovered through ACPI. */
+    piix3 = DO_UPCAST(PIIX3State, dev,
+            pci_create_simple_multifunction(b, -1, true, "PIIX3-xen"));
+    pci_bus_irqs(b, xen_piix3_set_irq, xen_pci_slot_get_pirq,
+            piix3, XEN_PIIX_NUM_PIRQS);
+    piix3->pic = pic;
+    *isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(piix3), "isa.0"));
+
+    *piix3_devfn = piix3->dev.devfn;
+

hw/pci-host/piix.c +1 -0

+
 PCIBus *find_i440fx(void)
 {
     PCIHostState *s = OBJECT_CHECK(PCIHostState,

include/hw/i386/pc.h +10 -0

index 1c0c382..51656d9 100644
@@ -239,6 +239,16 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
                     MemoryRegion *pci_memory,
                     MemoryRegion *ram_memory);
 
+PCIBus *xen_igd_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

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   | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/i386/pc.h | 10 +++++++
 2 files changed, 89 insertions(+)

hw/pci-host/piix.c +68 -0

index 9feddf5..7ef08d7 100644
@@ -407,6 +407,85 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
     return b;
 }
 
+PCIBus *xen_igd_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)
+{
+    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);
+    b = pci_bus_new(dev, NULL, pci_address_space,
+                    address_space_io, 0, TYPE_PCI_BUS);
+    s->bus = b;
+    object_property_add_child(qdev_get_machine(), "i440fx", OBJECT(dev), NULL);
+    qdev_init_nofail(dev);
+
+    d = pci_create_simple(b, 0, TYPE_I440FX_XEN_PCI_DEVICE);
+    *pi440fx_state = I440FX_XEN_PCI_DEVICE(d);
+    f = *pi440fx_state;
+    f->system_memory = address_space_mem;
+    f->pci_address_space = pci_address_space;
+    f->ram_memory = ram_memory;
+
+    i440fx = I440FX_PCI_HOST_BRIDGE(dev);
+    i440fx->pci_info.w32.begin = below_4g_mem_size;
+
+    /* setup pci memory mapping */
+    pc_pci_as_mapping_init(OBJECT(f), f->system_memory,
+                           f->pci_address_space);
+
+    memory_region_init_alias(&f->smram_region, OBJECT(d), "smram-region",
+                             f->pci_address_space, 0xa0000, 0x20000);
+    memory_region_add_subregion_overlap(f->system_memory, 0xa0000,
+                                        &f->smram_region, 1);
+    memory_region_set_enabled(&f->smram_region, false);
+    init_pam(dev, f->ram_memory, f->system_memory, f->pci_address_space,
+             &f->pam_regions[0], PAM_BIOS_BASE, PAM_BIOS_SIZE);
+    for (i = 0; i < 12; ++i) {
+        init_pam(dev, f->ram_memory, f->system_memory, f->pci_address_space,
+                 &f->pam_regions[i+1], PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE,
+                 PAM_EXPAN_SIZE);
+    }
+
+    /* Xen supports additional interrupt routes from the PCI devices to
+     * the IOAPIC: the four pins of each PCI device on the bus are also
+     * connected to the IOAPIC directly.
+     * These additional routes can be discovered through ACPI. */
+    piix3 = DO_UPCAST(PIIX3State, dev,
+            pci_create_simple_multifunction(b, -1, true, "PIIX3-xen"));
+    pci_bus_irqs(b, xen_piix3_set_irq, xen_pci_slot_get_pirq,
+            piix3, XEN_PIIX_NUM_PIRQS);
+    piix3->pic = pic;
+    *isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(piix3), "isa.0"));
+
+    *piix3_devfn = piix3->dev.devfn;
+

hw/pci-host/piix.c +1 -0

+
 PCIBus *find_i440fx(void)
 {
     PCIHostState *s = OBJECT_CHECK(PCIHostState,

include/hw/i386/pc.h +10 -0

index 1c0c382..51656d9 100644
@@ -239,6 +239,16 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
                     MemoryRegion *pci_memory,
                     MemoryRegion *ram_memory);
 
+PCIBus *xen_igd_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

Quoted 89 lines · 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   | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++
  include/hw/i386/pc.h | 10 +++++++
  2 files changed, 89 insertions(+)

hw/pci-host/piix.c +72 -0

index 9feddf5..7ef08d7 100644
@@ -407,6 +407,85 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
      return b;
  }
 
+PCIBus *xen_igd_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)
+{
+    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);
+    b = pci_bus_new(dev, NULL, pci_address_space,
+                    address_space_io, 0, TYPE_PCI_BUS);
+    s->bus = b;
+    object_property_add_child(qdev_get_machine(), "i440fx", OBJECT(dev), NULL);
+    qdev_init_nofail(dev);
+
+    d = pci_create_simple(b, 0, TYPE_I440FX_XEN_PCI_DEVICE);
+    *pi440fx_state = I440FX_XEN_PCI_DEVICE(d);
+    f = *pi440fx_state;
+    f->system_memory = address_space_mem;
+    f->pci_address_space = pci_address_space;
+    f->ram_memory = ram_memory;
+
+    i440fx = I440FX_PCI_HOST_BRIDGE(dev);
+    i440fx->pci_info.w32.begin = below_4g_mem_size;
+
+    /* setup pci memory mapping */
+    pc_pci_as_mapping_init(OBJECT(f), f->system_memory,
+                           f->pci_address_space);
+
+    memory_region_init_alias(&f->smram_region, OBJECT(d), "smram-region",
+                             f->pci_address_space, 0xa0000, 0x20000);
+    memory_region_add_subregion_overlap(f->system_memory, 0xa0000,
+                                        &f->smram_region, 1);
+    memory_region_set_enabled(&f->smram_region, false);
+    init_pam(dev, f->ram_memory, f->system_memory, f->pci_address_space,
+             &f->pam_regions[0], PAM_BIOS_BASE, PAM_BIOS_SIZE);
+    for (i = 0; i < 12; ++i) {
+        init_pam(dev, f->ram_memory, f->system_memory, f->pci_address_space,
+                 &f->pam_regions[i+1], PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE,
+                 PAM_EXPAN_SIZE);
+    }
+
+    /* Xen supports additional interrupt routes from the PCI devices to
+     * the IOAPIC: the four pins of each PCI device on the bus are also
+     * connected to the IOAPIC directly.
+     * These additional routes can be discovered through ACPI. */
+    piix3 = DO_UPCAST(PIIX3State, dev,
+            pci_create_simple_multifunction(b, -1, true, "PIIX3-xen"));
+    pci_bus_irqs(b, xen_piix3_set_irq, xen_pci_slot_get_pirq,
+            piix3, XEN_PIIX_NUM_PIRQS);
+    piix3->pic = pic;
+    *isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(piix3), "isa.0"));
+
+    *piix3_devfn = piix3->dev.devfn;
+
+    ram_size = ram_size / 8 / 1024 / 1024;
+    if (ram_size > 255) {
+        ram_size = 255;
+    }

hw/pci-host/piix.c PCIBus *i440fx_init(PCII440FXState **pi440fx_state, · line 487

Quoted 6 lines

hw/pci-host/piix.c +6 -0

+    d->config[0x57] = ram_size;
+
+    i440fx_update_memory_mappings(f);
+
+    return b;
+}

Too much copy-paste. Please refactor to avoid code duplication.

Is the only difference here the use of TYPE_I440FX_XEN_PCI_DEVICE?

Then you can pass type in as a parameter to a common static sub-function.

Quoted 28 lines

hw/pci-host/piix.c +1 -0

+
  PCIBus *find_i440fx(void)
  {
      PCIHostState *s = OBJECT_CHECK(PCIHostState,

include/hw/i386/pc.h +10 -1

index 1c0c382..51656d9 100644
@@ -239,6 +239,16 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
                      MemoryRegion *pci_memory,
                      MemoryRegion *ram_memory);
 
+PCIBus *xen_igd_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

Quoted 89 lines · 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   | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++
  include/hw/i386/pc.h | 10 +++++++
  2 files changed, 89 insertions(+)

hw/pci-host/piix.c +72 -0

index 9feddf5..7ef08d7 100644
@@ -407,6 +407,85 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
      return b;
  }
 
+PCIBus *xen_igd_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)
+{
+    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);
+    b = pci_bus_new(dev, NULL, pci_address_space,
+                    address_space_io, 0, TYPE_PCI_BUS);
+    s->bus = b;
+    object_property_add_child(qdev_get_machine(), "i440fx", OBJECT(dev), NULL);
+    qdev_init_nofail(dev);
+
+    d = pci_create_simple(b, 0, TYPE_I440FX_XEN_PCI_DEVICE);
+    *pi440fx_state = I440FX_XEN_PCI_DEVICE(d);
+    f = *pi440fx_state;
+    f->system_memory = address_space_mem;
+    f->pci_address_space = pci_address_space;
+    f->ram_memory = ram_memory;
+
+    i440fx = I440FX_PCI_HOST_BRIDGE(dev);
+    i440fx->pci_info.w32.begin = below_4g_mem_size;
+
+    /* setup pci memory mapping */
+    pc_pci_as_mapping_init(OBJECT(f), f->system_memory,
+                           f->pci_address_space);
+
+    memory_region_init_alias(&f->smram_region, OBJECT(d), "smram-region",
+                             f->pci_address_space, 0xa0000, 0x20000);
+    memory_region_add_subregion_overlap(f->system_memory, 0xa0000,
+                                        &f->smram_region, 1);
+    memory_region_set_enabled(&f->smram_region, false);
+    init_pam(dev, f->ram_memory, f->system_memory, f->pci_address_space,
+             &f->pam_regions[0], PAM_BIOS_BASE, PAM_BIOS_SIZE);
+    for (i = 0; i < 12; ++i) {
+        init_pam(dev, f->ram_memory, f->system_memory, f->pci_address_space,
+                 &f->pam_regions[i+1], PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE,
+                 PAM_EXPAN_SIZE);
+    }
+
+    /* Xen supports additional interrupt routes from the PCI devices to
+     * the IOAPIC: the four pins of each PCI device on the bus are also
+     * connected to the IOAPIC directly.
+     * These additional routes can be discovered through ACPI. */
+    piix3 = DO_UPCAST(PIIX3State, dev,
+            pci_create_simple_multifunction(b, -1, true, "PIIX3-xen"));
+    pci_bus_irqs(b, xen_piix3_set_irq, xen_pci_slot_get_pirq,
+            piix3, XEN_PIIX_NUM_PIRQS);
+    piix3->pic = pic;
+    *isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(piix3), "isa.0"));
+
+    *piix3_devfn = piix3->dev.devfn;
+
+    ram_size = ram_size / 8 / 1024 / 1024;
+    if (ram_size > 255) {
+        ram_size = 255;
+    }

hw/pci-host/piix.c PCIBus *i440fx_init(PCII440FXState **pi440fx_state, · line 487

Quoted 6 lines

hw/pci-host/piix.c +6 -0

+    d->config[0x57] = ram_size;
+
+    i440fx_update_memory_mappings(f);
+
+    return b;
+}

Too much copy-paste. Please refactor to avoid code duplication.

Is the only difference here the use of TYPE_I440FX_XEN_PCI_DEVICE?

Then you can pass type in as a parameter to a common static sub-function.

Quoted 28 lines

hw/pci-host/piix.c +1 -0

+
  PCIBus *find_i440fx(void)
  {
      PCIHostState *s = OBJECT_CHECK(PCIHostState,

include/hw/i386/pc.h +10 -1

index 1c0c382..51656d9 100644
@@ -239,6 +239,16 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
                      MemoryRegion *pci_memory,
                      MemoryRegion *ram_memory);
 
+PCIBus *xen_igd_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

Quoted 90 lines · Michael S. Tsirkin

Quoted 89 lines · 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   | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/i386/pc.h | 10 +++++++
 2 files changed, 89 insertions(+)

hw/pci-host/piix.c +72 -0

index 9feddf5..7ef08d7 100644
@@ -407,6 +407,85 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
     return b;
 }
 
+PCIBus *xen_igd_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)
+{
+    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);
+    b = pci_bus_new(dev, NULL, pci_address_space,
+                    address_space_io, 0, TYPE_PCI_BUS);
+    s->bus = b;
+    object_property_add_child(qdev_get_machine(), "i440fx", OBJECT(dev), NULL);
+    qdev_init_nofail(dev);
+
+    d = pci_create_simple(b, 0, TYPE_I440FX_XEN_PCI_DEVICE);
+    *pi440fx_state = I440FX_XEN_PCI_DEVICE(d);
+    f = *pi440fx_state;
+    f->system_memory = address_space_mem;
+    f->pci_address_space = pci_address_space;
+    f->ram_memory = ram_memory;
+
+    i440fx = I440FX_PCI_HOST_BRIDGE(dev);
+    i440fx->pci_info.w32.begin = below_4g_mem_size;
+
+    /* setup pci memory mapping */
+    pc_pci_as_mapping_init(OBJECT(f), f->system_memory,
+                           f->pci_address_space);
+
+    memory_region_init_alias(&f->smram_region, OBJECT(d), "smram-region",
+                             f->pci_address_space, 0xa0000, 0x20000);
+    memory_region_add_subregion_overlap(f->system_memory, 0xa0000,
+                                        &f->smram_region, 1);
+    memory_region_set_enabled(&f->smram_region, false);
+    init_pam(dev, f->ram_memory, f->system_memory, f->pci_address_space,
+             &f->pam_regions[0], PAM_BIOS_BASE, PAM_BIOS_SIZE);
+    for (i = 0; i < 12; ++i) {
+        init_pam(dev, f->ram_memory, f->system_memory, f->pci_address_space,
+                 &f->pam_regions[i+1], PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE,
+                 PAM_EXPAN_SIZE);
+    }
+
+    /* Xen supports additional interrupt routes from the PCI devices to
+     * the IOAPIC: the four pins of each PCI device on the bus are also
+     * connected to the IOAPIC directly.
+     * These additional routes can be discovered through ACPI. */
+    piix3 = DO_UPCAST(PIIX3State, dev,
+            pci_create_simple_multifunction(b, -1, true, "PIIX3-xen"));
+    pci_bus_irqs(b, xen_piix3_set_irq, xen_pci_slot_get_pirq,
+            piix3, XEN_PIIX_NUM_PIRQS);
+    piix3->pic = pic;
+    *isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(piix3), "isa.0"));
+
+    *piix3_devfn = piix3->dev.devfn;
+
+    ram_size = ram_size / 8 / 1024 / 1024;
+    if (ram_size > 255) {
+        ram_size = 255;
+    }

Quoted 8 lines

hw/pci-host/piix.c PCIBus *i440fx_init(PCII440FXState **pi440fx_state, · line 487

Quoted 6 lines

hw/pci-host/piix.c +6 -0

+    d->config[0x57] = ram_size;
+
+    i440fx_update_memory_mappings(f);
+
+    return b;
+}

Too much copy-paste. Please refactor to avoid code duplication.

Okay.

Quoted 1 line

Is the only difference here the use of TYPE_I440FX_XEN_PCI_DEVICE?

Yes.

Quoted 1 line

Then you can pass type in as a parameter to a common static sub-function.

I'm fine but as I remember Paolo don't like we intervene a common function. And we'll introduce something specific to IGD, like that faked PCIe device.

Thanks
Tiejun

Quoted 30 lines

Quoted 28 lines

hw/pci-host/piix.c +1 -0

+
 PCIBus *find_i440fx(void)
 {
     PCIHostState *s = OBJECT_CHECK(PCIHostState,

include/hw/i386/pc.h +10 -1

index 1c0c382..51656d9 100644
@@ -239,6 +239,16 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
                     MemoryRegion *pci_memory,
                     MemoryRegion *ram_memory);
 
+PCIBus *xen_igd_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

Quoted 90 lines · Michael S. Tsirkin

Quoted 89 lines · 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   | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/i386/pc.h | 10 +++++++
 2 files changed, 89 insertions(+)

hw/pci-host/piix.c +72 -0

index 9feddf5..7ef08d7 100644
@@ -407,6 +407,85 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
     return b;
 }
 
+PCIBus *xen_igd_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)
+{
+    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);
+    b = pci_bus_new(dev, NULL, pci_address_space,
+                    address_space_io, 0, TYPE_PCI_BUS);
+    s->bus = b;
+    object_property_add_child(qdev_get_machine(), "i440fx", OBJECT(dev), NULL);
+    qdev_init_nofail(dev);
+
+    d = pci_create_simple(b, 0, TYPE_I440FX_XEN_PCI_DEVICE);
+    *pi440fx_state = I440FX_XEN_PCI_DEVICE(d);
+    f = *pi440fx_state;
+    f->system_memory = address_space_mem;
+    f->pci_address_space = pci_address_space;
+    f->ram_memory = ram_memory;
+
+    i440fx = I440FX_PCI_HOST_BRIDGE(dev);
+    i440fx->pci_info.w32.begin = below_4g_mem_size;
+
+    /* setup pci memory mapping */
+    pc_pci_as_mapping_init(OBJECT(f), f->system_memory,
+                           f->pci_address_space);
+
+    memory_region_init_alias(&f->smram_region, OBJECT(d), "smram-region",
+                             f->pci_address_space, 0xa0000, 0x20000);
+    memory_region_add_subregion_overlap(f->system_memory, 0xa0000,
+                                        &f->smram_region, 1);
+    memory_region_set_enabled(&f->smram_region, false);
+    init_pam(dev, f->ram_memory, f->system_memory, f->pci_address_space,
+             &f->pam_regions[0], PAM_BIOS_BASE, PAM_BIOS_SIZE);
+    for (i = 0; i < 12; ++i) {
+        init_pam(dev, f->ram_memory, f->system_memory, f->pci_address_space,
+                 &f->pam_regions[i+1], PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE,
+                 PAM_EXPAN_SIZE);
+    }
+
+    /* Xen supports additional interrupt routes from the PCI devices to
+     * the IOAPIC: the four pins of each PCI device on the bus are also
+     * connected to the IOAPIC directly.
+     * These additional routes can be discovered through ACPI. */
+    piix3 = DO_UPCAST(PIIX3State, dev,
+            pci_create_simple_multifunction(b, -1, true, "PIIX3-xen"));
+    pci_bus_irqs(b, xen_piix3_set_irq, xen_pci_slot_get_pirq,
+            piix3, XEN_PIIX_NUM_PIRQS);
+    piix3->pic = pic;
+    *isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(piix3), "isa.0"));
+
+    *piix3_devfn = piix3->dev.devfn;
+
+    ram_size = ram_size / 8 / 1024 / 1024;
+    if (ram_size > 255) {
+        ram_size = 255;
+    }

Quoted 8 lines

hw/pci-host/piix.c PCIBus *i440fx_init(PCII440FXState **pi440fx_state, · line 487

Quoted 6 lines

hw/pci-host/piix.c +6 -0

+    d->config[0x57] = ram_size;
+
+    i440fx_update_memory_mappings(f);
+
+    return b;
+}

Too much copy-paste. Please refactor to avoid code duplication.

Okay.

Quoted 1 line

Is the only difference here the use of TYPE_I440FX_XEN_PCI_DEVICE?

Yes.

Quoted 1 line

Then you can pass type in as a parameter to a common static sub-function.

I'm fine but as I remember Paolo don't like we intervene a common function. And we'll introduce something specific to IGD, like that faked PCIe device.

Thanks
Tiejun

Quoted 30 lines

Quoted 28 lines

hw/pci-host/piix.c +1 -0

+
 PCIBus *find_i440fx(void)
 {
     PCIHostState *s = OBJECT_CHECK(PCIHostState,

include/hw/i386/pc.h +10 -1

index 1c0c382..51656d9 100644
@@ -239,6 +239,16 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
                     MemoryRegion *pci_memory,
                     MemoryRegion *ram_memory);
 
+PCIBus *xen_igd_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

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 | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 87 insertions(+)

hw/i386/pc_piix.c +12 -0

index 2391fda..46e5901 100644
@@ -206,6 +206,33 @@ static void pc_machine_pci_bus_init(MachineState *machine,
     }
 }
 
+static void xen_igd_pc_machine_pci_bus_init(MachineState *machine,
+                     int pci_enabled,
+                     PCII440FXState *i440fx_state,
+                     int piix3_devfn,
+                     PCIBus *pci_bus,
+                     ISABus *isa_bus,
+                     qemu_irq *gsi,
+                     MemoryRegion *pci_memory,
+                     MemoryRegion *ram_memory)
+{
+    MemoryRegion *system_memory = get_system_memory();
+    MemoryRegion *system_io = get_system_io();

hw/i386/pc_piix.c +62 -0

+}
+
 static void pc_machine_device_init(MachineState *machine,
                      int pci_enabled,
                      GSIState *gsi_state,

@@ -337,11 +364,39 @@ static void pc_init1(MachineState *machine,
                     piix3_devfn, fw_cfg, pci_bus, isa_bus, gsi);
 }
 
+static void xen_igd_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_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);
 }
 
+static void xen_igd_pc_init_pci(MachineState *machine)
+{
+    xen_igd_pc_init1(machine, 1, 1);
+}
+
 static void pc_compat_2_0(MachineState *machine)
 {
     smbios_legacy_mode = true;

@@ -470,6 +525,17 @@ static void pc_xen_hvm_init(MachineState *machine)
         pci_create_simple(bus, -1, "xen-platform");
     }
 }
+static void xen_igd_pc_hvm_init(MachineState *machine)
+{
+    PCIBus *bus;
+
+    xen_igd_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 \

@@ -919,6 +985,26 @@ 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_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)

@@ -942,6 +1028,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

Quoted 32 lines · 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 | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 87 insertions(+)

hw/i386/pc_piix.c +16 -0

index 2391fda..46e5901 100644
@@ -206,6 +206,33 @@ static void pc_machine_pci_bus_init(MachineState *machine,
      }
  }
 
+static void xen_igd_pc_machine_pci_bus_init(MachineState *machine,
+                     int pci_enabled,
+                     PCII440FXState *i440fx_state,
+                     int piix3_devfn,
+                     PCIBus *pci_bus,
+                     ISABus *isa_bus,
+                     qemu_irq *gsi,
+                     MemoryRegion *pci_memory,
+                     MemoryRegion *ram_memory)
+{
+    MemoryRegion *system_memory = get_system_memory();
+    MemoryRegion *system_io = get_system_io();
+
+    if (pci_enabled) {
+        pci_bus = xen_igd_i440fx_init(&i440fx_state, &piix3_devfn, &isa_bus,
+                              gsi, system_memory, system_io, machine->ram_size,

hw/i386/pc_piix.c static void pc_machine_pci_bus_init(MachineState *machine, · line 230

Quoted 6 lines

hw/i386/pc_piix.c +6 -0

+                              below_4g_mem_size,
+                              above_4g_mem_size,
+                              pci_memory, ram_memory);
+    } else {
+        pci_bus = NULL;
+        i440fx_state = NULL;

what does this do?

hw/i386/pc_piix.c static void pc_machine_pci_bus_init(MachineState *machine, · line 233

Quoted 4 lines

hw/i386/pc_piix.c +3 -0

+        isa_bus = isa_bus_new(NULL, system_io);
+        no_hpet = 1;
+    }

no_hpet is code duplicated from pc in chunk above, better to move to a common function.

Quoted 101 lines

hw/i386/pc_piix.c +62 -1

+}
+
  static void pc_machine_device_init(MachineState *machine,
                       int pci_enabled,
                       GSIState *gsi_state,

@@ -337,11 +364,39 @@ static void pc_init1(MachineState *machine,
                      piix3_devfn, fw_cfg, pci_bus, isa_bus, gsi);
  }
 
+static void xen_igd_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_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);
  }
 
+static void xen_igd_pc_init_pci(MachineState *machine)
+{
+    xen_igd_pc_init1(machine, 1, 1);
+}
+
  static void pc_compat_2_0(MachineState *machine)
  {
      smbios_legacy_mode = true;

@@ -470,6 +525,17 @@ static void pc_xen_hvm_init(MachineState *machine)
          pci_create_simple(bus, -1, "xen-platform");
      }
  }
+static void xen_igd_pc_hvm_init(MachineState *machine)
+{
+    PCIBus *bus;
+
+    xen_igd_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 \

@@ -919,6 +985,26 @@ 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_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)

@@ -942,6 +1028,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

Quoted 32 lines · 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 | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 87 insertions(+)

hw/i386/pc_piix.c +16 -0

index 2391fda..46e5901 100644
@@ -206,6 +206,33 @@ static void pc_machine_pci_bus_init(MachineState *machine,
      }
  }
 
+static void xen_igd_pc_machine_pci_bus_init(MachineState *machine,
+                     int pci_enabled,
+                     PCII440FXState *i440fx_state,
+                     int piix3_devfn,
+                     PCIBus *pci_bus,
+                     ISABus *isa_bus,
+                     qemu_irq *gsi,
+                     MemoryRegion *pci_memory,
+                     MemoryRegion *ram_memory)
+{
+    MemoryRegion *system_memory = get_system_memory();
+    MemoryRegion *system_io = get_system_io();
+
+    if (pci_enabled) {
+        pci_bus = xen_igd_i440fx_init(&i440fx_state, &piix3_devfn, &isa_bus,
+                              gsi, system_memory, system_io, machine->ram_size,

hw/i386/pc_piix.c static void pc_machine_pci_bus_init(MachineState *machine, · line 230

Quoted 6 lines

hw/i386/pc_piix.c +6 -0

+                              below_4g_mem_size,
+                              above_4g_mem_size,
+                              pci_memory, ram_memory);
+    } else {
+        pci_bus = NULL;
+        i440fx_state = NULL;

what does this do?