Re: [Qemu-ppc] [Qemu-devel] [PATCH 7/7] pseries: Configure PCI bridge using properties

[Andreas Färber]

Am 28.02.2012 04:18, schrieb David Gibson:
> From: Alexey Kardashevskiy <address@hidden>
> 
> Currently, the function spapr_create_phb() uses its parameters to
> initialize the correct memory windows for the new PCI Host Bridge
> (PHB).  This is not the way things are supposed to be done with qdevs,
> and means you can't create extra PHBs easily using -device.
> 
> Since pSeries machines can and do have many PHBs with various
> configurations, this is a real limitation, not just a theoretical.
> This patch, therefore, alters the PHB initialization code to use qdev
> properties to set these parameters of the new bridge, moving most of
> the code from spapr_create_phb() to spapr_phb_init().
> 
> While we're at it, we change the naming of each PCI bus and its
> associated memory regions to be less arbitrary and make it easier to
> relate the guest and qemu views of memory to each other.
> 
> Signed-off-by: Alexey Kardashevskiy <address@hidden>
> Signed-off-by: David Gibson <address@hidden>
> ---
>  hw/spapr.c     |    2 +-
>  hw/spapr_pci.c |  149 ++++++++++++++++++++++++++++++-------------------------
>  hw/spapr_pci.h |    5 +-
>  3 files changed, 84 insertions(+), 72 deletions(-)

> diff --git a/hw/spapr_pci.c b/hw/spapr_pci.c
> index b0254ee..654cb56 100644
> --- a/hw/spapr_pci.c
> +++ b/hw/spapr_pci.c
> @@ -161,49 +161,6 @@ static void pci_spapr_set_irq(void *opaque, int irq_num, 
> int level)
>      qemu_set_irq(phb->lsi_table[irq_num].qirq, level);
>  }
>  
> -static int spapr_phb_init(SysBusDevice *s)
> -{
> -    sPAPRPHBState *phb = FROM_SYSBUS(sPAPRPHBState, s);
> -    int i;
> -
> -    /* Initialize the LSI table */
> -    for (i = 0; i < SPAPR_PCI_NUM_LSI; i++) {
> -        qemu_irq qirq;
> -        uint32_t num;
> -
> -        qirq = spapr_allocate_lsi(0, &num);
> -        if (!qirq) {
> -            return -1;
> -        }
> -
> -        phb->lsi_table[i].dt_irq = num;
> -        phb->lsi_table[i].qirq = qirq;
> -    }
> -
> -    return 0;
> -}
> -
> -static void spapr_phb_class_init(ObjectClass *klass, void *data)
> -{
> -    SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
> -
> -    sdc->init = spapr_phb_init;
> -}
> -
> -static TypeInfo spapr_phb_info = {
> -    .name          = "spapr-pci-host-bridge",
> -    .parent        = TYPE_SYS_BUS_DEVICE,
> -    .instance_size = sizeof(sPAPRPHBState),
> -    .class_init    = spapr_phb_class_init,
> -};
> -
> -static void spapr_register_types(void)
> -{
> -    type_register_static(&spapr_phb_info);
> -}
> -
> -type_init(spapr_register_types)
> -
>  static uint64_t spapr_io_read(void *opaque, target_phys_addr_t addr,
>                                unsigned size)
>  {

> @@ -297,14 +248,76 @@ void spapr_create_phb(sPAPREnvironment *spapr,
>                                                   PCI_DEVFN(0, 0),
>                                                   SPAPR_PCI_NUM_LSI);
>  
> +    QLIST_INSERT_HEAD(&spapr->phbs, phb, list);
> +
> +    /* Initialize the LSI table */
> +    for (i = 0; i < SPAPR_PCI_NUM_LSI; i++) {
> +        qemu_irq qirq;
> +        uint32_t num;
> +
> +        qirq = spapr_allocate_lsi(0, &num);
> +        if (!qirq) {
> +            return -1;
> +        }
> +
> +        phb->lsi_table[i].dt_irq = num;
> +        phb->lsi_table[i].qirq = qirq;
> +    }
> +
> +    return 0;
> +}
> +
> +static Property spapr_phb_properties[] = {
> +    DEFINE_PROP_HEX64("buid", sPAPRPHBState, buid, 0),
> +    DEFINE_PROP_HEX64("mem_win_addr", sPAPRPHBState, mem_win_addr, 0),
> +    DEFINE_PROP_HEX64("mem_win_size", sPAPRPHBState, mem_win_size, 
> 0x20000000),
> +    DEFINE_PROP_HEX64("io_win_addr", sPAPRPHBState, io_win_addr, 0),
> +    DEFINE_PROP_HEX64("io_win_size", sPAPRPHBState, io_win_size, 0x10000),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void spapr_phb_class_init(ObjectClass *klass, void *data)
> +{
> +    SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    sdc->init = spapr_phb_init;
> +    dc->props = spapr_phb_properties;
> +}
> +
> +static TypeInfo spapr_phb_info = {
> +    .name          = "spapr-pci-host-bridge",
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(sPAPRPHBState),
> +    .class_init    = spapr_phb_class_init,
> +};
> +
> +static void spapr_register_pci_type(void)
> +{
> +    type_register_static(&spapr_phb_info);
> +
>      spapr_rtas_register("read-pci-config", rtas_read_pci_config);
>      spapr_rtas_register("write-pci-config", rtas_write_pci_config);
>      spapr_rtas_register("ibm,read-pci-config", rtas_ibm_read_pci_config);
>      spapr_rtas_register("ibm,write-pci-config", rtas_ibm_write_pci_config);
> +}
>  
> -    QLIST_INSERT_HEAD(&spapr->phbs, phb, list);
> +type_init(spapr_register_pci_type)

Please respect the recently enforced convention of naming the function
..._register_types as before and best putting it and type_init() in the
bottom so that we don't end up with multiple ones per file.

And registering types is all such a function can safely do since the
call of these constructors is about to be moved (spapr_rtas_register
looks fishy in that aspect).

Andreas

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