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

Vasilis Liaskovitis vasilis.liaskovitis at profitbricks.com
Wed Jan 16 18:10:29 CET 2013


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



More information about the SeaBIOS mailing list