| Message ID | 1406201429-21700-2-git-send-email-tiejun.chen@intel.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Jul 24, 2014 at 07:30:26PM +0800, Tiejun Chen wrote: > We'd like to split pc_init1 and then we can share something > with other stuff. > > Signed-off-by: Tiejun Chen <tiejun.chen@intel.com> Did you test this patch? It does not look like it can work. > --- > hw/i386/pc_piix.c | 93 +++++++++++++++++++++++++++++++++++++++++-------------- > 1 file changed, 70 insertions(+), 23 deletions(-) > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > index 7081c08..2391fda 100644 > --- a/hw/i386/pc_piix.c > +++ b/hw/i386/pc_piix.c > @@ -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) > +{ > + 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? > + > + 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
On 2014/7/29 19:26, Michael S. Tsirkin wrote: > On Thu, Jul 24, 2014 at 07:30:26PM +0800, Tiejun Chen wrote: >> We'd like to split pc_init1 and then we can share something >> with other stuff. >> >> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com> > > 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 > >> --- >> hw/i386/pc_piix.c | 93 +++++++++++++++++++++++++++++++++++++++++-------------- >> 1 file changed, 70 insertions(+), 23 deletions(-) >> >> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c >> index 7081c08..2391fda 100644 >> --- a/hw/i386/pc_piix.c >> +++ b/hw/i386/pc_piix.c >> @@ -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) >> +{ >> + 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? > > >> + >> + 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 >
On Wed, Jul 30, 2014 at 04:19:58PM +0800, Chen, Tiejun wrote: > On 2014/7/29 19:26, Michael S. Tsirkin wrote: > >On Thu, Jul 24, 2014 at 07:30:26PM +0800, Tiejun Chen wrote: > >>We'd like to split pc_init1 and then we can share something > >>with other stuff. > >> > >>Signed-off-by: Tiejun Chen <tiejun.chen@intel.com> > > > >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 Yea, and pls tell us what you tested exactly in the cover letter. > > > >>--- > >> hw/i386/pc_piix.c | 93 +++++++++++++++++++++++++++++++++++++++++-------------- > >> 1 file changed, 70 insertions(+), 23 deletions(-) > >> > >>diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > >>index 7081c08..2391fda 100644 > >>--- a/hw/i386/pc_piix.c > >>+++ b/hw/i386/pc_piix.c > >>@@ -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) > >>+{ > >>+ 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? > > > > > >>+ > >>+ 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 > >
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index 7081c08..2391fda 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -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) +{ + 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);
We'd like to split pc_init1 and then we can share something with other stuff. Signed-off-by: Tiejun Chen <tiejun.chen@intel.com> --- hw/i386/pc_piix.c | 93 +++++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 70 insertions(+), 23 deletions(-)