Re: [SeaBIOS] [Qemu-devel] [RFC PATCH v4 13/30] piix_pci and pc_piix: refactor

[Andreas Färber]

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 <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.

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 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 ++++++++++++++-------------------------------------------
>  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.
>    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.

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.
>        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?
>  }
>
>  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?
> +}
> 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:

PCIHostState *phb = PCI_HOST_BRIDGE(dev);
> I440FXState *s = 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);
>
> 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.
> 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.
>  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.

[...]
> @@ -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.
>  .instance_size = sizeof(I440FXState),
> .instance_init = i440fx_pcihost_initfn,
> .class_init    = i440fx_pcihost_class_init,
>
> };

[snip]

Regards,
Andreas

[Qemu-devel] [RFC PATCH v4 13/30] piix_pci and pc_piix: refactor

[Vasilis Liaskovitis]

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
> > > >
> > > > 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