From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43197) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XCP7O-0006OG-Sr for qemu-devel@nongnu.org; Wed, 30 Jul 2014 04:20:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XCP7I-0005Rs-5A for qemu-devel@nongnu.org; Wed, 30 Jul 2014 04:20:14 -0400 Received: from mga02.intel.com ([134.134.136.20]:48385) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XCP7H-0005Oe-PO for qemu-devel@nongnu.org; Wed, 30 Jul 2014 04:20:08 -0400 Message-ID: <53D8AAAE.1000705@intel.com> Date: Wed, 30 Jul 2014 16:19:58 +0800 From: "Chen, Tiejun" MIME-Version: 1.0 References: <1406201429-21700-1-git-send-email-tiejun.chen@intel.com> <1406201429-21700-2-git-send-email-tiejun.chen@intel.com> <20140729112600.GC13763@redhat.com> In-Reply-To: <20140729112600.GC13763@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/4] hw:i386:pc_piix: split pc_init1() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: pbonzini@redhat.com, xen-devel@lists.xen.org, qemu-devel@nongnu.org, stefano.stabellini@eu.citrix.com 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 > > 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 >