Hi,
On Wed, Jan 16, 2013 at 12:17:05PM +0100, Andreas Färber wrote:
> 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 <vasilis.liaskovi...@profitbricks.com>
> >>> ---
> >>> 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 <xen/hvm/hvm_info_table.h>
> >>> #endif
> >>> +#include "piix_pci.h"
> >>
> >> Can't find this file. Did you forget to add this file to git?
> >
> > sorry, you are right. Below is the corrected patch with the missing header
>
> Please take review comments on other similar series into account. You
> can also check if the QOM Vadis slides from KVM Forum are online somewhere.
thanks, I will take a look.
>
> You are aware that there were two people previously working on
> QOM'ifying i440fx?
I am aware of Anthony's i440fx-pmc patchset from about a year ago (a few months
ago I asked if it would be respinned, but got no response iirc, so I am not sure
what the status is)
What's the second effort you mention and its status? Are prep_pci patchsets
going to address this in the future? I don't mean to step on other people's
work-in-progress, so I don't mind dropping this patch if one of the other
efforts is still active.
> > ---
> > 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
> >
> > 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,
> > }
> >
> > if (pci_enabled) {
> > - pci_bus = 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) == 4
> > - ? 0
> > - : ((uint64_t)1 << 62)),
> > - pci_memory, ram_memory);
> > + i440fx_host = I440FX_HOST_DEVICE(qdev_create(NULL,
> > + TYPE_I440FX_HOST_DEVICE));
>
> Elsewhere it was requested to use _HOST_BRIDGE wording.
ok
>
> > + i440fx_host->mch.ram_memory = ram_memory;
> > + i440fx_host->mch.pci_address_space = pci_memory;
> > + i440fx_host->mch.system_memory = get_system_memory();
> > + i440fx_host->mch.address_space_io = get_system_io();;
> > + i440fx_host->mch.below_4g_mem_size = below_4g_mem_size;
> > + i440fx_host->mch.above_4g_mem_size = above_4g_mem_size;
> > +
> > + qdev_init_nofail(DEVICE(i440fx_host));
> > + i440fx_state = &i440fx_host->mch;
> > + pci_bus = 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.
ok
>
> PCIHostState *phb = PCI_HOST_BRIDGE(i440fx_host);
> ...
> pci_bus = phb->bus;
>
> > + /* Xen supports additional interrupt routes from the PCI devices 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 = 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.
Any examples of this? I assume the new macros should't use DO_UPCAST themselves.
>
> > + pci_bus_irqs(pci_bus, xen_piix3_set_irq, xen_pci_slot_get_pirq,
> > + piix3, XEN_PIIX_NUM_PIRQS);
> > + } else {
> > + piix3 = 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, piix3,
> > + PIIX_NUM_PIRQS);
> > + pci_bus_set_route_irq_fn(pci_bus, piix3_route_intx_pin_to_irq);
> > + }
> > + piix3->pic = gsi;
> > + isa_bus = DO_UPCAST(ISABus, qbus,
> > + qdev_get_child_bus(&piix3->dev.qdev, "isa.0"));
>
> isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(piix3), ...));
>
> > +
> > + piix3_devfn = piix3->dev.devfn;
> > +
> > + ram_size = ram_size / 8 / 1024 / 1024;
> > + if (ram_size > 255) {
> > + ram_size = 255;
> > + }
> > + i440fx_state->dev.config[0x57] = ram_size;
> > } else {
> > pci_bus = NULL;
> > - i440fx_state = NULL;
> > isa_bus = isa_bus_new(NULL, system_io);
> > no_hpet = 1;
> > }
> > +
> > isa_bus_irqs(isa_bus, gsi);
> >
> > if (kvm_irqchip_in_kernel()) {
> > @@ -157,7 +192,7 @@ static void pc_init1(MemoryRegion *system_memory,
> > gsi_state->i8259_irq[i] = i8259[i];
> > }
> > if (pci_enabled) {
> > - ioapic_init_gsi(gsi_state, "i440fx");
> > + ioapic_init_gsi(gsi_state, NULL);
>
> Unrelated? Why?
I was getting a segfault in object_propert_add_child because the path to
"i440fx" was resolved to a NULL object. This was just a quick fix - I think the
real issue is that the name of the i440fx-host device is not yet set in the new
code. I 'll try to fix.
>
> > }
> >
> > 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"
> >
> > -/*
> > - * 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 it
> > - * 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 = 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);
> >
> > /* 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 = (pci_dev->devfn >> 3) - 1;
> > @@ -180,149 +125,86 @@ static const VMStateDescription vmstate_i440fx = {
> > }
> > };
> >
> > -static int i440fx_pcihost_initfn(SysBusDevice *dev)
> > +static void i440fx_pcihost_initfn(Object *obj)
> > {
> > - PCIHostState *s = PCI_HOST_BRIDGE(dev);
> > + I440FXState *s = 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?
"mem-controller" maybe? although q35 also uses an "mch" property for its
memory controller.
>
> > +}
> >
> > - 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 = FROM_SYSBUS(PCIHostState, dev);
>
> Don't use FROM_SYSBUS() either:
Is a new macro required here as well?
>
> PCIHostState *phb = PCI_HOST_BRIDGE(dev);
>
> > + I440FXState *s = I440FX_HOST_DEVICE(&dev->qdev);
>
> No need to access ->qdev, just use I440FX_...(dev);
ok.
>
> > + 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);
> >
> > - 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 = pci_bus_new(&s->parent_obj.busdev.qdev, NULL,
> > s->mch.pci_address_space,
>
> DEVICE(dev)
>
> > + s->mch.address_space_io, 0);
>
> Initializing the bus in-place would be preferred.
Do you mean pci_bus_new_inplace? Why is this preferred?
>
> > + s->parent_obj.bus = 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.
ok
>
> >
> > return 0;
> > }
> >
> > static int i440fx_initfn(PCIDevice *dev)
> > {
> > - PCII440FXState *d = DO_UPCAST(PCII440FXState, dev, dev);
> > + int i;
> > + PCII440FXState *f = DO_UPCAST(PCII440FXState, dev, dev);
> > + hwaddr pci_hole64_size;
> >
> > - d->dev.config[I440FX_SMRAM] = 0x02;
> > + f->dev.config[I440FX_SMRAM] = 0x02;
> >
> > - 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.
yes, this renaming here isn't needed, I 'll omit it.
>
> [...]
> > @@ -550,7 +432,7 @@ static void i440fx_class_init(ObjectClass *klass, void
> > *data)
> > }
> >
> > static const TypeInfo i440fx_info = {
> > - .name = "i440FX",
> > + .name = TYPE_I440FX_PCI_DEVICE,
> > .parent = TYPE_PCI_DEVICE,
>
> This matches the _PCI_DEVICE naming in earlier series including prep_pci
>
> > .instance_size = sizeof(PCII440FXState),
> > .class_init = i440fx_class_init,
> > @@ -561,15 +443,16 @@ static void i440fx_pcihost_class_init(ObjectClass
> > *klass, void *data)
> > DeviceClass *dc = DEVICE_CLASS(klass);
> > SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> >
> > - k->init = i440fx_pcihost_initfn;
> > + k->init = i440fx_pcihost_init;
> > dc->fw_name = "pci";
> > dc->no_user = 1;
> > }
> >
> > static const TypeInfo i440fx_pcihost_info = {
> > - .name = "i440FX-pcihost",
> > + .name = TYPE_I440FX_HOST_DEVICE,
> > .parent = TYPE_PCI_HOST_BRIDGE,
>
> whereas here you see the mentioned _HOST_DEVICE vs. _HOST_BRIDGE.
>
so, TYPE_I440FX_HOST_BRIDGE for the name is preferred?
thanks,
- Vasilis
