Re: [Qemu-devel] [PATCH v1 07/13] q35: Introduce q35 pc based chipset emulator

[Anthony Liguori]

Jason Baron <address@hidden> writes:

> From: Isaku Yamahata <address@hidden>
>
> pc q35 based chipset emulator to support pci express natively. Based on
> Anthony Liguori's suggestion, the machine name is 'q35-next', with an alias
> of 'q35'. At this point, there are no compatibility guarantees. When the
> chipset stabilizes more, we will begin to version the machine names.
>
> Major features which still need to be added:
>
> -Migration support (mostly around ahci)
> -ACPI hotplug support (pcie hotplug support is working)
> -Passthrough support
>
> Signed-off-by: Isaku Yamahata <address@hidden>
> Signed-off-by: Jason Baron <address@hidden>
> ---
>  hw/i386/Makefile.objs |    2 +-
>  hw/pc.h               |    2 +
>  hw/pc_piix.c          |    4 +-
>  hw/pc_q35.c           |  326 
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/pci_ids.h          |    2 +
>  hw/q35.c              |  315 +++++++++++++++++++++++++++++++++++++++++++++++
>  hw/q35.h              |  161 ++++++++++++++++++++++++
>  7 files changed, 809 insertions(+), 3 deletions(-)
>  create mode 100644 hw/pc_q35.c
>  create mode 100644 hw/q35.c
>  create mode 100644 hw/q35.h
>
> diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
> index 693bd18..469b127 100644
> --- a/hw/i386/Makefile.objs
> +++ b/hw/i386/Makefile.objs
> @@ -7,7 +7,7 @@ obj-y += debugcon.o multiboot.o
>  obj-y += pc_piix.o
>  obj-y += pc_sysfw.o
>  obj-y += pam.o
> -obj-y += acpi_ich9.o lpc_ich9.o smbus_ich9.o
> +obj-y += acpi_ich9.o lpc_ich9.o smbus_ich9.o q35.o pc_q35.o

This shouldn't be here.  It should be in hw/Makefile.objs.

This is not target specific code.

>  obj-$(CONFIG_XEN) += xen_platform.o xen_apic.o
>  obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen-host-pci-device.o
>  obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen_pt.o xen_pt_config_init.o 
> xen_pt_msi.o
> diff --git a/hw/pc.h b/hw/pc.h
> index 2237e86..e1bf2fc 100644
> --- a/hw/pc.h
> +++ b/hw/pc.h
> @@ -42,6 +42,8 @@ int pic_read_irq(DeviceState *d);
>  int pic_get_output(DeviceState *d);
>  void pic_info(Monitor *mon);
>  void irq_info(Monitor *mon);
> +void kvm_piix3_gsi_handler(void *opaque, int n, int level);
> +void kvm_piix3_setup_irq_routing(bool pci_enabled);

kvm specific code declarations shouldn't be in pc.h.

BTW, if you've got q35 calling something called "kvm_piix3..." something
is wrong.  There's no piix3 in the q35.

>  /* Global System Interrupts */
>  
> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index 7bcac87..26565a1 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -54,7 +54,7 @@ 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 void kvm_piix3_setup_irq_routing(bool pci_enabled)
> +void kvm_piix3_setup_irq_routing(bool pci_enabled)
>  {
>  #ifdef CONFIG_KVM
>      KVMState *s = kvm_state;
> @@ -83,7 +83,7 @@ static void kvm_piix3_setup_irq_routing(bool pci_enabled)
>  #endif /* CONFIG_KVM */
>  }
>  
> -static void kvm_piix3_gsi_handler(void *opaque, int n, int level)
> +void kvm_piix3_gsi_handler(void *opaque, int n, int level)
>  {
>      GSIState *s = opaque;
>  
> diff --git a/hw/pc_q35.c b/hw/pc_q35.c
> new file mode 100644
> index 0000000..cf0d361
> --- /dev/null
> +++ b/hw/pc_q35.c
> @@ -0,0 +1,326 @@
> +/*
> + * Q35 chipset based pc system emulator
> + *
> + * Copyright (c) 2003-2004 Fabrice Bellard
> + * Copyright (c) 2009, 2010
> + *               Isaku Yamahata <yamahata at valinux co jp>
> + *               VA Linux Systems Japan K.K.
> + * Copyright (C) 2012 Jason Baron <address@hidden>
> + *
> + * This is based on pc.c, but heavily modified.
> + *
> + * 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 "hw.h"
> +#include "arch_init.h"
> +#include "pc.h"
> +#include "fdc.h"
> +#include "pci.h"
> +#include "pci_bridge.h"
> +#include "ioh3420.h"
> +#include "xio3130_upstream.h"
> +#include "xio3130_downstream.h"

There's way too many includes here.  Don't just copy/paste includes.

> +#include "block.h"
> +#include "blockdev.h"
> +#include "sysemu.h"
> +#include "audio/audio.h"
> +#include "net.h"
> +#include "smbus.h"
> +#include "boards.h"
> +#include "monitor.h"
> +#include "fw_cfg.h"
> +#include "hpet_emul.h"
> +#include "watchdog.h"
> +#include "smbios.h"
> +#include "ide.h"
> +#include "mc146818rtc.h"
> +#include "xen.h"
> +#include "kvm.h"
> +
> +#include "q35.h"
> +#include "exec-memory.h"
> +#include "ich9.h"
> +#include <hw/ide/pci.h>
> +#include <hw/ide/ahci.h>

Should be double quotes..

> +
> +/* ICH9 AHCI has 6 ports */
> +#define MAX_SATA_PORTS     6
> +
> +static void pc_q35_init_early(qemu_irq *gsi, GSIState *gsi_state,
> +                              DeviceState **gmch_host_p,
> +                              PCIBus **host_bus_p,
> +                              PCIDevice **lpc_p, ISABus **isa_bus,
> +                              MemoryRegion *system_memory,
> +                              MemoryRegion *pci_address_space,
> +                              MemoryRegion *address_space_io,
> +                              MemoryRegion *ram_memory,
> +                              ram_addr_t below_4g_mem_size,
> +                              ram_addr_t above_4g_mem_size)
> +{
> +    int i;
> +    hwaddr pci_hole64_size;
> +    DeviceState *gmch_host;
> +    PCIBus *host_bus;
> +
> +    PCIDevice *gmch_state;
> +    PCIDevice *lpc;
> +    GMCHPCIState *gmps;
> +    ICH9LPCState *ich9_lpc;
> +
> +    /* create pci host bus */
> +    host_bus = gmch_host_init(&gmch_host, pci_address_space, 
> address_space_io);
> +    gmch_state = gmch_init(gmch_host, host_bus);
> +    gmps = GMCH_PCI_DEVICE(gmch_state);
> +
> +    /* create ISA bus */
> +    lpc = pci_create_simple_multifunction(host_bus, PCI_DEVFN(ICH9_LPC_DEV,
> +                                          ICH9_LPC_FUNC), true,
> +                                          TYPE_ICH9_LPC_DEVICE);

Should be done through composition.

Q35 ought to be modelled as a device from the start properly.

> +    ich9_lpc = ICH9_LPC_DEVICE(lpc);
> +    ich9_lpc->pic = gsi;
> +    ich9_lpc->ioapic = gsi_state->ioapic_irq;
> +    pci_bus_irqs(host_bus, ich9_lpc_set_irq, ich9_lpc_map_irq, ich9_lpc,
> +                 ICH9_LPC_NB_PIRQS);
> +
> +    gmps->ram_memory = ram_memory;
> +    gmps->pci_address_space = pci_address_space;
> +    gmps->system_memory = system_memory;
> +    /* pci */
> +    memory_region_init_alias(&gmps->pci_hole, "pci-hole",
> +                             gmps->pci_address_space,
> +                             below_4g_mem_size,
> +                             0x100000000ULL - below_4g_mem_size);
> +    memory_region_add_subregion(gmps->system_memory, below_4g_mem_size,
> +                                &gmps->pci_hole);
> +    pci_hole64_size = (sizeof(hwaddr) == 4 ? 0 :
> +                                            ((uint64_t)1 << 62));
> +    memory_region_init_alias(&gmps->pci_hole_64bit, "pci-hole64",
> +                             gmps->pci_address_space,
> +                             0x100000000ULL + above_4g_mem_size,
> +                             pci_hole64_size);
> +    if (pci_hole64_size) {
> +        memory_region_add_subregion(gmps->system_memory,
> +                                    0x100000000ULL + above_4g_mem_size,
> +                                    &gmps->pci_hole_64bit);
> +    }
> +
> +    /* smram */
> +    memory_region_init_alias(&gmps->smram_region, "smram-region",
> +                             pci_address_space, 0xa0000, 0x20000);
> +    memory_region_add_subregion_overlap(system_memory, 0xa0000,
> +                                        &gmps->smram_region, 1);
> +    memory_region_set_enabled(&gmps->smram_region, false);
> +    init_pam(gmps->ram_memory, gmps->system_memory, gmps->pci_address_space,
> +             &gmps->pam_regions[0], PAM_BIOS_BASE, PAM_BIOS_SIZE);
> +    for (i = 0; i < 12; ++i) {
> +        init_pam(gmps->ram_memory, gmps->system_memory, 
> gmps->pci_address_space,
> +                 &gmps->pam_regions[i+1], PAM_EXPAN_BASE + i * 
> PAM_EXPAN_SIZE,
> +                 PAM_EXPAN_SIZE);
> +    }
> +
> +    *gmch_host_p = gmch_host;
> +    *host_bus_p = host_bus;
> +    *lpc_p = lpc;
> +    *isa_bus = ich9_lpc->isa_bus;
> +}
> +
> +
> +/* set CMOS shutdown status register (index 0xF) as S3_resume(0xFE)
> + *    BIOS will read it and start S3 resume at POST Entry */
> +static void pc_cmos_set_s3_resume(void *opaque, int irq, int level)
> +{
> +    ISADevice *s = opaque;
> +
> +    if (level) {
> +        rtc_set_memory(s, 0xF, 0xFE);
> +    }
> +}
> +
> +static void pc_q35_init_late(BusState **idebus, ISADevice *rtc_state,
> +                             DeviceState *gmch_host, PCIBus *host_bus,
> +                             PCIDevice *lpc)
> +{
> +    qemu_irq *cmos_s3;
> +    PCIDevice *ahci;
> +    DriveInfo **hd;
> +    struct AHCIPCIState *ahci_state;
> +    int ports, max_unit, max_bus;
> +
> +    /* connect pm stuff to lpc */
> +    cmos_s3 = qemu_allocate_irqs(pc_cmos_set_s3_resume, rtc_state, 1);
> +    ich9_lpc_pm_init(lpc, *cmos_s3);
> +
> +    /* ahci and SATA device, for q35 1 ahci controller is built-in */
> +    ahci = pci_create_simple_multifunction(host_bus,
> +                                           PCI_DEVFN(ICH9_SATA1_DEV,
> +                                                     ICH9_SATA1_FUNC),
> +                                           true, "ich9-ahci");
> +    ahci_state = DO_UPCAST(struct AHCIPCIState, card, ahci);
> +    ports = ahci_state->ahci.ports;
> +    max_unit = drive_get_max_unit(IF_AHCI);
> +    if (max_unit >= ports) {
> +        fprintf(stderr, "%d ports are available on the host ahci 
> controller.\n"
> +                        "Ignoring extra assignments.\n", ports);
> +    }
> +    max_bus = drive_get_max_bus(IF_AHCI);
> +    if (max_bus > 0) {
> +        fprintf(stderr, "1 bus is available on the host ahci controller.\n"
> +                        "Ignoring extra assignments.\n");
> +    }
> +    hd = g_malloc(ports * sizeof(DriveInfo *));
> +    ahci_drive_get(hd, 0, ports);
> +    pci_ahci_create_devs(ahci, hd, ports);
> +    idebus[0] = qdev_get_child_bus(&ahci->qdev, "ide.0");
> +    idebus[1] = qdev_get_child_bus(&ahci->qdev, "ide.1");
> +
> +    if (usb_enabled(false)) {
> +        /* Should we create 6 UHCI according to ich9 spec? */
> +        pci_create_simple_multifunction(
> +            host_bus, PCI_DEVFN(ICH9_USB_UHCI1_DEV, ICH9_USB_UHCI1_FUNC),
> +            true, "ich9-usb-uhci1");
> +        /* XXX: EHCI */
> +    }
> +
> +    /* TODO: Populate SPD eeprom data.  */
> +    smbus_eeprom_init(ich9_smb_init(host_bus,
> +                                    PCI_DEVFN(ICH9_SMB_DEV, ICH9_SMB_FUNC),
> +                                    0xb100),
> +                      8, NULL, 0);
> +}
> +
> +/* PC hardware initialisation */
> +static void pc_q35_init(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;
> +    ram_addr_t below_4g_mem_size, above_4g_mem_size;
> +    DeviceState *gmch_host;
> +    PCIBus *host_bus;
> +    PCIDevice *lpc;
> +    BusState *idebus[MAX_SATA_PORTS];
> +    ISADevice *rtc_state;
> +    ISADevice *floppy;
> +    MemoryRegion *pci_memory;
> +    MemoryRegion *rom_memory;
> +    MemoryRegion *ram_memory;
> +    GSIState *gsi_state;
> +    ISABus *isa_bus;
> +    int pci_enabled = 1;
> +    qemu_irq *cpu_irq;
> +    qemu_irq *gsi;
> +    qemu_irq *i8259;

All this should live in a device structure.

> +    int i;
> +
> +    pc_cpus_init(cpu_model);
> +
> +    if (ram_size >= 0xb0000000) {
> +        above_4g_mem_size = ram_size - 0xb0000000;
> +        below_4g_mem_size = 0xb0000000;
> +    } else {
> +        above_4g_mem_size = 0;
> +        below_4g_mem_size = ram_size;
> +    }
> +
> +    /* pci enabled */
> +    if (pci_enabled) {
> +        pci_memory = g_new(MemoryRegion, 1);
> +        memory_region_init(pci_memory, "pci", INT64_MAX);
> +        rom_memory = pci_memory;
> +    } else {
> +        pci_memory = NULL;
> +        rom_memory = get_system_memory();
> +    }
> +
> +    /* allocate ram and load rom/bios */
> +    if (!xen_enabled()) {
> +        pc_memory_init(get_system_memory(), kernel_filename, kernel_cmdline,
> +                       initrd_filename, below_4g_mem_size, above_4g_mem_size,
> +                       rom_memory, &ram_memory);
> +    }
> +
> +    /* irq lines */
> +    gsi_state = g_malloc0(sizeof(*gsi_state));
> +    if (kvm_irqchip_in_kernel()) {
> +        kvm_piix3_setup_irq_routing(pci_enabled);
> +        gsi = qemu_allocate_irqs(kvm_piix3_gsi_handler, gsi_state,
> +                                 GSI_NUM_PINS);
> +    } else {
> +        gsi = qemu_allocate_irqs(gsi_handler, gsi_state, GSI_NUM_PINS);
> +    }
> +
> +    pc_q35_init_early(gsi, gsi_state,
> +                      &gmch_host, &host_bus, &lpc, &isa_bus,
> +                      get_system_memory(), pci_memory, get_system_io(),
> +                      ram_memory, below_4g_mem_size, above_4g_mem_size);
> +    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];
> +    }
> +    if (pci_enabled) {
> +        ioapic_init_gsi(gsi_state, NULL);
> +    }
> +
> +    pc_register_ferr_irq(gsi[13]);
> +
> +    /* init basic PC hardware */
> +    pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy, false);
> +
> +    pc_q35_init_late(idebus, rtc_state, gmch_host, host_bus, lpc);
> +
> +    pc_cmos_init(below_4g_mem_size, above_4g_mem_size, boot_device,
> +                 floppy, idebus[0], idebus[1], rtc_state);
> +
> +    /* the rest devices to which pci devfn is automatically assigned */
> +    pc_vga_init(isa_bus, host_bus);
> +    audio_init(isa_bus, host_bus);
> +    pc_nic_init(isa_bus, host_bus);
> +    if (pci_enabled) {
> +        pc_pci_device_init(host_bus);
> +    }
> +}
> +
> +static QEMUMachine pc_q35_machine = {
> +    .name = "q35-next",
> +    .alias = "q35",
> +    .desc = "Q35 chipset PC",
> +    .init = pc_q35_init,
> +    .max_cpus = 255,
> +    .default_drive_if = IF_AHCI,
> +};
> +
> +static void pc_q35_machine_init(void)
> +{
> +    qemu_register_machine(&pc_q35_machine);
> +}
> +
> +machine_init(pc_q35_machine_init);
> diff --git a/hw/pci_ids.h b/hw/pci_ids.h
> index 17ae42e..ef24181 100644
> --- a/hw/pci_ids.h
> +++ b/hw/pci_ids.h
> @@ -137,6 +137,8 @@
>  #define PCI_DEVICE_ID_INTEL_82801I_EHCI2 0x293c
>  #define PCI_DEVICE_ID_INTEL_82599_SFP_VF 0x10ed
>  
> +#define PCI_DEVICE_ID_INTEL_Q35_MCH      0x29c0
> +
>  #define PCI_VENDOR_ID_XEN               0x5853
>  #define PCI_DEVICE_ID_XEN_PLATFORM      0x0001
>  
> diff --git a/hw/q35.c b/hw/q35.c
> new file mode 100644
> index 0000000..1fd8802
> --- /dev/null
> +++ b/hw/q35.c
> @@ -0,0 +1,315 @@
> +/*
> + * QEMU GMCH/ICH9 PCI Bridge Emulation
> + *
> + * Copyright (c) 2006 Fabrice Bellard
> + * Copyright (c) 2009, 2010, 2011
> + *               Isaku Yamahata <yamahata at valinux co jp>
> + *               VA Linux Systems Japan K.K.
> + * Copyright (C) 2012 Jason Baron <address@hidden>
> + *
> + * This is based on piix_pci.c, but heavily modified.
> + *
> + * 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 "hw.h"
> +#include "range.h"
> +#include "isa.h"
> +#include "sysbus.h"
> +#include "pc.h"
> +#include "apm.h"
> +#include "apic.h"
> +#include "pci.h"
> +#include "pcie_host.h"
> +#include "pci_bridge.h"
> +#include "q35.h"
> +#include "acpi.h"
> +#include "acpi_ich9.h"
> +#include "pam.h"
> +#include "pci_internals.h"
> +#include "exec-memory.h"
> +#include "isa.h"
> +#include "qemu-common.h"
> +#include "ich9.h"
> +
> +
> +
> +/****************************************************************************
> + * GMCH PCI host
> + */
> +
> +static int gmch_pcihost_initfn(SysBusDevice *dev)
> +{
> +    PCIHostState *pci = FROM_SYSBUS(PCIHostState, dev);
> +    GMCHPCIHost *s = GMCH_HOST_DEVICE(&dev->qdev);
> +
> +    memory_region_init_io(&pci->conf_mem, &pci_host_conf_le_ops, pci,
> +                          "pci-conf-idx", 4);
> +    sysbus_add_io(dev, GMCH_HOST_BRIDGE_CONFIG_ADDR, &pci->conf_mem);
> +    sysbus_init_ioports(&pci->busdev, GMCH_HOST_BRIDGE_CONFIG_ADDR, 4);
> +
> +    memory_region_init_io(&pci->data_mem, &pci_host_data_le_ops, pci,
> +                          "pci-conf-data", 4);
> +    sysbus_add_io(dev, GMCH_HOST_BRIDGE_CONFIG_DATA, &pci->data_mem);
> +    sysbus_init_ioports(&pci->busdev, GMCH_HOST_BRIDGE_CONFIG_DATA, 4);
> +
> +    if (pcie_host_init(&s->host) < 0) {
> +        abort();

Just return -1, it has the same affect but nicer errors.

> +    }
> +
> +    return 0;
> +}
> +
> +static Property gmch_props[] = {
> +    DEFINE_PROP_UINT64("MCFG", GMCHPCIHost, host.base_addr,
> +                        GMCH_HOST_BRIDGE_PCIEXBAR_DEFAULT),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void gmch_pcihost_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +
> +    k->init = gmch_pcihost_initfn;
> +    dc->props = gmch_props;
> +    dc->no_user = 1;

no_user is meaningless...

> +}
> +
> +static const TypeInfo gmch_pcihost_info = {
> +    .name       = TYPE_GMCH_HOST_DEVICE,
> +    .parent     = TYPE_PCIE_HOST_BRIDGE,
> +    .instance_size = sizeof(GMCHPCIHost),
> +    .class_init = gmch_pcihost_class_init,
> +};
> +
> +/* host bridge */
> +PCIBus *gmch_host_init(DeviceState **gmch_hostp,
> +                       MemoryRegion *pci_address_space,
> +                       MemoryRegion *address_space_io)
> +{
> +    DeviceState *dev;
> +    GMCHPCIHost *s;
> +    PCIBus *b;
> +
> +    dev = qdev_create(NULL, TYPE_GMCH_HOST_DEVICE);
> +    s = GMCH_HOST_DEVICE(dev);
> +    b = pci_bus_new(&s->host.pci.busdev.qdev, "pcie.0", pci_address_space,
> +                    address_space_io, 0);
> +    s->host.pci.bus = b;
> +    qdev_init_nofail(dev);
> +    *gmch_hostp = dev;
> +    return b;
> +}
> +
> +
> +/****************************************************************************
> + * GMCH D0:F0
> + */
> +
> +/* PCIE MMCFG */
> +static void gmch_update_pciexbar(GMCHPCIState *gs)
> +{
> +    PCIDevice *pci_dev = &gs->d;
> +    BusState *bus = qdev_get_parent_bus(&pci_dev->qdev);
> +    DeviceState *qdev = bus->parent;
> +    GMCHPCIHost *s = GMCH_HOST_DEVICE(qdev);
> +
> +    uint64_t pciexbar;
> +    int enable;
> +    uint64_t addr;
> +    uint64_t addr_mask;
> +    uint32_t length;
> +
> +    pciexbar = pci_get_quad(pci_dev->config + GMCH_HOST_BRIDGE_PCIEXBAR);
> +    enable = pciexbar & GMCH_HOST_BRIDGE_PCIEXBAREN;
> +
> +    addr_mask = GMCH_HOST_BRIDGE_PCIEXBAR_ADMSK;
> +    switch (pciexbar & GMCH_HOST_BRIDGE_PCIEXBAR_LENGTH_MASK) {
> +    case GMCH_HOST_BRIDGE_PCIEXBAR_LENGTH_256M:
> +        length = 256 * 1024 * 1024;
> +        break;
> +    case GMCH_HOST_BRIDGE_PCIEXBAR_LENGTH_128M:
> +        length = 128 * 1024 * 1024;
> +        addr_mask |= GMCH_HOST_BRIDGE_PCIEXBAR_128ADMSK |
> +            GMCH_HOST_BRIDGE_PCIEXBAR_64ADMSK;
> +        break;
> +    case GMCH_HOST_BRIDGE_PCIEXBAR_LENGTH_64M:
> +        length = 64 * 1024 * 1024;
> +        addr_mask |= GMCH_HOST_BRIDGE_PCIEXBAR_64ADMSK;
> +        break;
> +    case GMCH_HOST_BRIDGE_PCIEXBAR_LENGTH_RVD:
> +    default:
> +        enable = 0;
> +        length = 0;
> +        abort();
> +        break;
> +    }
> +    addr = pciexbar & addr_mask;
> +
> +    pcie_host_mmcfg_update(&s->host, enable, addr, length);
> +}
> +
> +/* PAM */
> +static void gmch_update_pam(GMCHPCIState *gs)
> +{
> +    int i;
> +
> +    memory_region_transaction_begin();
> +    for (i = 0; i < 13; i++) {
> +        pam_update(&gs->pam_regions[i], i,
> +                   gs->d.config[GMCH_HOST_BRIDGE_PAM0 + ((i + 1) / 2)]);
> +    }
> +    memory_region_transaction_commit();
> +}
> +
> +/* SMRAM */
> +static void gmch_update_smram(GMCHPCIState *gs)
> +{
> +    memory_region_transaction_begin();
> +    smram_update(&gs->smram_region, gs->d.config[GMCH_HOST_BRDIGE_SMRAM],
> +                    gs->smm_enabled);
> +    memory_region_transaction_commit();
> +}
> +
> +static void gmch_set_smm(int smm, void *arg)
> +{
> +    GMCHPCIState *gs = arg;
> +
> +    memory_region_transaction_begin();
> +    smram_set_smm(&gs->smm_enabled, smm, 
> gs->d.config[GMCH_HOST_BRDIGE_SMRAM],
> +                    &gs->smram_region);
> +    memory_region_transaction_commit();
> +}
> +
> +static void gmch_write_config(PCIDevice *d,
> +                              uint32_t address, uint32_t val, int len)
> +{
> +    GMCHPCIState *gs = GMCH_PCI_DEVICE(d);
> +
> +    /* XXX: implement SMRAM.D_LOCK */
> +    pci_default_write_config(d, address, val, len);
> +
> +    if (ranges_overlap(address, len, GMCH_HOST_BRIDGE_PAM0,
> +                       GMCH_HOST_BRIDGE_PAM_SIZE)) {
> +        gmch_update_pam(gs);
> +    }
> +
> +    if (ranges_overlap(address, len, GMCH_HOST_BRIDGE_PCIEXBAR,
> +                       GMCH_HOST_BRIDGE_PCIEXBAR_SIZE)) {
> +        gmch_update_pciexbar(gs);
> +    }
> +
> +    if (ranges_overlap(address, len, GMCH_HOST_BRDIGE_SMRAM,
> +                       GMCH_HOST_BRDIGE_SMRAM_SIZE)) {
> +        gmch_update_smram(gs);
> +    }
> +}
> +
> +static void gmch_update(GMCHPCIState *gs)
> +{
> +    gmch_update_pciexbar(gs);
> +    gmch_update_pam(gs);
> +    gmch_update_smram(gs);
> +}
> +
> +static int gmch_post_load(void *opaque, int version_id)
> +{
> +    GMCHPCIState *gs = opaque;
> +    gmch_update(gs);
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_gmch = {
> +    .name = "gmch",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .post_load = gmch_post_load,
> +    .fields = (VMStateField []) {
> +        VMSTATE_PCI_DEVICE(d, GMCHPCIState),
> +        VMSTATE_UINT8(smm_enabled, GMCHPCIState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void gmch_reset(DeviceState *qdev)
> +{
> +    PCIDevice *d = PCI_DEVICE(qdev);
> +    GMCHPCIState *gs = GMCH_PCI_DEVICE(d);
> +
> +    pci_set_quad(d->config + GMCH_HOST_BRIDGE_PCIEXBAR,
> +                 GMCH_HOST_BRIDGE_PCIEXBAR_DEFAULT);
> +
> +    d->config[GMCH_HOST_BRDIGE_SMRAM] = GMCH_HOST_BRIDGE_SMRAM_DEFAULT;
> +
> +    gmch_update(gs);
> +}
> +
> +static int pci_gmch_initfn(PCIDevice *d)
> +{
> +    GMCHPCIState *gs = GMCH_PCI_DEVICE(d);
> +
> +    cpu_smm_register(&gmch_set_smm, gs);
> +
> +    return 0;
> +}
> +
> +static void pci_gmch_class_init(ObjectClass *klass, void *data)
> +{
> +    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    k->init = pci_gmch_initfn;
> +    k->config_write = gmch_write_config;
> +    dc->reset = gmch_reset;
> +    dc->desc = "Host bridge";
> +    dc->vmsd = &vmstate_gmch;
> +    dc->no_user = 1;
> +    k->vendor_id = PCI_VENDOR_ID_INTEL;
> +    k->device_id = PCI_DEVICE_ID_INTEL_Q35_MCH;
> +    k->revision = GMCH_HOST_BRIDGE_REVISION_DEFUALT;
> +    k->class_id = PCI_CLASS_BRIDGE_HOST;
> +}
> +
> +static const TypeInfo pci_gmch_info = {
> +    .name = TYPE_GMCH_PCI_DEVICE,
> +    .parent     = TYPE_PCI_DEVICE,
> +    .instance_size = sizeof(GMCHPCIState),
> +    .class_init = pci_gmch_class_init,
> +};

So I think you could pretty easily make GMCH inherit from a generic
Northbridge to get the code sharing I was talking about before.

> +
> +/* host bridge */
> +PCIDevice *gmch_init(DeviceState *gmch_host, PCIBus *b)
> +{
> +    GMCHPCIHost *s = GMCH_HOST_DEVICE(gmch_host);
> +    PCIDevice *d;
> +
> +    d = pci_create_simple_multifunction(b, 0, false, TYPE_GMCH_PCI_DEVICE);
> +    s->dev = d;
> +
> +    return d;
> +}
> +
> +static void q35_register(void)
> +{
> +    type_register_static(&pci_gmch_info);
> +    type_register_static(&gmch_pcihost_info);
> +}
> +
> +type_init(q35_register);
> diff --git a/hw/q35.h b/hw/q35.h
> new file mode 100644
> index 0000000..0b66da7
> --- /dev/null
> +++ b/hw/q35.h
> @@ -0,0 +1,161 @@
> +/*
> + * q35.h
> + *
> + * Copyright (c) 2009 Isaku Yamahata <yamahata at valinux co jp>
> + *                    VA Linux Systems Japan K.K.
> + * Copyright (C) 2012 Jason Baron <address@hidden>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see 
> <http://www.gnu.org/licenses/>
> + */
> +
> +#ifndef HW_Q35_H
> +#define HW_Q35_H
> +
> +#include "hw.h"
> +#include "range.h"
> +#include "isa.h"
> +#include "sysbus.h"
> +#include "pc.h"
> +#include "apm.h"
> +#include "apic.h"
> +#include "pci.h"
> +#include "pcie_host.h"
> +#include "pci_bridge.h"
> +#include "q35.h"
> +#include "acpi.h"
> +#include "acpi_ich9.h"
> +#include "pam.h"
> +#include "pci_internals.h"
> +
> +
> +#define TYPE_GMCH_HOST_DEVICE "gmch-pcihost"
> +#define GMCH_HOST_DEVICE(obj) \
> +     OBJECT_CHECK(GMCHPCIHost, (obj), TYPE_GMCH_HOST_DEVICE)
> +
> +#define TYPE_GMCH_PCI_DEVICE "gmch"
> +#define GMCH_PCI_DEVICE(obj) \
> +     OBJECT_CHECK(GMCHPCIState, (obj), TYPE_GMCH_PCI_DEVICE)
> +
> +typedef struct GMCHPCIHost {
> +    PCIExpressHost      host;
> +    PCIDevice    *dev;
> +} GMCHPCIHost;
> +
> +typedef struct GMCHPCIState {
> +    PCIDevice   d;
> +    /*
> +     * GMCH_PCIHost   *gmch_host;
> +     * In order to get GMCH_PCIHost
> +     *  PCIDevice -> qdev -> parent_bus -> qdev -upcast-> GMCH_PCIHost
> +     */
> +    MemoryRegion *ram_memory;
> +    MemoryRegion *pci_address_space;
> +    MemoryRegion *system_memory;
> +    PAMMemoryRegion pam_regions[13];
> +    MemoryRegion smram_region;
> +    MemoryRegion pci_hole;
> +    MemoryRegion pci_hole_64bit;
> +    uint8_t smm_enabled;
> +} GMCHPCIState;
> +
> +PCIBus *gmch_host_init(DeviceState **gmch_hostp,
> +                       MemoryRegion *pci_address_space,
> +                       MemoryRegion *address_space_io);
> +PCIDevice *gmch_init(DeviceState *gmch_host, PCIBus *b);
> +
> +#define Q35_MASK(bit, ms_bit, ls_bit) \
> +((uint##bit##_t)(((1ULL << ((ms_bit) + 1)) - 1) & ~((1ULL << ls_bit)
> - 1)))

This is pretty wacky!

> +
> +/*
> + * gmch part
> + */
> +
> +/* PCI configuration */
> +#define GMCH_HOST_BRIDGE                        "GMCH"
> +
> +#define GMCH_HOST_BRIDGE_CONFIG_ADDR            0xcf8
> +#define GMCH_HOST_BRIDGE_CONFIG_DATA            0xcfc
> +
> +/* D0:F0 configuration space */
> +#define GMCH_HOST_BRIDGE_REVISION_DEFUALT       0x0
> +
> +#define GMCH_HOST_BRIDGE_PCIEXBAR               0x60    /* 64bit register */
> +#define GMCH_HOST_BRIDGE_PCIEXBAR_SIZE          8       /* 64bit register */
> +#define GMCH_HOST_BRIDGE_PCIEXBAR_DEFAULT       0xe0000000
> +#define GMCH_HOST_BRIDGE_PCIEXBAR_ADMSK         Q35_MASK(64, 35, 25) /* bit 
> 35:28 */
> +#define GMCH_HOST_BRIDGE_PCIEXBAR_128ADMSK      ((uint64_t)(1 << 26))
> +#define GMCH_HOST_BRIDGE_PCIEXBAR_64ADMSK       ((uint64_t)(1 << 25))
> +#define GMCH_HOST_BRIDGE_PCIEXBAR_LENGTH_MASK   ((uint64_t)(0x3 << 1))
> +#define GMCH_HOST_BRIDGE_PCIEXBAR_LENGTH_256M   ((uint64_t)(0x0 << 1))
> +#define GMCH_HOST_BRIDGE_PCIEXBAR_LENGTH_128M   ((uint64_t)(0x1 << 1))
> +#define GMCH_HOST_BRIDGE_PCIEXBAR_LENGTH_64M    ((uint64_t)(0x2 << 1))
> +#define GMCH_HOST_BRIDGE_PCIEXBAR_LENGTH_RVD    ((uint64_t)(0x3 << 1))
> +#define GMCH_HOST_BRIDGE_PCIEXBAREN             ((uint64_t)1)

The way these defines are setup are strange...  Why isn't everythign
using Q35_MASK?

Regards,

Anthony Liguori

> +
> +#define GMCH_HOST_BRIDGE_PAM_NB                 7
> +#define GMCH_HOST_BRIDGE_PAM_SIZE               7
> +#define GMCH_HOST_BRIDGE_PAM0                   0x90
> +#define GMCH_HOST_BRIDGE_PAM_BIOS_AREA          0xf0000
> +#define GMCH_HOST_BRIDGE_PAM_AREA_SIZE          0x10000 /* 16KB */
> +#define GMCH_HOST_BRIDGE_PAM1                   0x91
> +#define GMCH_HOST_BRIDGE_PAM_EXPAN_AREA         0xc0000
> +#define GMCH_HOST_BRIDGE_PAM_EXPAN_SIZE         0x04000
> +#define GMCH_HOST_BRIDGE_PAM2                   0x92
> +#define GMCH_HOST_BRIDGE_PAM3                   0x93
> +#define GMCH_HOST_BRIDGE_PAM4                   0x94
> +#define GMCH_HOST_BRIDGE_PAM_EXBIOS_AREA        0xe0000
> +#define GMCH_HOST_BRIDGE_PAM_EXBIOS_SIZE        0x04000
> +#define GMCH_HOST_BRIDGE_PAM5                   0x95
> +#define GMCH_HOST_BRIDGE_PAM6                   0x96
> +#define GMCH_HOST_BRIDGE_PAM_WE_HI              ((uint8_t)(0x2 << 4))
> +#define GMCH_HOST_BRIDGE_PAM_RE_HI              ((uint8_t)(0x1 << 4))
> +#define GMCH_HOST_BRIDGE_PAM_HI_MASK            ((uint8_t)(0x3 << 4))
> +#define GMCH_HOST_BRIDGE_PAM_WE_LO              ((uint8_t)0x2)
> +#define GMCH_HOST_BRIDGE_PAM_RE_LO              ((uint8_t)0x1)
> +#define GMCH_HOST_BRIDGE_PAM_LO_MASK            ((uint8_t)0x3)
> +#define GMCH_HOST_BRIDGE_PAM_WE                 ((uint8_t)0x2)
> +#define GMCH_HOST_BRIDGE_PAM_RE                 ((uint8_t)0x1)
> +#define GMCH_HOST_BRIDGE_PAM_MASK               ((uint8_t)0x3)
> +
> +#define GMCH_HOST_BRDIGE_SMRAM                  0x9d
> +#define GMCH_HOST_BRDIGE_SMRAM_SIZE             1
> +#define GMCH_HOST_BRIDGE_SMRAM_DEFAULT          ((uint8_t)0x2)
> +#define GMCH_HOST_BRIDGE_SMRAM_D_OPEN           ((uint8_t)(1 << 6))
> +#define GMCH_HOST_BRIDGE_SMRAM_D_CLS            ((uint8_t)(1 << 5))
> +#define GMCH_HOST_BRIDGE_SMRAM_D_LCK            ((uint8_t)(1 << 4))
> +#define GMCH_HOST_BRIDGE_SMRAM_G_SMRAME         ((uint8_t)(1 << 3))
> +#define GMCH_HOST_BRIDGE_SMRAM_C_BASE_SEG_MASK  ((uint8_t)0x7)
> +#define GMCH_HOST_BRIDGE_SMRAM_C_BASE_SEG       ((uint8_t)0x2)  /* hardwired 
> to b010 */
> +#define GMCH_HOST_BRIDGE_SMRAM_C_BASE           0xa0000
> +#define GMCH_HOST_BRIDGE_SMRAM_C_END            0xc0000
> +#define GMCH_HOST_BRIDGE_SMRAM_C_SIZE           0x20000
> +#define GMCH_HOST_BRIDGE_UPPER_SYSTEM_BIOS_END  0x100000
> +
> +#define GMCH_HOST_BRIDGE_ESMRAMC                0x9e
> +#define GMCH_HOST_BRDIGE_ESMRAMC_H_SMRAME       ((uint8_t)(1 << 6))
> +#define GMCH_HOST_BRDIGE_ESMRAMC_E_SMERR        ((uint8_t)(1 << 5))
> +#define GMCH_HOST_BRDIGE_ESMRAMC_SM_CACHE       ((uint8_t)(1 << 4))
> +#define GMCH_HOST_BRDIGE_ESMRAMC_SM_L1          ((uint8_t)(1 << 3))
> +#define GMCH_HOST_BRDIGE_ESMRAMC_SM_L2          ((uint8_t)(1 << 2))
> +#define GMCH_HOST_BRDIGE_ESMRAMC_TSEG_SZ_MASK   ((uint8_t)(0x3 << 1))
> +#define GMCH_HOST_BRDIGE_ESMRAMC_TSEG_SZ_1MB    ((uint8_t)(0x0 << 1))
> +#define GMCH_HOST_BRDIGE_ESMRAMC_TSEG_SZ_2MB    ((uint8_t)(0x1 << 1))
> +#define GMCH_HOST_BRDIGE_ESMRAMC_TSEG_SZ_8MB    ((uint8_t)(0x2 << 1))
> +#define GMCH_HOST_BRDIGE_ESMRAMC_T_EN           ((uint8_t)1)
> +
> +/* D1:F0 PCIE* port*/
> +#define GMCH_PCIE_DEV                           1
> +#define GMCH_PCIE_FUNC                          0
> +
> +#endif /* HW_Q35_H */
> -- 
> 1.7.1