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

["Chen, Tiejun" <tiejun.chen@intel.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 <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
>