Re: [Qemu-devel] [PATCH] Refactoring ISA only PC code for pc_isa.c

[Paolo Bonzini]

Il 10/06/2013 05:57, Simarpreet Singh ha scritto:
> Signed-off-by: Simarpreet Singh <address@hidden>
> ---
>  hw/i386/Makefile.objs |    2 +-
>  hw/i386/pc_isa.c      |  213 
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/i386/pc_piix.c     |   41 ----------
>  3 files changed, 214 insertions(+), 42 deletions(-)
>  create mode 100644 hw/i386/pc_isa.c
> 
> diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
> index 205d22e..fe67151 100644
> --- a/hw/i386/Makefile.objs
> +++ b/hw/i386/Makefile.objs
> @@ -1,6 +1,6 @@
>  obj-$(CONFIG_KVM) += kvm/
>  obj-y += multiboot.o smbios.o
> -obj-y += pc.o pc_piix.o pc_q35.o
> +obj-y += pc.o pc_piix.o pc_q35.o pc_isa.o
>  obj-$(CONFIG_XEN) += xen_domainbuild.o xen_machine_pv.o
> 
>  obj-y += kvmvapic.o
> diff --git a/hw/i386/pc_isa.c b/hw/i386/pc_isa.c
> new file mode 100644
> index 0000000..f0c7de2
> --- /dev/null
> +++ b/hw/i386/pc_isa.c
> @@ -0,0 +1,213 @@
> +/*
> + * QEMU PC System Emulator
> + *
> + * Copyright (c) 2003-2004 Fabrice Bellard
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this software and associated documentation files (the "Software"), to 
> deal
> + * in the Software without restriction, including without limitation the 
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include <glib.h>
> +
> +#include "hw/hw.h"
> +#include "hw/i386/pc.h"
> +#include "hw/i386/apic.h"
> +#include "hw/usb.h"
> +#include "net/net.h"
> +#include "hw/boards.h"
> +#include "hw/ide.h"
> +#include "sysemu/kvm.h"
> +#include "hw/kvm/clock.h"
> +#include "sysemu/sysemu.h"
> +#include "hw/sysbus.h"
> +#include "hw/cpu/icc_bus.h"
> +#include "sysemu/arch_init.h"
> +#include "sysemu/blockdev.h"
> +#include "hw/i2c/smbus.h"
> +#include "hw/xen/xen.h"
> +#include "exec/memory.h"
> +#include "exec/address-spaces.h"
> +#include "hw/acpi/acpi.h"
> +#include "cpu.h"
> +
> +#define MAX_IDE_BUS 2
> +
> +static const int ide_iobase[MAX_IDE_BUS] = { 0x1f0, 0x170 };
> +static const int ide_iobase2[MAX_IDE_BUS] = { 0x3f6, 0x376 };
> +static const int ide_irq[MAX_IDE_BUS] = { 14, 15 };
> +
> +static bool has_pvpanic = true;
> +
> +/* PC hardware initialisation */
> +static void pc_init1(MemoryRegion *system_memory,
> +                     MemoryRegion *system_io,
> +                     ram_addr_t ram_size,
> +                     const char *boot_device,
> +                     const char *kernel_filename,
> +                     const char *kernel_cmdline,
> +                     const char *initrd_filename,
> +                     const char *cpu_model,
> +                     int kvmclock_enabled)
> +{
> +    int i;
> +    ram_addr_t below_4g_mem_size, above_4g_mem_size;
> +    ISABus *isa_bus;
> +    qemu_irq *cpu_irq;
> +    qemu_irq *gsi;
> +    qemu_irq *i8259;
> +    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 *rom_memory;
> +    DeviceState *icc_bridge;
> +
> +    icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE);
> +    object_property_add_child(qdev_get_machine(), "icc-bridge",
> +                              OBJECT(icc_bridge), NULL);
> +
> +    pc_cpus_init(cpu_model, icc_bridge);
> +    pc_acpi_init("acpi-dsdt.aml");
> +
> +    if (kvmclock_enabled) {
> +        kvmclock_create();
> +    }
> +
> +    if (ram_size >= 0xe0000000) {
> +        above_4g_mem_size = ram_size - 0xe0000000;
> +        below_4g_mem_size = 0xe0000000;
> +    } else {
> +        above_4g_mem_size = 0;
> +        below_4g_mem_size = ram_size;
> +    }
> +
> +    rom_memory = system_memory;
> +
> +    /* allocate ram and load rom/bios */
> +    if (!xen_enabled()) {
> +        pc_memory_init(system_memory, kernel_filename,
> +                       kernel_cmdline, initrd_filename,
> +                       below_4g_mem_size, above_4g_mem_size,
> +                       rom_memory, &ram_memory);
> +    }
> +
> +
> +    gsi_state = g_malloc0(sizeof(*gsi_state));
> +    if (kvm_irqchip_in_kernel()) {
> +        kvm_pc_setup_irq_routing(false);
> +        gsi = qemu_allocate_irqs(kvm_pc_gsi_handler, gsi_state,
> +                                 GSI_NUM_PINS);
> +    } else {
> +        gsi = qemu_allocate_irqs(gsi_handler, gsi_state, GSI_NUM_PINS);
> +    }
> +
> +    isa_bus = isa_bus_new(NULL, system_io);
> +    no_hpet = 1;
> +    isa_bus_irqs(isa_bus, gsi);
> +
> +    if (kvm_irqchip_in_kernel()) {
> +        i8259 = kvm_i8259_init(isa_bus);
> +    } else if (xen_enabled()) {
> +        i8259 = xen_interrupt_controller_init();
> +    } else {
> +        cpu_irq = pc_allocate_cpu_irq();
> +        i8259 = i8259_init(isa_bus, cpu_irq[0]);
> +    }
> +
> +    for (i = 0; i < ISA_NUM_IRQS; i++) {
> +        gsi_state->i8259_irq[i] = i8259[i];
> +    }
> +
> +    qdev_init_nofail(icc_bridge);
> +
> +    pc_register_ferr_irq(gsi[13]);
> +
> +    pc_vga_init(isa_bus, NULL);
> +
> +
> +    /* init basic PC hardware */
> +    pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy, xen_enabled());
> +
> +    pc_nic_init(isa_bus, NULL);
> +
> +    ide_drive_get(hd, MAX_IDE_BUS);
> +
> +    for(i = 0; i < MAX_IDE_BUS; i++) {
> +        ISADevice *dev;
> +        dev = isa_ide_init(isa_bus, ide_iobase[i], ide_iobase2[i],
> +                           ide_irq[i], hd[MAX_IDE_DEVS * i],
> +                           hd[MAX_IDE_DEVS * i + 1]);
> +        idebus[i] = qdev_get_child_bus(&dev->qdev, "ide.0");
> +    }
> +
> +    pc_cmos_init(below_4g_mem_size, above_4g_mem_size, boot_device,
> +                 floppy, idebus[0], idebus[1], rtc_state);
> +
> +    if (has_pvpanic) {
> +        pvpanic_init(isa_bus);
> +    }
> +}
> +
> +
> +static void pc_init_isa(QEMUMachineInitArgs *args)
> +{
> +    ram_addr_t ram_size = args->ram_size;
> +    const char *cpu_model = args->cpu_model;
> +    const char *kernel_filename = args->kernel_filename;
> +    const char *kernel_cmdline = args->kernel_cmdline;
> +    const char *initrd_filename = args->initrd_filename;
> +    const char *boot_device = args->boot_device;
> +    has_pvpanic = false;
> +    if (cpu_model == NULL)
> +        cpu_model = "486";
> +    disable_kvm_pv_eoi();
> +    enable_compat_apic_id_mode();
> +    pc_init1(get_system_memory(),
> +             get_system_io(),
> +             ram_size, boot_device,
> +             kernel_filename, kernel_cmdline,
> +             initrd_filename, cpu_model, 1);
> +}
> +
> +
> +
> +static QEMUMachine isapc_machine = {
> +    .name = "isapc",
> +    .desc = "ISA-only PC",
> +    .init = pc_init_isa,
> +    .max_cpus = 1,
> +    .compat_props = (GlobalProperty[]) {
> +        {
> +            .driver   = "pc-sysfw",
> +            .property = "rom_only",
> +            .value    = stringify(1),
> +        },
> +        { /* end of list */ }
> +    },
> +    DEFAULT_MACHINE_OPTIONS,
> +};
> +
> +
> +static void pc_machine_init(void)
> +{
> +    qemu_register_machine(&isapc_machine);
> +}
> +
> +machine_init(pc_machine_init);
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index d618570..a32034c 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -297,25 +297,6 @@ static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs 
> *args)
>               initrd_filename, cpu_model, 1, 0);
>  }
> 
> -static void pc_init_isa(QEMUMachineInitArgs *args)
> -{
> -    ram_addr_t ram_size = args->ram_size;
> -    const char *cpu_model = args->cpu_model;
> -    const char *kernel_filename = args->kernel_filename;
> -    const char *kernel_cmdline = args->kernel_cmdline;
> -    const char *initrd_filename = args->initrd_filename;
> -    const char *boot_device = args->boot_device;
> -    has_pvpanic = false;
> -    if (cpu_model == NULL)
> -        cpu_model = "486";
> -    disable_kvm_pv_eoi();
> -    enable_compat_apic_id_mode();
> -    pc_init1(get_system_memory(),
> -             get_system_io(),
> -             ram_size, boot_device,
> -             kernel_filename, kernel_cmdline,
> -             initrd_filename, cpu_model, 0, 1);
> -}
> 
>  #ifdef CONFIG_XEN
>  static void pc_xen_hvm_init(QEMUMachineInitArgs *args)
> @@ -701,27 +682,6 @@ static QEMUMachine pc_machine_v0_10 = {
>      DEFAULT_MACHINE_OPTIONS,
>  };
> 
> -static QEMUMachine isapc_machine = {
> -    .name = "isapc",
> -    .desc = "ISA-only PC",
> -    .init = pc_init_isa,
> -    .max_cpus = 1,
> -    .compat_props = (GlobalProperty[]) {
> -        {
> -            .driver   = "pc-sysfw",
> -            .property = "rom_only",
> -            .value    = stringify(1),
> -        },
> -        {
> -            .driver   = "pc-sysfw",
> -            .property = "isapc_ram_fw",
> -            .value    = stringify(1),
> -        },
> -        { /* end of list */ }
> -    },
> -    DEFAULT_MACHINE_OPTIONS,
> -};
> -
>  #ifdef CONFIG_XEN
>  static QEMUMachine xenfv_machine = {
>      .name = "xenfv",
> @@ -747,7 +707,6 @@ static void pc_machine_init(void)
>      qemu_register_machine(&pc_machine_v0_12);
>      qemu_register_machine(&pc_machine_v0_11);
>      qemu_register_machine(&pc_machine_v0_10);
> -    qemu_register_machine(&isapc_machine);
>  #ifdef CONFIG_XEN
>      qemu_register_machine(&xenfv_machine);
>  #endif
> --
> 1.7.9.5
> 
> 
> 

This patch is good, but you need a followup that cleans up pc_piix.c
removing other isapc remnants (especially the penultimate argument of
pc_init1).

Paolo