Re: [PATCH 3/5] x86: move more functions to X86MachineState and x86.c

[Sergio Lopez]

Paolo Bonzini <address@hidden> writes:

> These are needed by microvm too, so move them outside of PC-specific files.
>
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  hw/i386/pc.c          |  99 -------------------------------------------------
>  hw/i386/x86.c         | 101 
> ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/i386/pc.h  |  12 ------
>  include/hw/i386/x86.h |  13 +++++++
>  4 files changed, 114 insertions(+), 111 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 3d2c5d8..308248d 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -342,17 +342,6 @@ GlobalProperty pc_compat_1_4[] = {
>  };
>  const size_t pc_compat_1_4_len = G_N_ELEMENTS(pc_compat_1_4);
>  
> -void gsi_handler(void *opaque, int n, int level)
> -{
> -    GSIState *s = opaque;
> -
> -    DPRINTF("pc: %s GSI %d\n", level ? "raising" : "lowering", n);
> -    if (n < ISA_NUM_IRQS) {
> -        qemu_set_irq(s->i8259_irq[n], level);
> -    }
> -    qemu_set_irq(s->ioapic_irq[n], level);
> -}
> -
>  GSIState *pc_gsi_create(qemu_irq **irqs, bool pci_enabled)
>  {
>      GSIState *s;
> @@ -392,55 +381,6 @@ static uint64_t ioportF0_read(void *opaque, hwaddr addr, 
> unsigned size)
>      return 0xffffffffffffffffULL;
>  }
>  
> -/* TSC handling */
> -uint64_t cpu_get_tsc(CPUX86State *env)
> -{
> -    return cpu_get_ticks();
> -}
> -
> -/* IRQ handling */
> -int cpu_get_pic_interrupt(CPUX86State *env)
> -{
> -    X86CPU *cpu = env_archcpu(env);
> -    int intno;
> -
> -    if (!kvm_irqchip_in_kernel()) {
> -        intno = apic_get_interrupt(cpu->apic_state);
> -        if (intno >= 0) {
> -            return intno;
> -        }
> -        /* read the irq from the PIC */
> -        if (!apic_accept_pic_intr(cpu->apic_state)) {
> -            return -1;
> -        }
> -    }
> -
> -    intno = pic_read_irq(isa_pic);
> -    return intno;
> -}
> -
> -static void pic_irq_request(void *opaque, int irq, int level)
> -{
> -    CPUState *cs = first_cpu;
> -    X86CPU *cpu = X86_CPU(cs);
> -
> -    DPRINTF("pic_irqs: %s irq %d\n", level? "raise" : "lower", irq);
> -    if (cpu->apic_state && !kvm_irqchip_in_kernel()) {
> -        CPU_FOREACH(cs) {
> -            cpu = X86_CPU(cs);
> -            if (apic_accept_pic_intr(cpu->apic_state)) {
> -                apic_deliver_pic_intr(cpu->apic_state, level);
> -            }
> -        }
> -    } else {
> -        if (level) {
> -            cpu_interrupt(cs, CPU_INTERRUPT_HARD);
> -        } else {
> -            cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
> -        }
> -    }
> -}
> -
>  /* PC cmos mappings */
>  
>  #define REG_EQUIPMENT_BYTE          0x14
> @@ -884,16 +824,6 @@ void pc_init_ne2k_isa(ISABus *bus, NICInfo *nd)
>      nb_ne2k++;
>  }
>  
> -DeviceState *cpu_get_current_apic(void)
> -{
> -    if (current_cpu) {
> -        X86CPU *cpu = X86_CPU(current_cpu);
> -        return cpu->apic_state;
> -    } else {
> -        return NULL;
> -    }
> -}
> -
>  void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
>  {
>      X86CPU *cpu = opaque;
> @@ -1289,11 +1219,6 @@ uint64_t pc_pci_hole64_start(void)
>      return ROUND_UP(hole64_start, 1 * GiB);
>  }
>  
> -qemu_irq pc_allocate_cpu_irq(void)
> -{
> -    return qemu_allocate_irq(pic_irq_request, NULL, 0);
> -}
> -
>  DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus)
>  {
>      DeviceState *dev = NULL;
> @@ -1480,30 +1405,6 @@ void pc_i8259_create(ISABus *isa_bus, qemu_irq 
> *i8259_irqs)
>      g_free(i8259);
>  }
>  
> -void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name)
> -{
> -    DeviceState *dev;
> -    SysBusDevice *d;
> -    unsigned int i;
> -
> -    if (kvm_ioapic_in_kernel()) {
> -        dev = qdev_create(NULL, TYPE_KVM_IOAPIC);
> -    } else {
> -        dev = qdev_create(NULL, TYPE_IOAPIC);
> -    }
> -    if (parent_name) {
> -        object_property_add_child(object_resolve_path(parent_name, NULL),
> -                                  "ioapic", OBJECT(dev), NULL);
> -    }
> -    qdev_init_nofail(dev);
> -    d = SYS_BUS_DEVICE(dev);
> -    sysbus_mmio_map(d, 0, IO_APIC_DEFAULT_ADDRESS);
> -
> -    for (i = 0; i < IOAPIC_NUM_PINS; i++) {
> -        gsi_state->ioapic_irq[i] = qdev_get_gpio_in(dev, i);
> -    }
> -}
> -
>  static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                                 Error **errp)
>  {
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index 394edc2..028bbf4 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -41,11 +41,15 @@
>  #include "hw/i386/fw_cfg.h"
>  
>  #include "hw/acpi/cpu_hotplug.h"
> +#include "hw/i386/pc.h"

Including pc.h from x86.h looks a bit weird to me. I think it'd be nicer
moving the i8259 declarations to a separate header, and the all the
GSI-related stuff to x86.h. I have a couple patches doing that, in case
they can save you some time.

> +#include "hw/irq.h"
>  #include "hw/nmi.h"
>  #include "hw/loader.h"
>  #include "multiboot.h"
>  #include "elf.h"
>  #include "standard-headers/asm-x86/bootparam.h"
> +#include "config-devices.h"
> +#include "kvm_i386.h"
>  
>  #define BIOS_FILENAME "bios.bin"
>  
> @@ -220,6 +224,103 @@ static long get_file_size(FILE *f)
>      return size;
>  }
>  
> +/* TSC handling */
> +uint64_t cpu_get_tsc(CPUX86State *env)
> +{
> +    return cpu_get_ticks();
> +}
> +
> +/* IRQ handling */
> +static void pic_irq_request(void *opaque, int irq, int level)
> +{
> +    CPUState *cs = first_cpu;
> +    X86CPU *cpu = X86_CPU(cs);
> +
> +    if (cpu->apic_state && !kvm_irqchip_in_kernel()) {
> +        CPU_FOREACH(cs) {
> +            cpu = X86_CPU(cs);
> +            if (apic_accept_pic_intr(cpu->apic_state)) {
> +                apic_deliver_pic_intr(cpu->apic_state, level);
> +            }
> +        }
> +    } else {
> +        if (level) {
> +            cpu_interrupt(cs, CPU_INTERRUPT_HARD);
> +        } else {
> +            cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
> +        }
> +    }
> +}
> +
> +qemu_irq pc_allocate_cpu_irq(void)
> +{
> +    return qemu_allocate_irq(pic_irq_request, NULL, 0);
> +}

Should we rename this one to "x86_allocate_cpu_irq(void)" ?

> +
> +int cpu_get_pic_interrupt(CPUX86State *env)
> +{
> +    X86CPU *cpu = env_archcpu(env);
> +    int intno;
> +
> +    if (!kvm_irqchip_in_kernel()) {
> +        intno = apic_get_interrupt(cpu->apic_state);
> +        if (intno >= 0) {
> +            return intno;
> +        }
> +        /* read the irq from the PIC */
> +        if (!apic_accept_pic_intr(cpu->apic_state)) {
> +            return -1;
> +        }
> +    }
> +
> +    intno = pic_read_irq(isa_pic);
> +    return intno;
> +}
> +
> +DeviceState *cpu_get_current_apic(void)
> +{
> +    if (current_cpu) {
> +        X86CPU *cpu = X86_CPU(current_cpu);
> +        return cpu->apic_state;
> +    } else {
> +        return NULL;
> +    }
> +}
> +
> +void gsi_handler(void *opaque, int n, int level)
> +{
> +    GSIState *s = opaque;
> +
> +    if (n < ISA_NUM_IRQS) {
> +        qemu_set_irq(s->i8259_irq[n], level);
> +    }
> +    qemu_set_irq(s->ioapic_irq[n], level);
> +}
> +
> +void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name)
> +{
> +    DeviceState *dev;
> +    SysBusDevice *d;
> +    unsigned int i;
> +
> +    if (kvm_ioapic_in_kernel()) {
> +        dev = qdev_create(NULL, TYPE_KVM_IOAPIC);
> +    } else {
> +        dev = qdev_create(NULL, TYPE_IOAPIC);
> +    }
> +    if (parent_name) {
> +        object_property_add_child(object_resolve_path(parent_name, NULL),
> +                                  "ioapic", OBJECT(dev), NULL);
> +    }
> +    qdev_init_nofail(dev);
> +    d = SYS_BUS_DEVICE(dev);
> +    sysbus_mmio_map(d, 0, IO_APIC_DEFAULT_ADDRESS);
> +
> +    for (i = 0; i < IOAPIC_NUM_PINS; i++) {
> +        gsi_state->ioapic_irq[i] = qdev_get_gpio_in(dev, i);
> +    }
> +}
> +
>  struct setup_data {
>      uint64_t next;
>      uint32_t type;
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 1f86eba..0ab4a2a 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -3,11 +3,9 @@
>  
>  #include "exec/memory.h"
>  #include "hw/boards.h"
> -#include "hw/isa/isa.h"
>  #include "hw/block/fdc.h"
>  #include "hw/block/flash.h"
>  #include "net/net.h"
> -#include "hw/i386/ioapic.h"
>  #include "hw/i386/x86.h"
>  
>  #include "qemu/range.h"
> @@ -144,15 +142,6 @@ int pic_get_output(DeviceState *d);
>  
>  /* ioapic.c */
>  
> -/* Global System Interrupts */
> -
> -#define GSI_NUM_PINS IOAPIC_NUM_PINS
> -
> -typedef struct GSIState {
> -    qemu_irq i8259_irq[ISA_NUM_IRQS];
> -    qemu_irq ioapic_irq[IOAPIC_NUM_PINS];
> -} GSIState;
> -
>  void gsi_handler(void *opaque, int n, int level);
>  
>  GSIState *pc_gsi_create(qemu_irq **irqs, bool pci_enabled);
> @@ -217,7 +206,6 @@ void pc_pci_device_init(PCIBus *pci_bus);
>  typedef void (*cpu_set_smm_t)(int smm, void *arg);
>  
>  void pc_i8259_create(ISABus *isa_bus, qemu_irq *i8259_irqs);
> -void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name);
>  
>  ISADevice *pc_find_fdc0(void);
>  int cmos_get_fd_drive_type(FloppyDriveType fd0);
> diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
> index 4b84917..a6c23ef 100644
> --- a/include/hw/i386/x86.h
> +++ b/include/hw/i386/x86.h
> @@ -23,6 +23,8 @@
>  
>  #include "hw/boards.h"
>  #include "hw/nmi.h"
> +#include "hw/isa/isa.h"
> +#include "hw/i386/ioapic.h"
>  
>  typedef struct {
>      /*< private >*/
> @@ -95,4 +97,15 @@ void x86_load_linux(X86MachineState *x86ms,
>                      bool pvh_enabled,
>                      bool linuxboot_dma_enabled);
>  
> +/* Global System Interrupts */
> +
> +#define GSI_NUM_PINS IOAPIC_NUM_PINS
> +
> +typedef struct GSIState {
> +    qemu_irq i8259_irq[ISA_NUM_IRQS];
> +    qemu_irq ioapic_irq[IOAPIC_NUM_PINS];
> +} GSIState;
> +
> +void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name);
> +
>  #endif

signature.asc
Description: PGP signature