Re: [Qemu-devel] [PATCH 09/17] e500: move mpic under CCSR

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 11117 bytes --]

On Sun, Nov 26, 2017 at 03:59:07PM -0600, Michael Davidsaver wrote:
> Start moving code out of ppce500_init()
> 
> Existing ppce500_init_mpic() suggests that MPIC may not be created w/ KVM.
> However, ppce500_init() used mpicdev unconditionally, and would
> fail if the MPIC isn't created.  So require creation.
> 
> Not tested with KVM for lack of hardware.
> 
> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
> ---
>  hw/ppc/e500.c      | 102 +++--------------------------------------------------
>  hw/ppc/e500_ccsr.c |  85 +++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 84 insertions(+), 103 deletions(-)
> 
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index e22919f4f1..1872bb8eaa 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -29,7 +29,6 @@
>  #include "sysemu/kvm.h"
>  #include "kvm_ppc.h"
>  #include "sysemu/device_tree.h"
> -#include "hw/ppc/openpic.h"
>  #include "hw/ppc/ppc.h"
>  #include "hw/loader.h"
>  #include "elf.h"
> @@ -679,92 +678,6 @@ static void ppce500_cpu_reset(void *opaque)
>      mmubooke_create_initial_mapping(env);
>  }
>  
> -static DeviceState *ppce500_init_mpic_qemu(PPCE500Params *params,
> -                                           qemu_irq **irqs)
> -{
> -    DeviceState *dev;
> -    SysBusDevice *s;
> -    int i, j, k;
> -
> -    dev = qdev_create(NULL, TYPE_OPENPIC);
> -    object_property_add_child(qdev_get_machine(), "pic", OBJECT(dev),
> -                              &error_fatal);
> -    qdev_prop_set_uint32(dev, "model", params->mpic_version);
> -    qdev_prop_set_uint32(dev, "nb_cpus", smp_cpus);
> -
> -    qdev_init_nofail(dev);
> -    s = SYS_BUS_DEVICE(dev);
> -
> -    k = 0;
> -    for (i = 0; i < smp_cpus; i++) {
> -        for (j = 0; j < OPENPIC_OUTPUT_NB; j++) {
> -            sysbus_connect_irq(s, k++, irqs[i][j]);
> -        }
> -    }
> -
> -    return dev;
> -}
> -
> -static DeviceState *ppce500_init_mpic_kvm(PPCE500Params *params,
> -                                          qemu_irq **irqs, Error **errp)
> -{
> -    Error *err = NULL;
> -    DeviceState *dev;
> -    CPUState *cs;
> -
> -    dev = qdev_create(NULL, TYPE_KVM_OPENPIC);
> -    qdev_prop_set_uint32(dev, "model", params->mpic_version);
> -
> -    object_property_set_bool(OBJECT(dev), true, "realized", &err);
> -    if (err) {
> -        error_propagate(errp, err);
> -        object_unparent(OBJECT(dev));
> -        return NULL;
> -    }
> -
> -    CPU_FOREACH(cs) {
> -        if (kvm_openpic_connect_vcpu(dev, cs)) {
> -            fprintf(stderr, "%s: failed to connect vcpu to irqchip\n",
> -                    __func__);
> -            abort();
> -        }
> -    }
> -
> -    return dev;
> -}
> -
> -static DeviceState *ppce500_init_mpic(MachineState *machine,
> -                                      PPCE500Params *params,
> -                                      MemoryRegion *ccsr,
> -                                      qemu_irq **irqs)
> -{
> -    DeviceState *dev = NULL;
> -    SysBusDevice *s;
> -
> -    if (kvm_enabled()) {
> -        Error *err = NULL;
> -
> -        if (machine_kernel_irqchip_allowed(machine)) {
> -            dev = ppce500_init_mpic_kvm(params, irqs, &err);
> -        }
> -        if (machine_kernel_irqchip_required(machine) && !dev) {
> -            error_reportf_err(err,
> -                              "kernel_irqchip requested but unavailable: ");
> -            exit(1);
> -        }
> -    }
> -
> -    if (!dev) {
> -        dev = ppce500_init_mpic_qemu(params, irqs);
> -    }
> -
> -    s = SYS_BUS_DEVICE(dev);
> -    memory_region_add_subregion(ccsr, MPC8544_MPIC_REGS_OFFSET,
> -                                s->mmio[0].memory);
> -
> -    return dev;
> -}
> -
>  static void ppce500_power_off(void *opaque, int line, int on)
>  {
>      if (on) {
> @@ -794,18 +707,14 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
>      /* irq num for pin INTA, INTB, INTC and INTD is 1, 2, 3 and
>       * 4 respectively */
>      unsigned int pci_irq_nrs[PCI_NUM_PINS] = {1, 2, 3, 4};
> -    qemu_irq **irqs;
>      DeviceState *dev, *mpicdev;
>      CPUPPCState *firstenv = NULL;
>      MemoryRegion *ccsr_addr_space;
>      SysBusDevice *s;
>  
> -    irqs = g_malloc0(smp_cpus * sizeof(qemu_irq *));
> -    irqs[0] = g_malloc0(smp_cpus * sizeof(qemu_irq) * OPENPIC_OUTPUT_NB);
>      for (i = 0; i < smp_cpus; i++) {
>          PowerPCCPU *cpu;
>          CPUState *cs;
> -        qemu_irq *input;
>  
>          cpu = POWERPC_CPU(cpu_create(machine->cpu_type));
>          env = &cpu->env;
> @@ -821,13 +730,7 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
>              firstenv = env;
>          }
>  
> -        irqs[i] = irqs[0] + (i * OPENPIC_OUTPUT_NB);
> -        input = (qemu_irq *)env->irq_inputs;
> -        irqs[i][OPENPIC_OUTPUT_INT] = input[PPCE500_INPUT_INT];
> -        irqs[i][OPENPIC_OUTPUT_CINT] = input[PPCE500_INPUT_CINT];
>          env->spr_cb[SPR_BOOKE_PIR].default_value = cs->cpu_index = i;
> -        env->mpic_iack = params->ccsrbar_base +
> -                         MPC8544_MPIC_REGS_OFFSET + 0xa0;
>  
>          ppc_booke_timers_init(cpu, 400000000, PPC_TIMER_E500);
>  
> @@ -857,12 +760,15 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
>      dev = qdev_create(NULL, "e500-ccsr");
>      object_property_add_child(qdev_get_machine(), "e500-ccsr",
>                                OBJECT(dev), NULL);
> +    qdev_prop_set_uint32(dev, "mpic-model", params->mpic_version);
>      qdev_prop_set_uint32(dev, "base", params->ccsrbar_base);
>      qdev_prop_set_uint32(dev, "ram-size", ram_size);
>      qdev_init_nofail(dev);
>      ccsr_addr_space = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0);
>  
> -    mpicdev = ppce500_init_mpic(machine, params, ccsr_addr_space, irqs);
> +    /* created under "e500-ccsr" */
> +    mpicdev = DEVICE(object_resolve_path("/machine/pic", 0));
> +    assert(mpicdev);
>  
>      /* Serial */
>      if (serial_hds[0]) {
> diff --git a/hw/ppc/e500_ccsr.c b/hw/ppc/e500_ccsr.c
> index 9400d7cf13..68d952794e 100644
> --- a/hw/ppc/e500_ccsr.c
> +++ b/hw/ppc/e500_ccsr.c
> @@ -24,10 +24,14 @@
>  #include "qemu-common.h"
>  #include "qemu/log.h"
>  #include "qemu/error-report.h"
> +#include "qapi/error.h"
>  #include "cpu.h"
>  #include "hw/hw.h"
> +#include "hw/boards.h"
>  #include "sysemu/sysemu.h"
> +#include "sysemu/kvm.h"
>  #include "hw/sysbus.h"
> +#include "hw/ppc/openpic.h"
>  
>  /* E500_ denotes registers common to all */
>  /* Some CCSR offsets duplicated in e500.c */
> @@ -185,20 +189,90 @@ static void e500_ccsr_reset(DeviceState *dev)
>      e500_ccsr_post_move(ccsr);
>  }
>  
> -static void e500_ccsr_initfn(Object *obj)
> +static void e500_ccsr_init(Object *obj)
>  {
> -    CCSRState *ccsr = E500_CCSR(obj);
> +    DeviceState *dev = DEVICE(obj);
> +    CCSRState *ccsr = E500_CCSR(dev);
> +
> +    assert(current_machine);
> +    if (kvm_enabled()) {
> +
> +        if (!machine_kernel_irqchip_allowed(current_machine)) {
> +            error_report("Machine does not allow PIC,"
> +                         " but this is not supported");
> +            exit(1);

You've changed the logic here from the original; this now always
requires a kernel irqchip with kvm, whereas previously it would allow
fallback to the non kernel irqchip unless
machine_kernel_irqchip_required().

> +        }
>  
> -    memory_region_init_io(&ccsr->iomem, obj, &e500_ccsr_ops,
> +        ccsr->pic = qdev_create(NULL, TYPE_KVM_OPENPIC);
> +    } else {
> +        ccsr->pic = qdev_create(NULL, TYPE_OPENPIC);
> +    }
> +
> +    if (!ccsr->pic) {
> +        error_report("Failed to create PIC");
> +        exit(1);
> +    }
> +
> +    object_property_add_child(qdev_get_machine(), "pic", OBJECT(ccsr->pic),
> +                              &error_fatal);
> +
> +    qdev_prop_set_uint32(ccsr->pic, "nb_cpus", smp_cpus);
> +
> +    object_property_add_alias(obj, "mpic-model",
> +                              OBJECT(ccsr->pic), "model",
> +                              &error_fatal);
> +}
> +
> +static void e500_ccsr_realize(DeviceState *dev, Error **errp)
> +{
> +    CCSRState *ccsr = E500_CCSR(dev);
> +    SysBusDevice *pic;
> +
> +    /* Base 1MB CCSR Region */
> +    memory_region_init_io(&ccsr->iomem, OBJECT(dev), &e500_ccsr_ops,
>                            ccsr, "e500-ccsr", 1024 * 1024);
> -    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &ccsr->iomem);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &ccsr->iomem);
> +
> +    qdev_init_nofail(ccsr->pic);
> +    pic = SYS_BUS_DEVICE(ccsr->pic);
> +
> +    /* connect MPIC to CPU(s) */
> +    if (kvm_enabled()) {
> +        CPUState *cs;
> +
> +        CPU_FOREACH(cs) {
> +            if (kvm_openpic_connect_vcpu(ccsr->pic, cs)) {
> +                error_setg(errp, "%s: failed to connect vcpu to irqchip",
> +                           __func__);
> +                return;
> +            }
> +        }
> +
> +    } else {
> +        CPUState *cs;
> +
> +        CPU_FOREACH(cs) {
> +            PowerPCCPU *cpu = POWERPC_CPU(cs);
> +            CPUPPCState *env = &cpu->env;
> +            qemu_irq *inputs = (qemu_irq *)env->irq_inputs;
> +            int base = cs->cpu_index * PPCE500_INPUT_NB;
> +
> +            sysbus_connect_irq(pic, base + OPENPIC_OUTPUT_INT,
> +                               inputs[PPCE500_INPUT_INT]);
> +            sysbus_connect_irq(pic, base + OPENPIC_OUTPUT_CINT,
> +                               inputs[PPCE500_INPUT_CINT]);
> +        }
> +    }
>  
> +    memory_region_add_subregion(&ccsr->iomem, E500_MPIC_OFFSET,
> +                                sysbus_mmio_get_region(pic, 0));
>  }
>  
>  static Property e500_ccsr_props[] = {
>      DEFINE_PROP_UINT32("base", CCSRState, defbase, 0xff700000),
>      DEFINE_PROP_UINT32("ram-size", CCSRState, ram_size, 0),
>      DEFINE_PROP_UINT32("porpllsr", CCSRState, porpllsr, 0),
> +    /* "mpic-model" aliased from MPIC */
>      DEFINE_PROP_END_OF_LIST()
>  };
>  
> @@ -221,6 +295,7 @@ void e500_ccsr_class_initfn(ObjectClass *klass, void *data)
>  
>      dc->props = e500_ccsr_props;
>      dc->vmsd = &vmstate_e500_ccsr;
> +    dc->realize = e500_ccsr_realize;
>      dc->reset = e500_ccsr_reset;
>  }
>  
> @@ -228,7 +303,7 @@ static const TypeInfo e500_ccsr_info = {
>      .name          = TYPE_E500_CCSR,
>      .parent        = TYPE_SYS_BUS_DEVICE,
>      .instance_size = sizeof(CCSRState),
> -    .instance_init = e500_ccsr_initfn,
> +    .instance_init = e500_ccsr_init,
>      .class_size    = sizeof(SysBusDeviceClass),
>      .class_init    = e500_ccsr_class_initfn
>  };

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]