Re: [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 <address@hidden>
>>> ---
>>>  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 <address@hidden>
> ---
>  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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg