From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:44882) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TvQzb-0002pI-Lv for qemu-devel@nongnu.org; Wed, 16 Jan 2013 06:17:18 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TvQzZ-0000RK-7E for qemu-devel@nongnu.org; Wed, 16 Jan 2013 06:17:15 -0500 Received: from cantor2.suse.de ([195.135.220.15]:44474 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TvQzY-0000Qw-N2 for qemu-devel@nongnu.org; Wed, 16 Jan 2013 06:17:13 -0500 Message-ID: <50F68C31.70006@suse.de> Date: Wed, 16 Jan 2013 12:17:05 +0100 From: =?ISO-8859-1?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1355834518-17989-1-git-send-email-vasilis.liaskovitis@profitbricks.com> <1355834518-17989-14-git-send-email-vasilis.liaskovitis@profitbricks.com> <20130116072040.GE15251@localhost.localdomain> <20130116093638.GA6141@dhcp-192-168-178-175.profitbricks.localdomain> In-Reply-To: <20130116093638.GA6141@dhcp-192-168-178-175.profitbricks.localdomain> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [RFC PATCH v4 13/30] piix_pci and pc_piix: refactor List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vasilis Liaskovitis Cc: pingfank@linux.vnet.ibm.com, gleb@redhat.com, Hu Tao , jbaron@redhat.com, seabios@seabios.org, qemu-devel@nongnu.org, blauwirbel@gmail.com, kevin@koconnor.net, kraxel@redhat.com, anthony@codemonkey.ws, stefanha@gmail.com Hi, Am 16.01.2013 10:36, schrieb Vasilis Liaskovitis: > On Wed, Jan 16, 2013 at 03:20:40PM +0800, Hu Tao wrote: >> On Tue, Dec 18, 2012 at 01:41:41PM +0100, Vasilis Liaskovitis wrote: >>> Refactor code so that chipset initialization is similar to q35. This = will >>> allow memory map initialization at chipset qdev init time for both >>> machines, as well as more similar code structure overall. >>> >>> Signed-off-by: Vasilis Liaskovitis >>> --- >>> hw/pc_piix.c | 57 ++++++++++++--- >>> hw/piix_pci.c | 225 ++++++++++++++---------------------------------= ---------- >>> 2 files changed, 100 insertions(+), 182 deletions(-) >>> >>> diff --git a/hw/pc_piix.c b/hw/pc_piix.c >>> index 19e342a..6a9b508 100644 >>> --- a/hw/pc_piix.c >>> +++ b/hw/pc_piix.c >>> @@ -47,6 +47,7 @@ >>> #ifdef CONFIG_XEN >>> # include >>> #endif >>> +#include "piix_pci.h" >> >> Can't find this file. Did you forget to add this file to git? >=20 > sorry, you are right. Below is the corrected patch with the missing hea= der Please take review comments on other similar series into account. You can also check if the QOM Vadis slides from KVM Forum are online somewher= e. You are aware that there were two people previously working on QOM'ifying i440fx? > Refactor code so that chipset initialization is similar to q35. This wi= ll > allow memory map initialization at chipset qdev init time for both > machines, as well as more similar code structure overall. >=20 > Signed-off-by: Vasilis Liaskovitis > --- > hw/pc_piix.c | 57 ++++++++++++--- > hw/piix_pci.c | 225 ++++++++++++++-----------------------------------= -------- > hw/piix_pci.h | 116 +++++++++++++++++++++++++++++ > 3 files changed, 216 insertions(+), 182 deletions(-) > create mode 100644 hw/piix_pci.h >=20 > diff --git a/hw/pc_piix.c b/hw/pc_piix.c > index 19e342a..6a9b508 100644 > --- a/hw/pc_piix.c > +++ b/hw/pc_piix.c [...] > @@ -127,21 +130,53 @@ static void pc_init1(MemoryRegion *system_memory, > } > =20 > if (pci_enabled) { > - pci_bus =3D i440fx_init(&i440fx_state, &piix3_devfn, &isa_bus,= gsi, > - system_memory, system_io, ram_size, > - below_4g_mem_size, > - 0x100000000ULL - below_4g_mem_size, > - 0x100000000ULL + above_4g_mem_size, > - (sizeof(hwaddr) =3D=3D 4 > - ? 0 > - : ((uint64_t)1 << 62)), > - pci_memory, ram_memory); > + i440fx_host =3D I440FX_HOST_DEVICE(qdev_create(NULL, > + TYPE_I440FX_HOST_DEVICE)); Elsewhere it was requested to use _HOST_BRIDGE wording. > + i440fx_host->mch.ram_memory =3D ram_memory; > + i440fx_host->mch.pci_address_space =3D pci_memory; > + i440fx_host->mch.system_memory =3D get_system_memory(); > + i440fx_host->mch.address_space_io =3D get_system_io();; > + i440fx_host->mch.below_4g_mem_size =3D below_4g_mem_size; > + i440fx_host->mch.above_4g_mem_size =3D above_4g_mem_size; > + > + qdev_init_nofail(DEVICE(i440fx_host)); > + i440fx_state =3D &i440fx_host->mch; > + pci_bus =3D i440fx_host->parent_obj.bus; Please don't access the parent field, in particular not "parent_obj". It was specifically renamed after checking that no more users exist. PCIHostState *phb =3D PCI_HOST_BRIDGE(i440fx_host); ... pci_bus =3D phb->bus; > + /* Xen supports additional interrupt routes from the PCI devic= es to > + * the IOAPIC: the four pins of each PCI device on the bus are= also > + * connected to the IOAPIC directly. > + * These additional routes can be discovered through ACPI. */ > + if (xen_enabled()) { > + piix3 =3D DO_UPCAST(PIIX3State, dev, > + pci_create_simple_multifunction(pci_bus, -1, true, > + "PIIX3-xen")); Please don't introduce new usages of DO_UPCAST() with QOM types. Instead add QOM cast macros where needed and use them. > + pci_bus_irqs(pci_bus, xen_piix3_set_irq, xen_pci_slot_get_= pirq, > + piix3, XEN_PIIX_NUM_PIRQS); > + } else { > + piix3 =3D DO_UPCAST(PIIX3State, dev, > + pci_create_simple_multifunction(pci_bus, -1, true, > + "PIIX3")); > + pci_bus_irqs(pci_bus, piix3_set_irq, pci_slot_get_pirq, pi= ix3, > + PIIX_NUM_PIRQS); > + pci_bus_set_route_irq_fn(pci_bus, piix3_route_intx_pin_to_= irq); > + } > + piix3->pic =3D gsi; > + isa_bus =3D DO_UPCAST(ISABus, qbus, > + qdev_get_child_bus(&piix3->dev.qdev, "isa.0")); isa_bus =3D ISA_BUS(qdev_get_child_bus(DEVICE(piix3), ...)); > + > + piix3_devfn =3D piix3->dev.devfn; > + > + ram_size =3D ram_size / 8 / 1024 / 1024; > + if (ram_size > 255) { > + ram_size =3D 255; > + } > + i440fx_state->dev.config[0x57] =3D ram_size; > } else { > pci_bus =3D NULL; > - i440fx_state =3D NULL; > isa_bus =3D isa_bus_new(NULL, system_io); > no_hpet =3D 1; > } > + > isa_bus_irqs(isa_bus, gsi); > =20 > if (kvm_irqchip_in_kernel()) { > @@ -157,7 +192,7 @@ static void pc_init1(MemoryRegion *system_memory, > gsi_state->i8259_irq[i] =3D i8259[i]; > } > if (pci_enabled) { > - ioapic_init_gsi(gsi_state, "i440fx"); > + ioapic_init_gsi(gsi_state, NULL); Unrelated? Why? > } > =20 > pc_register_ferr_irq(gsi[13]); > diff --git a/hw/piix_pci.c b/hw/piix_pci.c > index ba1b3de..7ca3c73 100644 > --- a/hw/piix_pci.c > +++ b/hw/piix_pci.c > @@ -31,70 +31,15 @@ > #include "range.h" > #include "xen.h" > #include "pam.h" > +#include "piix_pci.h" > =20 > -/* > - * I440FX chipset data sheet. > - * http://download.intel.com/design/chipsets/datashts/29054901.pdf > - */ > - > -typedef struct I440FXState { > - PCIHostState parent_obj; > -} I440FXState; > - > -#define PIIX_NUM_PIC_IRQS 16 /* i8259 * 2 */ > -#define PIIX_NUM_PIRQS 4ULL /* PIRQ[A-D] */ > -#define XEN_PIIX_NUM_PIRQS 128ULL > -#define PIIX_PIRQC 0x60 > - > -typedef struct PIIX3State { > - PCIDevice dev; > - > - /* > - * bitmap to track pic levels. > - * The pic level is the logical OR of all the PCI irqs mapped to i= t > - * So one PIC level is tracked by PIIX_NUM_PIRQS bits. > - * > - * PIRQ is mapped to PIC pins, we track it by > - * PIIX_NUM_PIRQS * PIIX_NUM_PIC_IRQS =3D 64 bits with > - * pic_irq * PIIX_NUM_PIRQS + pirq > - */ > -#if PIIX_NUM_PIC_IRQS * PIIX_NUM_PIRQS > 64 > -#error "unable to encode pic state in 64bit in pic_levels." > -#endif > - uint64_t pic_levels; > - > - qemu_irq *pic; > - > - /* This member isn't used. Just for save/load compatibility */ > - int32_t pci_irq_levels_vmstate[PIIX_NUM_PIRQS]; > -} PIIX3State; > - > -struct PCII440FXState { > - PCIDevice dev; > - MemoryRegion *system_memory; > - MemoryRegion *pci_address_space; > - MemoryRegion *ram_memory; > - MemoryRegion pci_hole; > - MemoryRegion pci_hole_64bit; > - PAMMemoryRegion pam_regions[13]; > - MemoryRegion smram_region; > - uint8_t smm_enabled; > -}; > - > - > -#define I440FX_PAM 0x59 > -#define I440FX_PAM_SIZE 7 > -#define I440FX_SMRAM 0x72 > - > -static void piix3_set_irq(void *opaque, int pirq, int level); > -static PCIINTxRoute piix3_route_intx_pin_to_irq(void *opaque, int pci_= intx); > static void piix3_write_config_xen(PCIDevice *dev, > uint32_t address, uint32_t val, int len= ); > =20 > /* return the global irq number corresponding to a given device irq > pin. We could also use the bus number to have a more precise > mapping. */ > -static int pci_slot_get_pirq(PCIDevice *pci_dev, int pci_intx) > +int pci_slot_get_pirq(PCIDevice *pci_dev, int pci_intx) > { > int slot_addend; > slot_addend =3D (pci_dev->devfn >> 3) - 1; > @@ -180,149 +125,86 @@ static const VMStateDescription vmstate_i440fx =3D= { > } > }; > =20 > -static int i440fx_pcihost_initfn(SysBusDevice *dev) > +static void i440fx_pcihost_initfn(Object *obj) > { > - PCIHostState *s =3D PCI_HOST_BRIDGE(dev); > + I440FXState *s =3D I440FX_HOST_DEVICE(obj); > + object_initialize(&s->mch, TYPE_I440FX_PCI_DEVICE); > + object_property_add_child(OBJECT(s), "mch", OBJECT(&s->mch), NULL)= ; Is there maybe a more readable property name? > +} > =20 > - memory_region_init_io(&s->conf_mem, &pci_host_conf_le_ops, s, > - "pci-conf-idx", 4); > - sysbus_add_io(dev, 0xcf8, &s->conf_mem); > - sysbus_init_ioports(&s->busdev, 0xcf8, 4); > +static int i440fx_pcihost_init(SysBusDevice *dev) > +{ > + PCIHostState *pci =3D FROM_SYSBUS(PCIHostState, dev); Don't use FROM_SYSBUS() either: PCIHostState *phb =3D PCI_HOST_BRIDGE(dev); > + I440FXState *s =3D I440FX_HOST_DEVICE(&dev->qdev); No need to access ->qdev, just use I440FX_...(dev); > + PCIBus *b; > + > + memory_region_init_io(&pci->conf_mem, &pci_host_conf_le_ops, pci, > + "pci-conf-idx", 4); > + sysbus_add_io(dev, 0xcf8, &pci->conf_mem); > + sysbus_init_ioports(&pci->busdev, 0xcf8, 4); > + memory_region_init_io(&pci->data_mem, &pci_host_data_le_ops, pci, > + "pci-conf-data", 4); > =20 > - memory_region_init_io(&s->data_mem, &pci_host_data_le_ops, s, > - "pci-conf-data", 4); > - sysbus_add_io(dev, 0xcfc, &s->data_mem); > - sysbus_init_ioports(&s->busdev, 0xcfc, 4); > + sysbus_add_io(dev, 0xcfc, &pci->data_mem); > + sysbus_init_ioports(&pci->busdev, 0xcfc, 4); > + > + b =3D pci_bus_new(&s->parent_obj.busdev.qdev, NULL, s->mch.pci_add= ress_space, DEVICE(dev) > + s->mch.address_space_io, 0); Initializing the bus in-place would be preferred. > + s->parent_obj.bus =3D b; > + qdev_set_parent_bus(DEVICE(&s->mch), BUS(b)); > + qdev_init_nofail(DEVICE(&s->mch)); When casts other than OBJECT() are used multiple times, a variable is preferred. > =20 > return 0; > } > =20 > static int i440fx_initfn(PCIDevice *dev) > { > - PCII440FXState *d =3D DO_UPCAST(PCII440FXState, dev, dev); > + int i; > + PCII440FXState *f =3D DO_UPCAST(PCII440FXState, dev, dev); > + hwaddr pci_hole64_size; > =20 > - d->dev.config[I440FX_SMRAM] =3D 0x02; > + f->dev.config[I440FX_SMRAM] =3D 0x02; > =20 > - cpu_smm_register(&i440fx_set_smm, d); > - return 0; > -} > + cpu_smm_register(&i440fx_set_smm, f); Is all this d -> f variable renaming really necessary? I can understand the s -> pci (or for less ambiguity: phb) renaming above (I believe I left it s to keep my patch small ;)), but here no new variable is introduced so it just seems to enlarge the patch. [...] > @@ -550,7 +432,7 @@ static void i440fx_class_init(ObjectClass *klass, v= oid *data) > } > =20 > static const TypeInfo i440fx_info =3D { > - .name =3D "i440FX", > + .name =3D TYPE_I440FX_PCI_DEVICE, > .parent =3D TYPE_PCI_DEVICE, This matches the _PCI_DEVICE naming in earlier series including prep_pci > .instance_size =3D sizeof(PCII440FXState), > .class_init =3D i440fx_class_init, > @@ -561,15 +443,16 @@ static void i440fx_pcihost_class_init(ObjectClass= *klass, void *data) > DeviceClass *dc =3D DEVICE_CLASS(klass); > SysBusDeviceClass *k =3D SYS_BUS_DEVICE_CLASS(klass); > =20 > - k->init =3D i440fx_pcihost_initfn; > + k->init =3D i440fx_pcihost_init; > dc->fw_name =3D "pci"; > dc->no_user =3D 1; > } > =20 > static const TypeInfo i440fx_pcihost_info =3D { > - .name =3D "i440FX-pcihost", > + .name =3D TYPE_I440FX_HOST_DEVICE, > .parent =3D TYPE_PCI_HOST_BRIDGE, whereas here you see the mentioned _HOST_DEVICE vs. _HOST_BRIDGE. > .instance_size =3D sizeof(I440FXState), > + .instance_init =3D i440fx_pcihost_initfn, > .class_init =3D i440fx_pcihost_class_init, > }; > =20 [snip] Regards, Andreas --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg