All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v2] kvm/openpic: in-kernel mpic support
@ 2013-06-12 20:32 Scott Wood
  2013-06-12 20:56 ` Alexander Graf
                   ` (4 more replies)
  0 siblings, 5 replies; 15+ messages in thread
From: Scott Wood @ 2013-06-12 20:32 UTC (permalink / raw)
  To: Alexander Graf; +Cc: Scott Wood, qemu-ppc, qemu-devel

Enables support for the in-kernel MPIC that thas been merged into the
KVM next branch.  This includes irqfd/KVM_IRQ_LINE support from Alex
Graf (along with some other improvements).

Note from Alex regarding kvm_irqchip_create():

  On x86, one would call kvm_irqchip_create() to initialize an
  in-kernel interrupt controller.  That function then goes ahead and
  initializes global capability variables as well as the default irq
  routing table.

  On ppc, we can't call kvm_irqchip_create() because we can have
  different types of interrupt controllers.  So we want to do all the
  things that function would do for us in the in-kernel device init
  handler.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
v2: fix "llx" -> PRI_x64, and remove some broken leftover code
involving reg_base.
---
 default-configs/ppc-softmmu.mak   |    1 +
 default-configs/ppc64-softmmu.mak |    1 +
 hw/intc/Makefile.objs             |    1 +
 hw/intc/openpic_kvm.c             |  250 +++++++++++++++++++++++++++++++++++++
 hw/ppc/e500.c                     |   79 +++++++++++-
 include/hw/ppc/openpic.h          |    2 +-
 6 files changed, 328 insertions(+), 6 deletions(-)
 create mode 100644 hw/intc/openpic_kvm.c

diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
index cc3587f..63255dc 100644
--- a/default-configs/ppc-softmmu.mak
+++ b/default-configs/ppc-softmmu.mak
@@ -43,5 +43,6 @@ CONFIG_XILINX=y
 CONFIG_XILINX_ETHLITE=y
 CONFIG_OPENPIC=y
 CONFIG_E500=$(CONFIG_FDT)
+CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
 # For PReP
 CONFIG_MC146818RTC=y
diff --git a/default-configs/ppc64-softmmu.mak b/default-configs/ppc64-softmmu.mak
index 884ea8a..e3c0c68 100644
--- a/default-configs/ppc64-softmmu.mak
+++ b/default-configs/ppc64-softmmu.mak
@@ -44,6 +44,7 @@ CONFIG_XILINX_ETHLITE=y
 CONFIG_OPENPIC=y
 CONFIG_PSERIES=$(CONFIG_FDT)
 CONFIG_E500=$(CONFIG_FDT)
+CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
 # For pSeries
 CONFIG_PCI_HOTPLUG=y
 # For PReP
diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
index 718d97a..837ef19 100644
--- a/hw/intc/Makefile.objs
+++ b/hw/intc/Makefile.objs
@@ -20,4 +20,5 @@ obj-$(CONFIG_GRLIB) += grlib_irqmp.o
 obj-$(CONFIG_IOAPIC) += ioapic.o
 obj-$(CONFIG_OMAP) += omap_intc.o
 obj-$(CONFIG_OPENPIC) += openpic.o
+obj-$(CONFIG_OPENPIC_KVM) += openpic_kvm.o
 obj-$(CONFIG_SH4) += sh_intc.o
diff --git a/hw/intc/openpic_kvm.c b/hw/intc/openpic_kvm.c
new file mode 100644
index 0000000..809b34b
--- /dev/null
+++ b/hw/intc/openpic_kvm.c
@@ -0,0 +1,250 @@
+/*
+ * KVM in-kernel OpenPIC
+ *
+ * Copyright 2013 Freescale Semiconductor, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include <sys/ioctl.h>
+#include "exec/address-spaces.h"
+#include "hw/hw.h"
+#include "hw/ppc/openpic.h"
+#include "hw/pci/msi.h"
+#include "hw/sysbus.h"
+#include "sysemu/kvm.h"
+#include "qemu/log.h"
+
+typedef struct KVMOpenPICState {
+    SysBusDevice busdev;
+    MemoryRegion mem;
+    MemoryListener mem_listener;
+    uint32_t fd;
+    uint32_t model;
+} KVMOpenPICState;
+
+static void kvm_openpic_set_irq(void *opaque, int n_IRQ, int level)
+{
+    kvm_set_irq(kvm_state, n_IRQ, level);
+}
+
+static void kvm_openpic_reset(DeviceState *d)
+{
+    qemu_log_mask(LOG_UNIMP, "%s: unimplemented\n", __func__);
+}
+
+static void kvm_openpic_write(void *opaque, hwaddr addr, uint64_t val,
+                              unsigned size)
+{
+    KVMOpenPICState *opp = opaque;
+    struct kvm_device_attr attr;
+    uint32_t val32 = val;
+    int ret;
+
+    attr.group = KVM_DEV_MPIC_GRP_REGISTER;
+    attr.attr = addr;
+    attr.addr = (uint64_t)(unsigned long)&val32;
+
+    ret = ioctl(opp->fd, KVM_SET_DEVICE_ATTR, &attr);
+    if (ret < 0) {
+        qemu_log_mask(LOG_UNIMP, "%s: %s %" PRIx64 "\n", __func__,
+                      strerror(errno), attr.attr);
+    }
+}
+
+static uint64_t kvm_openpic_read(void *opaque, hwaddr addr, unsigned size)
+{
+    KVMOpenPICState *opp = opaque;
+    struct kvm_device_attr attr;
+    uint32_t val = 0xdeadbeef;
+    int ret;
+
+    attr.group = KVM_DEV_MPIC_GRP_REGISTER;
+    attr.attr = addr;
+    attr.addr = (uint64_t)(unsigned long)&val;
+
+    ret = ioctl(opp->fd, KVM_GET_DEVICE_ATTR, &attr);
+    if (ret < 0) {
+        qemu_log_mask(LOG_UNIMP, "%s: %s %" PRIx64 "\n", __func__,
+                      strerror(errno), attr.attr);
+        return 0;
+    }
+
+    return val;
+}
+
+static const MemoryRegionOps kvm_openpic_mem_ops = {
+    .write = kvm_openpic_write,
+    .read  = kvm_openpic_read,
+    .endianness = DEVICE_BIG_ENDIAN,
+    .impl = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
+};
+
+static void kvm_openpic_region_add(MemoryListener *listener,
+                                   MemoryRegionSection *section)
+{
+    KVMOpenPICState *opp = container_of(listener, KVMOpenPICState,
+                                        mem_listener);
+    struct kvm_device_attr attr;
+    uint64_t reg_base;
+    int ret;
+
+    if (section->address_space != &address_space_memory) {
+        abort();
+    }
+
+    reg_base = section->offset_within_address_space;
+
+    attr.group = KVM_DEV_MPIC_GRP_MISC;
+    attr.attr = KVM_DEV_MPIC_BASE_ADDR;
+    attr.addr = (uint64_t)(unsigned long)&reg_base;
+
+    ret = ioctl(opp->fd, KVM_SET_DEVICE_ATTR, &attr);
+    if (ret < 0) {
+        fprintf(stderr, "%s: %s %" PRIx64 "\n", __func__,
+                strerror(errno), reg_base);
+    }
+}
+
+static void kvm_openpic_region_del(MemoryListener *listener,
+                                   MemoryRegionSection *section)
+{
+    KVMOpenPICState *opp = container_of(listener, KVMOpenPICState,
+                                        mem_listener);
+    struct kvm_device_attr attr;
+    uint64_t reg_base = 0;
+    int ret;
+
+    attr.group = KVM_DEV_MPIC_GRP_MISC;
+    attr.attr = KVM_DEV_MPIC_BASE_ADDR;
+    attr.addr = (uint64_t)(unsigned long)&reg_base;
+
+    ret = ioctl(opp->fd, KVM_SET_DEVICE_ATTR, &attr);
+    if (ret < 0) {
+        fprintf(stderr, "%s: %s %" PRIx64 "\n", __func__,
+                strerror(errno), reg_base);
+    }
+}
+
+static int kvm_openpic_init(SysBusDevice *dev)
+{
+    KVMState *s = kvm_state;
+    KVMOpenPICState *opp = FROM_SYSBUS(typeof(*opp), dev);
+    int kvm_openpic_model;
+    struct kvm_create_device cd = {0};
+    int ret, i;
+
+    if (!kvm_check_extension(s, KVM_CAP_DEVICE_CTRL)) {
+        return -EINVAL;
+    }
+
+    switch (opp->model) {
+    case OPENPIC_MODEL_FSL_MPIC_20:
+        kvm_openpic_model = KVM_DEV_TYPE_FSL_MPIC_20;
+        break;
+
+    case OPENPIC_MODEL_FSL_MPIC_42:
+        kvm_openpic_model = KVM_DEV_TYPE_FSL_MPIC_42;
+        break;
+
+    default:
+        return -EINVAL;
+    }
+
+    cd.type = kvm_openpic_model;
+    ret = kvm_vm_ioctl(s, KVM_CREATE_DEVICE, &cd);
+    if (ret < 0) {
+        qemu_log_mask(LOG_UNIMP, "%s: can't create device %d: %s\n",
+                      __func__, cd.type, strerror(errno));
+        return -EINVAL;
+    }
+    opp->fd = cd.fd;
+
+    memory_region_init_io(&opp->mem, &kvm_openpic_mem_ops, opp,
+                          "kvm-openpic", 0x40000);
+
+    sysbus_init_mmio(dev, &opp->mem);
+    qdev_init_gpio_in(&dev->qdev, kvm_openpic_set_irq, OPENPIC_MAX_IRQ);
+
+    opp->mem_listener.region_add = kvm_openpic_region_add;
+    opp->mem_listener.region_add = kvm_openpic_region_del;
+    memory_listener_register(&opp->mem_listener, &address_space_memory);
+
+    /* indicate pic capabilities */
+    msi_supported = true;
+    kvm_kernel_irqchip = true;
+    kvm_async_interrupts_allowed = true;
+
+    /* set up irq routing */
+    kvm_init_irq_routing(kvm_state);
+    for (i = 0; i < 256; ++i) {
+        kvm_irqchip_add_irq_route(kvm_state, i, 0, i);
+    }
+
+    kvm_irqfds_allowed = true;
+    kvm_msi_via_irqfd_allowed = true;
+    kvm_gsi_routing_allowed = true;
+
+    return 0;
+}
+
+int kvm_openpic_connect_vcpu(DeviceState *d, CPUState *cs)
+{
+    KVMOpenPICState *opp = FROM_SYSBUS(typeof(*opp), SYS_BUS_DEVICE(d));
+    struct kvm_enable_cap encap = {};
+
+    encap.cap = KVM_CAP_IRQ_MPIC;
+    encap.args[0] = opp->fd;
+    encap.args[1] = cs->cpu_index;
+
+    return kvm_vcpu_ioctl(cs, KVM_ENABLE_CAP, &encap);
+}
+
+static Property kvm_openpic_properties[] = {
+    DEFINE_PROP_UINT32("model", KVMOpenPICState, model,
+                       OPENPIC_MODEL_FSL_MPIC_20),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void kvm_openpic_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
+
+    k->init = kvm_openpic_init;
+    dc->props = kvm_openpic_properties;
+    dc->reset = kvm_openpic_reset;
+}
+
+static const TypeInfo kvm_openpic_info = {
+    .name          = "kvm-openpic",
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(KVMOpenPICState),
+    .class_init    = kvm_openpic_class_init,
+};
+
+static void kvm_openpic_register_types(void)
+{
+    type_register_static(&kvm_openpic_info);
+}
+
+type_init(kvm_openpic_register_types)
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 14e0547..f790ed1 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -448,18 +448,17 @@ static void ppce500_cpu_reset(void *opaque)
     mmubooke_create_initial_mapping(env);
 }
 
-static qemu_irq *ppce500_init_mpic(PPCE500Params *params, MemoryRegion *ccsr,
-                                   qemu_irq **irqs)
+static DeviceState *ppce500_init_mpic_qemu(PPCE500Params *params,
+                                           qemu_irq **irqs)
 {
-    qemu_irq *mpic;
     DeviceState *dev;
     SysBusDevice *s;
     int i, j, k;
 
-    mpic = g_new(qemu_irq, 256);
     dev = qdev_create(NULL, "openpic");
-    qdev_prop_set_uint32(dev, "nb_cpus", smp_cpus);
     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);
 
@@ -470,10 +469,80 @@ static qemu_irq *ppce500_init_mpic(PPCE500Params *params, MemoryRegion *ccsr,
         }
     }
 
+    return dev;
+}
+
+static DeviceState *ppce500_init_mpic_kvm(PPCE500Params *params,
+                                          qemu_irq **irqs)
+{
+    DeviceState *dev;
+    CPUPPCState *env;
+    CPUState *cs;
+    int r;
+
+    dev = qdev_create(NULL, "kvm-openpic");
+    qdev_prop_set_uint32(dev, "model", params->mpic_version);
+
+    r = qdev_init(dev);
+    if (r) {
+        return NULL;
+    }
+
+    for (env = first_cpu; env != NULL; env = env->next_cpu) {
+        cs = ENV_GET_CPU(env);
+
+        if (kvm_openpic_connect_vcpu(dev, cs)) {
+            fprintf(stderr, "%s: failed to connect vcpu to irqchip\n",
+                    __func__);
+            abort();
+        }
+    }
+
+    return dev;
+}
+
+static qemu_irq *ppce500_init_mpic(PPCE500Params *params, MemoryRegion *ccsr,
+                                   qemu_irq **irqs)
+{
+    QemuOptsList *list;
+    qemu_irq *mpic;
+    DeviceState *dev = NULL;
+    SysBusDevice *s;
+    int i;
+
+    mpic = g_new(qemu_irq, 256);
+
+    if (kvm_enabled()) {
+        bool irqchip_allowed = true, irqchip_required = false;
+
+        list = qemu_find_opts("machine");
+        if (!QTAILQ_EMPTY(&list->head)) {
+            irqchip_allowed = qemu_opt_get_bool(QTAILQ_FIRST(&list->head),
+                                                "kernel_irqchip", true);
+            irqchip_required = qemu_opt_get_bool(QTAILQ_FIRST(&list->head),
+                                                 "kernel_irqchip", false);
+        }
+
+        if (irqchip_allowed) {
+            dev = ppce500_init_mpic_kvm(params, irqs);
+        }
+
+        if (irqchip_required && !dev) {
+            fprintf(stderr, "%s: irqchip requested but unavailable\n",
+                    __func__);
+            abort();
+        }
+    }
+
+    if (!dev) {
+        dev = ppce500_init_mpic_qemu(params, irqs);
+    }
+
     for (i = 0; i < 256; i++) {
         mpic[i] = qdev_get_gpio_in(dev, i);
     }
 
+    s = SYS_BUS_DEVICE(dev);
     memory_region_add_subregion(ccsr, MPC8544_MPIC_REGS_OFFSET,
                                 s->mmio[0].memory);
 
diff --git a/include/hw/ppc/openpic.h b/include/hw/ppc/openpic.h
index d873bb6..1fe4865 100644
--- a/include/hw/ppc/openpic.h
+++ b/include/hw/ppc/openpic.h
@@ -24,6 +24,6 @@ enum {
 #define OPENPIC_MAX_IRQ     (OPENPIC_MAX_SRC + OPENPIC_MAX_IPI + \
                              OPENPIC_MAX_TMR)
 
-DeviceState *kvm_openpic_create(BusState *bus, int model);
+int kvm_openpic_connect_vcpu(DeviceState *d, CPUState *cs);
 
 #endif /* __OPENPIC_H__ */
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v2] kvm/openpic: in-kernel mpic support
  2013-06-12 20:32 [Qemu-devel] [PATCH v2] kvm/openpic: in-kernel mpic support Scott Wood
@ 2013-06-12 20:56 ` Alexander Graf
  2013-06-13 11:01 ` Andreas Färber
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 15+ messages in thread
From: Alexander Graf @ 2013-06-12 20:56 UTC (permalink / raw)
  To: Scott Wood; +Cc: qemu-ppc, qemu-devel


On 12.06.2013, at 22:32, Scott Wood wrote:

> Enables support for the in-kernel MPIC that thas been merged into the
> KVM next branch.  This includes irqfd/KVM_IRQ_LINE support from Alex
> Graf (along with some other improvements).
> 
> Note from Alex regarding kvm_irqchip_create():
> 
>  On x86, one would call kvm_irqchip_create() to initialize an
>  in-kernel interrupt controller.  That function then goes ahead and
>  initializes global capability variables as well as the default irq
>  routing table.
> 
>  On ppc, we can't call kvm_irqchip_create() because we can have
>  different types of interrupt controllers.  So we want to do all the
>  things that function would do for us in the in-kernel device init
>  handler.
> 
> Signed-off-by: Scott Wood <scottwood@freescale.com>

Thanks, applied to ppc-next.


Alex

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v2] kvm/openpic: in-kernel mpic support
  2013-06-12 20:32 [Qemu-devel] [PATCH v2] kvm/openpic: in-kernel mpic support Scott Wood
  2013-06-12 20:56 ` Alexander Graf
@ 2013-06-13 11:01 ` Andreas Färber
  2013-06-13 17:32   ` Scott Wood
  2013-06-16 18:32 ` [Qemu-devel] [PATCH ppc-next fixup] intc/openpic_kvm: Fix QOM and build issues Andreas Färber
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 15+ messages in thread
From: Andreas Färber @ 2013-06-13 11:01 UTC (permalink / raw)
  To: Scott Wood, Alexander Graf; +Cc: qemu-ppc, qemu-devel

Am 12.06.2013 22:32, schrieb Scott Wood:
> Enables support for the in-kernel MPIC that thas been merged into the
> KVM next branch.  This includes irqfd/KVM_IRQ_LINE support from Alex
> Graf (along with some other improvements).
> 
> Note from Alex regarding kvm_irqchip_create():
> 
>   On x86, one would call kvm_irqchip_create() to initialize an
>   in-kernel interrupt controller.  That function then goes ahead and
>   initializes global capability variables as well as the default irq
>   routing table.
> 
>   On ppc, we can't call kvm_irqchip_create() because we can have
>   different types of interrupt controllers.  So we want to do all the
>   things that function would do for us in the in-kernel device init
>   handler.
> 
> Signed-off-by: Scott Wood <scottwood@freescale.com>
> ---
> v2: fix "llx" -> PRI_x64, and remove some broken leftover code
> involving reg_base.
> ---
>  default-configs/ppc-softmmu.mak   |    1 +
>  default-configs/ppc64-softmmu.mak |    1 +
>  hw/intc/Makefile.objs             |    1 +
>  hw/intc/openpic_kvm.c             |  250 +++++++++++++++++++++++++++++++++++++
>  hw/ppc/e500.c                     |   79 +++++++++++-
>  include/hw/ppc/openpic.h          |    2 +-
>  6 files changed, 328 insertions(+), 6 deletions(-)
>  create mode 100644 hw/intc/openpic_kvm.c
> 
> diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
> index cc3587f..63255dc 100644
> --- a/default-configs/ppc-softmmu.mak
> +++ b/default-configs/ppc-softmmu.mak
> @@ -43,5 +43,6 @@ CONFIG_XILINX=y
>  CONFIG_XILINX_ETHLITE=y
>  CONFIG_OPENPIC=y
>  CONFIG_E500=$(CONFIG_FDT)
> +CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
>  # For PReP
>  CONFIG_MC146818RTC=y
> diff --git a/default-configs/ppc64-softmmu.mak b/default-configs/ppc64-softmmu.mak
> index 884ea8a..e3c0c68 100644
> --- a/default-configs/ppc64-softmmu.mak
> +++ b/default-configs/ppc64-softmmu.mak
> @@ -44,6 +44,7 @@ CONFIG_XILINX_ETHLITE=y
>  CONFIG_OPENPIC=y
>  CONFIG_PSERIES=$(CONFIG_FDT)
>  CONFIG_E500=$(CONFIG_FDT)
> +CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
>  # For pSeries
>  CONFIG_PCI_HOTPLUG=y
>  # For PReP
> diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
> index 718d97a..837ef19 100644
> --- a/hw/intc/Makefile.objs
> +++ b/hw/intc/Makefile.objs
> @@ -20,4 +20,5 @@ obj-$(CONFIG_GRLIB) += grlib_irqmp.o
>  obj-$(CONFIG_IOAPIC) += ioapic.o
>  obj-$(CONFIG_OMAP) += omap_intc.o
>  obj-$(CONFIG_OPENPIC) += openpic.o
> +obj-$(CONFIG_OPENPIC_KVM) += openpic_kvm.o
>  obj-$(CONFIG_SH4) += sh_intc.o
> diff --git a/hw/intc/openpic_kvm.c b/hw/intc/openpic_kvm.c
> new file mode 100644
> index 0000000..809b34b
> --- /dev/null
> +++ b/hw/intc/openpic_kvm.c
> @@ -0,0 +1,250 @@
> +/*
> + * KVM in-kernel OpenPIC
> + *
> + * Copyright 2013 Freescale Semiconductor, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include <sys/ioctl.h>
> +#include "exec/address-spaces.h"
> +#include "hw/hw.h"
> +#include "hw/ppc/openpic.h"
> +#include "hw/pci/msi.h"
> +#include "hw/sysbus.h"
> +#include "sysemu/kvm.h"
> +#include "qemu/log.h"
> +
> +typedef struct KVMOpenPICState {
> +    SysBusDevice busdev;

SysBusDevice parent_obj; please!

http://wiki.qemu.org/QOMConventions

> +    MemoryRegion mem;
> +    MemoryListener mem_listener;
> +    uint32_t fd;
> +    uint32_t model;
> +} KVMOpenPICState;
> +
> +static void kvm_openpic_set_irq(void *opaque, int n_IRQ, int level)
> +{
> +    kvm_set_irq(kvm_state, n_IRQ, level);
> +}
> +
> +static void kvm_openpic_reset(DeviceState *d)
> +{
> +    qemu_log_mask(LOG_UNIMP, "%s: unimplemented\n", __func__);
> +}
> +
> +static void kvm_openpic_write(void *opaque, hwaddr addr, uint64_t val,
> +                              unsigned size)
> +{
> +    KVMOpenPICState *opp = opaque;
> +    struct kvm_device_attr attr;
> +    uint32_t val32 = val;
> +    int ret;
> +
> +    attr.group = KVM_DEV_MPIC_GRP_REGISTER;
> +    attr.attr = addr;
> +    attr.addr = (uint64_t)(unsigned long)&val32;
> +
> +    ret = ioctl(opp->fd, KVM_SET_DEVICE_ATTR, &attr);
> +    if (ret < 0) {
> +        qemu_log_mask(LOG_UNIMP, "%s: %s %" PRIx64 "\n", __func__,
> +                      strerror(errno), attr.attr);
> +    }
> +}
> +
> +static uint64_t kvm_openpic_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    KVMOpenPICState *opp = opaque;
> +    struct kvm_device_attr attr;
> +    uint32_t val = 0xdeadbeef;
> +    int ret;
> +
> +    attr.group = KVM_DEV_MPIC_GRP_REGISTER;
> +    attr.attr = addr;
> +    attr.addr = (uint64_t)(unsigned long)&val;
> +
> +    ret = ioctl(opp->fd, KVM_GET_DEVICE_ATTR, &attr);
> +    if (ret < 0) {
> +        qemu_log_mask(LOG_UNIMP, "%s: %s %" PRIx64 "\n", __func__,
> +                      strerror(errno), attr.attr);
> +        return 0;
> +    }
> +
> +    return val;
> +}
> +
> +static const MemoryRegionOps kvm_openpic_mem_ops = {
> +    .write = kvm_openpic_write,
> +    .read  = kvm_openpic_read,
> +    .endianness = DEVICE_BIG_ENDIAN,
> +    .impl = {
> +        .min_access_size = 4,
> +        .max_access_size = 4,
> +    },
> +};
> +
> +static void kvm_openpic_region_add(MemoryListener *listener,
> +                                   MemoryRegionSection *section)
> +{
> +    KVMOpenPICState *opp = container_of(listener, KVMOpenPICState,
> +                                        mem_listener);
> +    struct kvm_device_attr attr;
> +    uint64_t reg_base;
> +    int ret;
> +
> +    if (section->address_space != &address_space_memory) {
> +        abort();
> +    }
> +
> +    reg_base = section->offset_within_address_space;
> +
> +    attr.group = KVM_DEV_MPIC_GRP_MISC;
> +    attr.attr = KVM_DEV_MPIC_BASE_ADDR;
> +    attr.addr = (uint64_t)(unsigned long)&reg_base;
> +
> +    ret = ioctl(opp->fd, KVM_SET_DEVICE_ATTR, &attr);
> +    if (ret < 0) {
> +        fprintf(stderr, "%s: %s %" PRIx64 "\n", __func__,
> +                strerror(errno), reg_base);
> +    }
> +}
> +
> +static void kvm_openpic_region_del(MemoryListener *listener,
> +                                   MemoryRegionSection *section)
> +{
> +    KVMOpenPICState *opp = container_of(listener, KVMOpenPICState,
> +                                        mem_listener);
> +    struct kvm_device_attr attr;
> +    uint64_t reg_base = 0;
> +    int ret;
> +
> +    attr.group = KVM_DEV_MPIC_GRP_MISC;
> +    attr.attr = KVM_DEV_MPIC_BASE_ADDR;
> +    attr.addr = (uint64_t)(unsigned long)&reg_base;
> +
> +    ret = ioctl(opp->fd, KVM_SET_DEVICE_ATTR, &attr);
> +    if (ret < 0) {
> +        fprintf(stderr, "%s: %s %" PRIx64 "\n", __func__,
> +                strerror(errno), reg_base);
> +    }
> +}
> +
> +static int kvm_openpic_init(SysBusDevice *dev)

Please make this instance_init + realize functions - "dev" should rather
be reserved for DeviceState.

> +{
> +    KVMState *s = kvm_state;
> +    KVMOpenPICState *opp = FROM_SYSBUS(typeof(*opp), dev);

NACK, please introduce your own KVM_OPENPIC(obj) cast macro instead for
new devices - has been a topic for several weeks and months now.

> +    int kvm_openpic_model;
> +    struct kvm_create_device cd = {0};
> +    int ret, i;
> +
> +    if (!kvm_check_extension(s, KVM_CAP_DEVICE_CTRL)) {
> +        return -EINVAL;
> +    }
> +
> +    switch (opp->model) {
> +    case OPENPIC_MODEL_FSL_MPIC_20:
> +        kvm_openpic_model = KVM_DEV_TYPE_FSL_MPIC_20;
> +        break;
> +
> +    case OPENPIC_MODEL_FSL_MPIC_42:
> +        kvm_openpic_model = KVM_DEV_TYPE_FSL_MPIC_42;
> +        break;
> +
> +    default:
> +        return -EINVAL;
> +    }

If there's only two supported enum-style options, why not make it two
devices with the value set as a class field?

> +
> +    cd.type = kvm_openpic_model;
> +    ret = kvm_vm_ioctl(s, KVM_CREATE_DEVICE, &cd);
> +    if (ret < 0) {
> +        qemu_log_mask(LOG_UNIMP, "%s: can't create device %d: %s\n",
> +                      __func__, cd.type, strerror(errno));
> +        return -EINVAL;
> +    }
> +    opp->fd = cd.fd;
> +
> +    memory_region_init_io(&opp->mem, &kvm_openpic_mem_ops, opp,
> +                          "kvm-openpic", 0x40000);
> +
> +    sysbus_init_mmio(dev, &opp->mem);
> +    qdev_init_gpio_in(&dev->qdev, kvm_openpic_set_irq, OPENPIC_MAX_IRQ);
> +
> +    opp->mem_listener.region_add = kvm_openpic_region_add;
> +    opp->mem_listener.region_add = kvm_openpic_region_del;
> +    memory_listener_register(&opp->mem_listener, &address_space_memory);
> +
> +    /* indicate pic capabilities */
> +    msi_supported = true;
> +    kvm_kernel_irqchip = true;
> +    kvm_async_interrupts_allowed = true;
> +
> +    /* set up irq routing */
> +    kvm_init_irq_routing(kvm_state);
> +    for (i = 0; i < 256; ++i) {
> +        kvm_irqchip_add_irq_route(kvm_state, i, 0, i);
> +    }
> +
> +    kvm_irqfds_allowed = true;
> +    kvm_msi_via_irqfd_allowed = true;
> +    kvm_gsi_routing_allowed = true;
> +
> +    return 0;
> +}
> +
> +int kvm_openpic_connect_vcpu(DeviceState *d, CPUState *cs)
> +{
> +    KVMOpenPICState *opp = FROM_SYSBUS(typeof(*opp), SYS_BUS_DEVICE(d));

KVM_OPENPIC(d)

> +    struct kvm_enable_cap encap = {};
> +
> +    encap.cap = KVM_CAP_IRQ_MPIC;
> +    encap.args[0] = opp->fd;
> +    encap.args[1] = cs->cpu_index;
> +
> +    return kvm_vcpu_ioctl(cs, KVM_ENABLE_CAP, &encap);
> +}
> +
> +static Property kvm_openpic_properties[] = {
> +    DEFINE_PROP_UINT32("model", KVMOpenPICState, model,
> +                       OPENPIC_MODEL_FSL_MPIC_20),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void kvm_openpic_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +
> +    k->init = kvm_openpic_init;
> +    dc->props = kvm_openpic_properties;
> +    dc->reset = kvm_openpic_reset;
> +}
> +
> +static const TypeInfo kvm_openpic_info = {
> +    .name          = "kvm-openpic",
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(KVMOpenPICState),
> +    .class_init    = kvm_openpic_class_init,
> +};
> +
> +static void kvm_openpic_register_types(void)
> +{
> +    type_register_static(&kvm_openpic_info);
> +}
> +
> +type_init(kvm_openpic_register_types)
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 14e0547..f790ed1 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -448,18 +448,17 @@ static void ppce500_cpu_reset(void *opaque)
>      mmubooke_create_initial_mapping(env);
>  }
>  
> -static qemu_irq *ppce500_init_mpic(PPCE500Params *params, MemoryRegion *ccsr,
> -                                   qemu_irq **irqs)
> +static DeviceState *ppce500_init_mpic_qemu(PPCE500Params *params,
> +                                           qemu_irq **irqs)
>  {
> -    qemu_irq *mpic;
>      DeviceState *dev;
>      SysBusDevice *s;
>      int i, j, k;
>  
> -    mpic = g_new(qemu_irq, 256);
>      dev = qdev_create(NULL, "openpic");
> -    qdev_prop_set_uint32(dev, "nb_cpus", smp_cpus);
>      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);
>  
> @@ -470,10 +469,80 @@ static qemu_irq *ppce500_init_mpic(PPCE500Params *params, MemoryRegion *ccsr,
>          }
>      }
>  
> +    return dev;
> +}
> +
> +static DeviceState *ppce500_init_mpic_kvm(PPCE500Params *params,
> +                                          qemu_irq **irqs)
> +{
> +    DeviceState *dev;
> +    CPUPPCState *env;
> +    CPUState *cs;
> +    int r;
> +
> +    dev = qdev_create(NULL, "kvm-openpic");
> +    qdev_prop_set_uint32(dev, "model", params->mpic_version);
> +
> +    r = qdev_init(dev);
> +    if (r) {
> +        return NULL;
> +    }
> +
> +    for (env = first_cpu; env != NULL; env = env->next_cpu) {
> +        cs = ENV_GET_CPU(env);
> +
> +        if (kvm_openpic_connect_vcpu(dev, cs)) {
> +            fprintf(stderr, "%s: failed to connect vcpu to irqchip\n",
> +                    __func__);
> +            abort();
> +        }
> +    }

Note: My series on the list converts first_cpu to CPUState, so this may
conflict depending on pull timing. Resolution would be s/env/cs/g.

Andreas

> +
> +    return dev;
> +}
> +
> +static qemu_irq *ppce500_init_mpic(PPCE500Params *params, MemoryRegion *ccsr,
> +                                   qemu_irq **irqs)
> +{
> +    QemuOptsList *list;
> +    qemu_irq *mpic;
> +    DeviceState *dev = NULL;
> +    SysBusDevice *s;
> +    int i;
> +
> +    mpic = g_new(qemu_irq, 256);
> +
> +    if (kvm_enabled()) {
> +        bool irqchip_allowed = true, irqchip_required = false;
> +
> +        list = qemu_find_opts("machine");
> +        if (!QTAILQ_EMPTY(&list->head)) {
> +            irqchip_allowed = qemu_opt_get_bool(QTAILQ_FIRST(&list->head),
> +                                                "kernel_irqchip", true);
> +            irqchip_required = qemu_opt_get_bool(QTAILQ_FIRST(&list->head),
> +                                                 "kernel_irqchip", false);
> +        }
> +
> +        if (irqchip_allowed) {
> +            dev = ppce500_init_mpic_kvm(params, irqs);
> +        }
> +
> +        if (irqchip_required && !dev) {
> +            fprintf(stderr, "%s: irqchip requested but unavailable\n",
> +                    __func__);
> +            abort();
> +        }
> +    }
> +
> +    if (!dev) {
> +        dev = ppce500_init_mpic_qemu(params, irqs);
> +    }
> +
>      for (i = 0; i < 256; i++) {
>          mpic[i] = qdev_get_gpio_in(dev, i);
>      }
>  
> +    s = SYS_BUS_DEVICE(dev);
>      memory_region_add_subregion(ccsr, MPC8544_MPIC_REGS_OFFSET,
>                                  s->mmio[0].memory);
>  
> diff --git a/include/hw/ppc/openpic.h b/include/hw/ppc/openpic.h
> index d873bb6..1fe4865 100644
> --- a/include/hw/ppc/openpic.h
> +++ b/include/hw/ppc/openpic.h
> @@ -24,6 +24,6 @@ enum {
>  #define OPENPIC_MAX_IRQ     (OPENPIC_MAX_SRC + OPENPIC_MAX_IPI + \
>                               OPENPIC_MAX_TMR)
>  
> -DeviceState *kvm_openpic_create(BusState *bus, int model);
> +int kvm_openpic_connect_vcpu(DeviceState *d, CPUState *cs);
>  
>  #endif /* __OPENPIC_H__ */
> 


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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v2] kvm/openpic: in-kernel mpic support
  2013-06-13 11:01 ` Andreas Färber
@ 2013-06-13 17:32   ` Scott Wood
  2013-06-14 14:59     ` Andreas Färber
  0 siblings, 1 reply; 15+ messages in thread
From: Scott Wood @ 2013-06-13 17:32 UTC (permalink / raw)
  To: Andreas Färber; +Cc: qemu-ppc, Alexander Graf, qemu-devel

On 06/13/2013 06:01:49 AM, Andreas Färber wrote:
> Am 12.06.2013 22:32, schrieb Scott Wood:
> > +typedef struct KVMOpenPICState {
> > +    SysBusDevice busdev;
> 
> SysBusDevice parent_obj; please!
> 
> http://wiki.qemu.org/QOMConventions

The word "QOMConventions" doesn't exist once in the QEMU source tree.   
How is one supposed to know that this documentation exists? :-P

Plus, this is just copied from the non-KVM MPIC file, and I see many  
other instances throughout the source tree.

> > +static int kvm_openpic_init(SysBusDevice *dev)
> 
> Please make this instance_init + realize functions - "dev" should  
> rather
> be reserved for DeviceState.

Could you elaborate?  I'm really not familiar with this stuff, and have  
not found much documentation.  Again, this is patterned after the  
existing
non-KVM openpic file.

> > +{
> > +    KVMState *s = kvm_state;
> > +    KVMOpenPICState *opp = FROM_SYSBUS(typeof(*opp), dev);
> 
> NACK, please introduce your own KVM_OPENPIC(obj) cast macro instead  
> for
> new devices - has been a topic for several weeks and months now.

There's way too much traffic on the list for me to know about something  
just because it's "been a topic".  Lately it's been too much for me to  
even scan the subject lines looking for things that look important,  
given that QEMU is only a fraction of what I spend my time on.

If you'd like to update hw/intc/openpic.c to comply with the style of  
the day, then this could be updated to match...

Also note that this is hardly the first time this patch has been posted  
(v1 was a few weeks ago, and there were RFC patches well before that).   
The first version may have even preceded this "topic".  This seems a  
bit late in the process for a bunch of style churn, when existing code  
hasn't been updated.

> > +    int kvm_openpic_model;
> > +    struct kvm_create_device cd = {0};
> > +    int ret, i;
> > +
> > +    if (!kvm_check_extension(s, KVM_CAP_DEVICE_CTRL)) {
> > +        return -EINVAL;
> > +    }
> > +
> > +    switch (opp->model) {
> > +    case OPENPIC_MODEL_FSL_MPIC_20:
> > +        kvm_openpic_model = KVM_DEV_TYPE_FSL_MPIC_20;
> > +        break;
> > +
> > +    case OPENPIC_MODEL_FSL_MPIC_42:
> > +        kvm_openpic_model = KVM_DEV_TYPE_FSL_MPIC_42;
> > +        break;
> > +
> > +    default:
> > +        return -EINVAL;
> > +    }
> 
> If there's only two supported enum-style options, why not make it two
> devices with the value set as a class field?

I'm not 100% sure what you mean here, but it is not intended that there  
will always be only two supported options.  At the least we will  
probably support v4.3 at some point.

-Scott

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v2] kvm/openpic: in-kernel mpic support
  2013-06-13 17:32   ` Scott Wood
@ 2013-06-14 14:59     ` Andreas Färber
  2013-06-14 22:57       ` Scott Wood
  0 siblings, 1 reply; 15+ messages in thread
From: Andreas Färber @ 2013-06-14 14:59 UTC (permalink / raw)
  To: Scott Wood; +Cc: qemu-ppc, Alexander Graf, qemu-devel

Am 13.06.2013 19:32, schrieb Scott Wood:
> On 06/13/2013 06:01:49 AM, Andreas Färber wrote:
>> Am 12.06.2013 22:32, schrieb Scott Wood:
>> > +typedef struct KVMOpenPICState {
>> > +    SysBusDevice busdev;
>>
>> SysBusDevice parent_obj; please!
>>
>> http://wiki.qemu.org/QOMConventions
> 
> The word "QOMConventions" doesn't exist once in the QEMU source tree. 
> How is one supposed to know that this documentation exists? :-P

I do expect people to read at least the subject of patch series on
qemu-devel, short of individual review comments. CPU, PReP PCI,
Versatile PCI, ISA and more recently virtio series had been posted.
Some such review comments have been collected into the above Wiki page
because this is a recurring topic unfortunately.

> Plus, this is just copied from the non-KVM MPIC file, and I see many
> other instances throughout the source tree.

Which exactly is the reason for my grief: Your ignorance is making other
people even more work, and at least Alex should've spotted it - I can't
review all patches just because they might or might not be touching on QOM.
Just as we are supposed to not copy old Coding Style in new patches, we
should be applying new patterns and conventions in new patches, too.

>> > +static int kvm_openpic_init(SysBusDevice *dev)
>>
>> Please make this instance_init + realize functions - "dev" should rather
>> be reserved for DeviceState.
> 
> Could you elaborate?  I'm really not familiar with this stuff, and have
> not found much documentation.  Again, this is patterned after the existing
> non-KVM openpic file.

static void kvm_openpic_init(Object *obj) should initialize simple
variables, MemoryRegions that don't depend on parameters and any QOM
properties.

static void kvm_openpic_realize(DeviceState *dev, Error **errp) should
initialize the rest.

kvm_openpic_unrealize(Device *dev, Error **errp) and
kvm_openpic_finalize(Object *obj) would be their counterparts for cleanup.

>> > +{
>> > +    KVMState *s = kvm_state;
>> > +    KVMOpenPICState *opp = FROM_SYSBUS(typeof(*opp), dev);
>>
>> NACK, please introduce your own KVM_OPENPIC(obj) cast macro instead for
>> new devices - has been a topic for several weeks and months now.
> 
> There's way too much traffic on the list for me to know about something
> just because it's "been a topic".  Lately it's been too much for me to
> even scan the subject lines looking for things that look important,
> given that QEMU is only a fraction of what I spend my time on.
> 
> If you'd like to update hw/intc/openpic.c to comply with the style of
> the day, then this could be updated to match...
> 
> Also note that this is hardly the first time this patch has been posted
> (v1 was a few weeks ago, and there were RFC patches well before that). 
> The first version may have even preceded this "topic".  This seems a bit
> late in the process for a bunch of style churn, when existing code
> hasn't been updated.

I'm not talking about style churn, I'm talking about using the wrong
infrastructure and making it even more difficult to drop FROM_SYSBUS()
macro.

Again, whether or not some particular other file uses some style doesn't
mean that it's okay to apply that to new files. Instead of complaining
it would've been a task of five minutes to supply Alex with a fixup
patch to squash/follow-up or to post a v3. QOM realize is merged since
January 2013 and was presented by Anthony in January 2012.

>> > +    int kvm_openpic_model;
>> > +    struct kvm_create_device cd = {0};
>> > +    int ret, i;
>> > +
>> > +    if (!kvm_check_extension(s, KVM_CAP_DEVICE_CTRL)) {
>> > +        return -EINVAL;
>> > +    }
>> > +
>> > +    switch (opp->model) {
>> > +    case OPENPIC_MODEL_FSL_MPIC_20:
>> > +        kvm_openpic_model = KVM_DEV_TYPE_FSL_MPIC_20;
>> > +        break;
>> > +
>> > +    case OPENPIC_MODEL_FSL_MPIC_42:
>> > +        kvm_openpic_model = KVM_DEV_TYPE_FSL_MPIC_42;
>> > +        break;
>> > +
>> > +    default:
>> > +        return -EINVAL;
>> > +    }
>>
>> If there's only two supported enum-style options, why not make it two
>> devices with the value set as a class field?
> 
> I'm not 100% sure what you mean here, but it is not intended that there
> will always be only two supported options.  At the least we will
> probably support v4.3 at some point.

Afterwards I saw that you copied the "model" property from the non-KVM.
What I was pointing about here is that unlike qdev QOM allows to have
arbitrary levels of inheritence, i.e. you can have a kvm-openpic base
class and specialized subclasses like mipc42-kvm-openpic rather than
kvm-openpic,model=42 given that the user can't specify arbitrary
integers - whether two or three doesn't really matter in that respect.

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v2] kvm/openpic: in-kernel mpic support
  2013-06-14 14:59     ` Andreas Färber
@ 2013-06-14 22:57       ` Scott Wood
  2013-06-16 19:11         ` Andreas Färber
  0 siblings, 1 reply; 15+ messages in thread
From: Scott Wood @ 2013-06-14 22:57 UTC (permalink / raw)
  To: Andreas Färber; +Cc: qemu-ppc, Alexander Graf, qemu-devel

On 06/14/2013 09:59:18 AM, Andreas Färber wrote:
> Am 13.06.2013 19:32, schrieb Scott Wood:
> > On 06/13/2013 06:01:49 AM, Andreas Färber wrote:
> >> Am 12.06.2013 22:32, schrieb Scott Wood:
> >> > +typedef struct KVMOpenPICState {
> >> > +    SysBusDevice busdev;
> >>
> >> SysBusDevice parent_obj; please!
> >>
> >> http://wiki.qemu.org/QOMConventions
> >
> > The word "QOMConventions" doesn't exist once in the QEMU source  
> tree.
> > How is one supposed to know that this documentation exists? :-P
> 
> I do expect people to read at least the subject of patch series on
> qemu-devel,

I have over 12000 currently unread e-mails from that list.  They are  
not separated into "patch" and "other".  Even among the patches, they  
get posted at least twice due to the (unique to QEMU and KVM as far as  
I can tell) practice of reposting everything before a pull request.

There's just no way I can keep up with all of this, *plus* all the  
non-QEMU stuff I need to keep up with.  Sorry.  I generally rely on  
Alex to guide me on things like qdev/QOM.  Quite frankly, I didn't even  
realize I was using QOM.  I thought this was still qdev.  I even create  
it using qdev_create()...

> short of individual review comments. CPU, PReP PCI,
> Versatile PCI, ISA and more recently virtio series had been posted.

...which of those would make me think "hmm, there's something in here  
that I need to read before submitting patches for in-kernel mpic"?

I'm not trying to be difficult -- I'm just trying to point out that  
there's room for improvement in how the QEMU community communicates to  
developers what is expected.  Maybe an announcement list that just  
contains important updates and summaries?  Also, even starting on  
http://wiki.qemu.org/ I don't see any obvious path to get to  
QOMConventions.  It's not even linked to from the main QOM page.

I do understand the frustration of having to correct people on the same  
things over and over -- I experience it myself in other contexts.  But  
there are more constructive ways to deal with it than exclamation  
points.

> > Plus, this is just copied from the non-KVM MPIC file, and I see many
> > other instances throughout the source tree.
> 
> Which exactly is the reason for my grief: Your ignorance is making  
> other
> people even more work, and at least Alex should've spotted it - I  
> can't
> review all patches just because they might or might not be touching  
> on QOM.
> Just as we are supposed to not copy old Coding Style in new patches,  
> we
> should be applying new patterns and conventions in new patches, too.

I'm usually the first person to complain about bad copy and paste, but  
this is a situation where the KVM version of openpic is supposed to  
interface with the rest of the system in exactly the same way as the  
regular openpic.  It really does not make sense to write the glue code  
from scratch.  And it's not as if the rest of QEMU had just been fixed,  
and this patch reintroduces the old stuff.  I count 166 instances of  
"SysBusDevice busdev" and only 9 instances of "SysBusDevice  
parent_obj".  Perhaps these could all be fixed up in an automated way?

And "making other people even more work" goes both ways...

> >> > +static int kvm_openpic_init(SysBusDevice *dev)
> >>
> >> Please make this instance_init + realize functions - "dev" should  
> rather
> >> be reserved for DeviceState.
> >
> > Could you elaborate?  I'm really not familiar with this stuff, and  
> have
> > not found much documentation.  Again, this is patterned after the  
> existing
> > non-KVM openpic file.
> 
> static void kvm_openpic_init(Object *obj) should initialize simple
> variables, MemoryRegions that don't depend on parameters and any QOM
> properties.
> 
> static void kvm_openpic_realize(DeviceState *dev, Error **errp) should
> initialize the rest.
> 
> kvm_openpic_unrealize(Device *dev, Error **errp) and
> kvm_openpic_finalize(Object *obj) would be their counterparts for  
> cleanup.

When would kvm_openpic_realize/unrealize/finalize get called?

Note that we must create the kernel side of the device in  
kvm_openpic_init(), because we need to return failure if it's not  
supported, so that the platform can fall back onto creating a normal  
QEMU openpic instead.

Also note that an in-kernel MPIC cannot be destroyed, without  
destroying the entire VM.  So I'm not sure what unrealize/finalize  
would do.

All of this is basically done the way Alex told/showed me to do it.

> >> > +{
> >> > +    KVMState *s = kvm_state;
> >> > +    KVMOpenPICState *opp = FROM_SYSBUS(typeof(*opp), dev);
> >>
> >> NACK, please introduce your own KVM_OPENPIC(obj) cast macro  
> instead for
> >> new devices - has been a topic for several weeks and months now.
> >
> > There's way too much traffic on the list for me to know about  
> something
> > just because it's "been a topic".  Lately it's been too much for me  
> to
> > even scan the subject lines looking for things that look important,
> > given that QEMU is only a fraction of what I spend my time on.
> >
> > If you'd like to update hw/intc/openpic.c to comply with the style  
> of
> > the day, then this could be updated to match...
> >
> > Also note that this is hardly the first time this patch has been  
> posted
> > (v1 was a few weeks ago, and there were RFC patches well before  
> that).
> > The first version may have even preceded this "topic".  This seems  
> a bit
> > late in the process for a bunch of style churn, when existing code
> > hasn't been updated.
> 
> I'm not talking about style churn, I'm talking about using the wrong
> infrastructure and making it even more difficult to drop FROM_SYSBUS()
> macro.

Sigh.  From what you said above, it seemed like you were asking me to  
do this:

#define KVM_OPENPIC(obj) FROM_SYSBUS(KVMOpenPICState, (obj))

Do you mean you want me to use DO_UPCAST directly?  And is it forbidden  
for DO_UPCAST to be opencoded?

> Again, whether or not some particular other file uses some style  
> doesn't
> mean that it's okay to apply that to new files. Instead of complaining
> it would've been a task of five minutes to supply Alex with a fixup
> patch to squash/follow-up or to post a v3. QOM realize is merged since
> January 2013 and was presented by Anthony in January 2012.

It would have taken me much more than 5 minutes because I had no clue  
what "QOM realize" was, not to mention other unresolved questions about  
what you would want in a v3.

Alex, are you expecting a v3 or are you happy with the version you  
already put in ppc-next?

-Scott

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Qemu-devel] [PATCH ppc-next fixup] intc/openpic_kvm: Fix QOM and build issues
  2013-06-12 20:32 [Qemu-devel] [PATCH v2] kvm/openpic: in-kernel mpic support Scott Wood
  2013-06-12 20:56 ` Alexander Graf
  2013-06-13 11:01 ` Andreas Färber
@ 2013-06-16 18:32 ` Andreas Färber
  2013-06-16 19:34   ` Andreas Färber
  2013-06-16 19:25 ` [Qemu-devel] [PATCH v2] kvm/openpic: in-kernel mpic support Andreas Färber
  2013-06-16 19:30 ` [Qemu-devel] [PATCH ppc-next fixup v2] intc/openpic_kvm: Fix QOM and build issues Andreas Färber
  4 siblings, 1 reply; 15+ messages in thread
From: Andreas Färber @ 2013-06-16 18:32 UTC (permalink / raw)
  To: qemu-devel; +Cc: Scott Wood, qemu-ppc, Andreas Färber, Alexander Graf

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 default-configs/ppcemb-softmmu.mak |  1 +
 hw/intc/openpic_kvm.c              | 50 +++++++++++++++++++++++---------------
 hw/ppc/e500.c                      |  2 +-
 include/hw/ppc/openpic.h           |  1 +
 4 files changed, 34 insertions(+), 20 deletions(-)

diff --git a/default-configs/ppcemb-softmmu.mak b/default-configs/ppcemb-softmmu.mak
index be93e03..97be9f6 100644
--- a/default-configs/ppcemb-softmmu.mak
+++ b/default-configs/ppcemb-softmmu.mak
@@ -38,5 +38,6 @@ CONFIG_XILINX=y
 CONFIG_XILINX_ETHLITE=y
 CONFIG_OPENPIC=y
 CONFIG_E500=$(CONFIG_FDT)
+CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
 # For PReP
 CONFIG_MC146818RTC=y
diff --git a/hw/intc/openpic_kvm.c b/hw/intc/openpic_kvm.c
index 17d0a35..8818b5d 100644
--- a/hw/intc/openpic_kvm.c
+++ b/hw/intc/openpic_kvm.c
@@ -31,8 +31,14 @@
 #include "sysemu/kvm.h"
 #include "qemu/log.h"
 
+#define KVM_OPENPIC(obj) \
+    OBJECT_CHECK(KVMOpenPICState, (obj), TYPE_KVM_OPENPIC)
+
 typedef struct KVMOpenPICState {
+    /*< private >*/
     SysBusDevice busdev;
+    /*< public >*/
+
     MemoryRegion mem;
     MemoryListener mem_listener;
     uint32_t fd;
@@ -145,16 +151,26 @@ static void kvm_openpic_region_del(MemoryListener *listener,
     }
 }
 
-static int kvm_openpic_init(SysBusDevice *dev)
+static void kvm_openpic_init(Object *obj)
+{
+    KVMOpenPICState *opp = KVM_OPENPIC(obj);
+
+    memory_region_init_io(&opp->mem, &kvm_openpic_mem_ops, opp,
+                          "kvm-openpic", 0x40000);
+}
+
+static void kvm_openpic_realize(DeviceState *dev, Error **errp)
 {
+    SysBusDevice *d = SYS_BUS_DEVICE(dev);
+    KVMOpenPICState *opp = KVM_OPENPIC(dev);
     KVMState *s = kvm_state;
-    KVMOpenPICState *opp = FROM_SYSBUS(typeof(*opp), dev);
     int kvm_openpic_model;
     struct kvm_create_device cd = {0};
     int ret, i;
 
     if (!kvm_check_extension(s, KVM_CAP_DEVICE_CTRL)) {
-        return -EINVAL;
+        error_setg(errp, "Kernel is lacking Device Control API");
+        return;
     }
 
     switch (opp->model) {
@@ -167,23 +183,21 @@ static int kvm_openpic_init(SysBusDevice *dev)
         break;
 
     default:
-        return -EINVAL;
+        error_setg(errp, "Unsupported OpenPIC model %" PRIu32, opp->model);
+        return;
     }
 
     cd.type = kvm_openpic_model;
     ret = kvm_vm_ioctl(s, KVM_CREATE_DEVICE, &cd);
     if (ret < 0) {
-        qemu_log_mask(LOG_UNIMP, "%s: can't create device %d: %s\n",
-                      __func__, cd.type, strerror(errno));
-        return -EINVAL;
+        error_setg(errp, "Can't create device %d: %s",
+                   cd.type, strerror(errno));
+        return;
     }
     opp->fd = cd.fd;
 
-    memory_region_init_io(&opp->mem, &kvm_openpic_mem_ops, opp,
-                          "kvm-openpic", 0x40000);
-
-    sysbus_init_mmio(dev, &opp->mem);
-    qdev_init_gpio_in(&dev->qdev, kvm_openpic_set_irq, OPENPIC_MAX_IRQ);
+    sysbus_init_mmio(d, &opp->mem);
+    qdev_init_gpio_in(dev, kvm_openpic_set_irq, OPENPIC_MAX_IRQ);
 
     opp->mem_listener.region_add = kvm_openpic_region_add;
     opp->mem_listener.region_add = kvm_openpic_region_del;
@@ -205,8 +219,6 @@ static int kvm_openpic_init(SysBusDevice *dev)
     kvm_gsi_routing_allowed = true;
 
     kvm_irqchip_commit_routes(s);
-
-    return 0;
 }
 
 int kvm_openpic_connect_vcpu(DeviceState *d, CPUState *cs)
@@ -227,20 +239,20 @@ static Property kvm_openpic_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
-static void kvm_openpic_class_init(ObjectClass *klass, void *data)
+static void kvm_openpic_class_init(ObjectClass *oc, void *data)
 {
-    DeviceClass *dc = DEVICE_CLASS(klass);
-    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
+    DeviceClass *dc = DEVICE_CLASS(oc);
 
-    k->init = kvm_openpic_init;
+    dc->realize = kvm_openpic_realize;
     dc->props = kvm_openpic_properties;
     dc->reset = kvm_openpic_reset;
 }
 
 static const TypeInfo kvm_openpic_info = {
-    .name          = "kvm-openpic",
+    .name          = TYPE_KVM_OPENPIC,
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(KVMOpenPICState),
+    .instance_init = kvm_openpic_init,
     .class_init    = kvm_openpic_class_init,
 };
 
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index d38a688..47e8cb6 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -504,7 +504,7 @@ static DeviceState *ppce500_init_mpic_kvm(PPCE500Params *params,
     CPUState *cs;
     int r;
 
-    dev = qdev_create(NULL, "kvm-openpic");
+    dev = qdev_create(NULL, TYPE_KVM_OPENPIC);
     qdev_prop_set_uint32(dev, "model", params->mpic_version);
 
     r = qdev_init(dev);
diff --git a/include/hw/ppc/openpic.h b/include/hw/ppc/openpic.h
index 1fe4865..9628277 100644
--- a/include/hw/ppc/openpic.h
+++ b/include/hw/ppc/openpic.h
@@ -24,6 +24,7 @@ enum {
 #define OPENPIC_MAX_IRQ     (OPENPIC_MAX_SRC + OPENPIC_MAX_IPI + \
                              OPENPIC_MAX_TMR)
 
+#define TYPE_KVM_OPENPIC "kvm-openpic"
 int kvm_openpic_connect_vcpu(DeviceState *d, CPUState *cs);
 
 #endif /* __OPENPIC_H__ */
-- 
1.8.1.4

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v2] kvm/openpic: in-kernel mpic support
  2013-06-14 22:57       ` Scott Wood
@ 2013-06-16 19:11         ` Andreas Färber
  2013-06-19 22:25           ` Scott Wood
  0 siblings, 1 reply; 15+ messages in thread
From: Andreas Färber @ 2013-06-16 19:11 UTC (permalink / raw)
  To: Scott Wood; +Cc: qemu-ppc, Alexander Graf, qemu-devel

Am 15.06.2013 00:57, schrieb Scott Wood:
> On 06/14/2013 09:59:18 AM, Andreas Färber wrote:
>> Am 13.06.2013 19:32, schrieb Scott Wood:
>> > On 06/13/2013 06:01:49 AM, Andreas Färber wrote:
>> >> Am 12.06.2013 22:32, schrieb Scott Wood:
>> >> > +typedef struct KVMOpenPICState {
>> >> > +    SysBusDevice busdev;
>> >>
>> >> SysBusDevice parent_obj; please!
>> >>
>> >> http://wiki.qemu.org/QOMConventions
>> >
>> > The word "QOMConventions" doesn't exist once in the QEMU source tree.
>> > How is one supposed to know that this documentation exists? :-P
>>
>> I do expect people to read at least the subject of patch series on
>> qemu-devel,
> 
> I have over 12000 currently unread e-mails from that list.

FWIW I have over 53000 unread.

>  They are not
> separated into "patch" and "other".  Even among the patches, they get
> posted at least twice due to the (unique to QEMU and KVM as far as I can
> tell) practice of reposting everything before a pull request.
> 
> There's just no way I can keep up with all of this, *plus* all the
> non-QEMU stuff I need to keep up with.  Sorry.  I generally rely on Alex
> to guide me on things like qdev/QOM.  Quite frankly, I didn't even
> realize I was using QOM.  I thought this was still qdev.  I even create
> it using qdev_create()...
> 
>> short of individual review comments. CPU, PReP PCI,
>> Versatile PCI, ISA and more recently virtio series had been posted.
> 
> ...which of those would make me think "hmm, there's something in here
> that I need to read before submitting patches for in-kernel mpic"?
> 
> I'm not trying to be difficult -- I'm just trying to point out that
> there's room for improvement in how the QEMU community communicates to
> developers what is expected.  Maybe an announcement list that just
> contains important updates and summaries?

+1 - but surely not all changes would get communicated on such a list.

>  Also, even starting on
> http://wiki.qemu.org/ I don't see any obvious path to get to
> QOMConventions.  It's not even linked to from the main QOM page.
> 
> I do understand the frustration of having to correct people on the same
> things over and over -- I experience it myself in other contexts.  But
> there are more constructive ways to deal with it than exclamation points.
> 
>> > Plus, this is just copied from the non-KVM MPIC file, and I see many
>> > other instances throughout the source tree.
>>
>> Which exactly is the reason for my grief: Your ignorance is making other
>> people even more work, and at least Alex should've spotted it - I can't
>> review all patches just because they might or might not be touching on
>> QOM.
>> Just as we are supposed to not copy old Coding Style in new patches, we
>> should be applying new patterns and conventions in new patches, too.
> 
> I'm usually the first person to complain about bad copy and paste, but
> this is a situation where the KVM version of openpic is supposed to
> interface with the rest of the system in exactly the same way as the
> regular openpic.  It really does not make sense to write the glue code
> from scratch.  And it's not as if the rest of QEMU had just been fixed,
> and this patch reintroduces the old stuff.  I count 166 instances of
> "SysBusDevice busdev" and only 9 instances of "SysBusDevice
> parent_obj".  Perhaps these could all be fixed up in an automated way?

No, they can't, because FROM_SYSBUS() needs to be replaced with
per-device macros like I asked you to introduce for your new device. And
what code to put in which function is not automatic either.

> And "making other people even more work" goes both ways...

Right. But if you're complaining about QOM and QOM realize, address your
complaints to Anthony instead. :)
We're a community, and sending patches in write-only mode conflicts with
my understanding of being e500 co-maintainer. I have no personal
advantage of this e500 KVM PIC, it just makes more work for me. So it's
your job to keep the code from bitrotting, especially when not everyone
can actually compile-test it.

Anyway, I have just sent Alex a fixup patch to squash as code says more
than a thousand words - now you have no more excuses for the future. :P

>> >> > +static int kvm_openpic_init(SysBusDevice *dev)
>> >>
>> >> Please make this instance_init + realize functions - "dev" should
>> rather
>> >> be reserved for DeviceState.
>> >
>> > Could you elaborate?  I'm really not familiar with this stuff, and have
>> > not found much documentation.  Again, this is patterned after the
>> existing
>> > non-KVM openpic file.
>>
>> static void kvm_openpic_init(Object *obj) should initialize simple
>> variables, MemoryRegions that don't depend on parameters and any QOM
>> properties.
>>
>> static void kvm_openpic_realize(DeviceState *dev, Error **errp) should
>> initialize the rest.
>>
>> kvm_openpic_unrealize(Device *dev, Error **errp) and
>> kvm_openpic_finalize(Object *obj) would be their counterparts for
>> cleanup.
> 
> When would kvm_openpic_realize/unrealize/finalize get called?

Today realize is called as part of your qdev_init_nofail() or via
object_property_set_bool().
In the future it will be called last thing before the machine starts
executing - therefore moving basic initializations into instance_init.
Documented in include/hw/qdev-core.h.

> Note that we must create the kernel side of the device in
> kvm_openpic_init(), because we need to return failure if it's not
> supported, so that the platform can fall back onto creating a normal
> QEMU openpic instead.

No, you don't have to and you shouldn't. SysBusDeviceClass::init is
legacy cruft. As you can see from my patch, kvm_openpic_realize allows
for even better error reporting.

> Also note that an in-kernel MPIC cannot be destroyed, without destroying
> the entire VM.  So I'm not sure what unrealize/finalize would do.

I mentioned them for completeness. Which of them apply to your devices
is for you to determine.

> All of this is basically done the way Alex told/showed me to do it.

Yes, Alex should've kept an eye on it before applying it, but your
colleague Bharat also has experiences with QOM if you have questions.

>> >> > +{
>> >> > +    KVMState *s = kvm_state;
>> >> > +    KVMOpenPICState *opp = FROM_SYSBUS(typeof(*opp), dev);
>> >>
>> >> NACK, please introduce your own KVM_OPENPIC(obj) cast macro instead
>> for
>> >> new devices - has been a topic for several weeks and months now.
>> >
>> > There's way too much traffic on the list for me to know about something
>> > just because it's "been a topic".  Lately it's been too much for me to
>> > even scan the subject lines looking for things that look important,
>> > given that QEMU is only a fraction of what I spend my time on.
>> >
>> > If you'd like to update hw/intc/openpic.c to comply with the style of
>> > the day, then this could be updated to match...
>> >
>> > Also note that this is hardly the first time this patch has been posted
>> > (v1 was a few weeks ago, and there were RFC patches well before that).
>> > The first version may have even preceded this "topic".  This seems a
>> bit
>> > late in the process for a bunch of style churn, when existing code
>> > hasn't been updated.
>>
>> I'm not talking about style churn, I'm talking about using the wrong
>> infrastructure and making it even more difficult to drop FROM_SYSBUS()
>> macro.
> 
> Sigh.  From what you said above, it seemed like you were asking me to do
> this:
> 
> #define KVM_OPENPIC(obj) FROM_SYSBUS(KVMOpenPICState, (obj))

No. It's implemented using OBJECT_CHECK(), see include/qom/object.h.
KVM_OPENPIC() is supposed to allow any input type, not just SysBusDevice.

> Do you mean you want me to use DO_UPCAST directly?  And is it forbidden
> for DO_UPCAST to be opencoded?

Answered in the link I gave (quote):
* DO use QOM cast macros (based on struct layout)
* DON'T rely on qdev's DO_UPCAST() macro (field names)

You're not supposed to touch parent fields outside VMStateDescription -
that's exactly what the QOM cast macros and per-type variables hide.

Andreas

>> Again, whether or not some particular other file uses some style doesn't
>> mean that it's okay to apply that to new files. Instead of complaining
>> it would've been a task of five minutes to supply Alex with a fixup
>> patch to squash/follow-up or to post a v3. QOM realize is merged since
>> January 2013 and was presented by Anthony in January 2012.
> 
> It would have taken me much more than 5 minutes because I had no clue
> what "QOM realize" was, not to mention other unresolved questions about
> what you would want in a v3.
> 
> Alex, are you expecting a v3 or are you happy with the version you
> already put in ppc-next?
> 
> -Scott

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v2] kvm/openpic: in-kernel mpic support
  2013-06-12 20:32 [Qemu-devel] [PATCH v2] kvm/openpic: in-kernel mpic support Scott Wood
                   ` (2 preceding siblings ...)
  2013-06-16 18:32 ` [Qemu-devel] [PATCH ppc-next fixup] intc/openpic_kvm: Fix QOM and build issues Andreas Färber
@ 2013-06-16 19:25 ` Andreas Färber
  2013-06-19 21:49   ` Scott Wood
  2013-07-01 16:54   ` Andreas Färber
  2013-06-16 19:30 ` [Qemu-devel] [PATCH ppc-next fixup v2] intc/openpic_kvm: Fix QOM and build issues Andreas Färber
  4 siblings, 2 replies; 15+ messages in thread
From: Andreas Färber @ 2013-06-16 19:25 UTC (permalink / raw)
  To: Scott Wood, Alexander Graf; +Cc: qemu-ppc, qemu-devel

Subject is misleading: it's intc/openpic_kvm, not kvm/openpic. Alex,
please fix when squashing.

Am 12.06.2013 22:32, schrieb Scott Wood:
> Enables support for the in-kernel MPIC that thas been merged into the
> KVM next branch.  This includes irqfd/KVM_IRQ_LINE support from Alex
> Graf (along with some other improvements).
> 
> Note from Alex regarding kvm_irqchip_create():
> 
>   On x86, one would call kvm_irqchip_create() to initialize an
>   in-kernel interrupt controller.  That function then goes ahead and
>   initializes global capability variables as well as the default irq
>   routing table.
> 
>   On ppc, we can't call kvm_irqchip_create() because we can have
>   different types of interrupt controllers.  So we want to do all the
>   things that function would do for us in the in-kernel device init
>   handler.
> 
> Signed-off-by: Scott Wood <scottwood@freescale.com>
> ---
> v2: fix "llx" -> PRI_x64, and remove some broken leftover code
> involving reg_base.
> ---
>  default-configs/ppc-softmmu.mak   |    1 +
>  default-configs/ppc64-softmmu.mak |    1 +

This breaks KVM-enabled ppcemb-softmmu build with unresolved symbol
kvm_openpic_connect_vcpu() in e500.o. Fix in my patch:

http://patchwork.ozlabs.org/patch/251731/

Because intc/openpic.c gets rebuilt for each of the three ppc*-softmmu,
I added a patch to my qom-cpu-10 series to stop that for openpic. For
openpic_kvm I believe that won't be possible due to sysemu/kvm.h's
inline stubs.

Andreas

>  hw/intc/Makefile.objs             |    1 +
>  hw/intc/openpic_kvm.c             |  250 +++++++++++++++++++++++++++++++++++++
>  hw/ppc/e500.c                     |   79 +++++++++++-
>  include/hw/ppc/openpic.h          |    2 +-
>  6 files changed, 328 insertions(+), 6 deletions(-)
>  create mode 100644 hw/intc/openpic_kvm.c

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Qemu-devel] [PATCH ppc-next fixup v2] intc/openpic_kvm: Fix QOM and build issues
  2013-06-12 20:32 [Qemu-devel] [PATCH v2] kvm/openpic: in-kernel mpic support Scott Wood
                   ` (3 preceding siblings ...)
  2013-06-16 19:25 ` [Qemu-devel] [PATCH v2] kvm/openpic: in-kernel mpic support Andreas Färber
@ 2013-06-16 19:30 ` Andreas Färber
  2013-06-18 12:39   ` Alexander Graf
  4 siblings, 1 reply; 15+ messages in thread
From: Andreas Färber @ 2013-06-16 19:30 UTC (permalink / raw)
  To: qemu-devel; +Cc: Scott Wood, qemu-ppc, Andreas Färber, Alexander Graf

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 v1 -> v2:
 * Renamed busdev to parent_obj;
 * Fixed one remaining occurrence of FROM_SYSBUS().

 default-configs/ppcemb-softmmu.mak |  1 +
 hw/intc/openpic_kvm.c              | 54 +++++++++++++++++++++++---------------
 hw/ppc/e500.c                      |  2 +-
 include/hw/ppc/openpic.h           |  1 +
 4 files changed, 36 insertions(+), 22 deletions(-)

diff --git a/default-configs/ppcemb-softmmu.mak b/default-configs/ppcemb-softmmu.mak
index be93e03..97be9f6 100644
--- a/default-configs/ppcemb-softmmu.mak
+++ b/default-configs/ppcemb-softmmu.mak
@@ -38,5 +38,6 @@ CONFIG_XILINX=y
 CONFIG_XILINX_ETHLITE=y
 CONFIG_OPENPIC=y
 CONFIG_E500=$(CONFIG_FDT)
+CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
 # For PReP
 CONFIG_MC146818RTC=y
diff --git a/hw/intc/openpic_kvm.c b/hw/intc/openpic_kvm.c
index 17d0a35..6775879 100644
--- a/hw/intc/openpic_kvm.c
+++ b/hw/intc/openpic_kvm.c
@@ -31,8 +31,14 @@
 #include "sysemu/kvm.h"
 #include "qemu/log.h"
 
+#define KVM_OPENPIC(obj) \
+    OBJECT_CHECK(KVMOpenPICState, (obj), TYPE_KVM_OPENPIC)
+
 typedef struct KVMOpenPICState {
-    SysBusDevice busdev;
+    /*< private >*/
+    SysBusDevice parent_obj;
+    /*< public >*/
+
     MemoryRegion mem;
     MemoryListener mem_listener;
     uint32_t fd;
@@ -145,16 +151,26 @@ static void kvm_openpic_region_del(MemoryListener *listener,
     }
 }
 
-static int kvm_openpic_init(SysBusDevice *dev)
+static void kvm_openpic_init(Object *obj)
+{
+    KVMOpenPICState *opp = KVM_OPENPIC(obj);
+
+    memory_region_init_io(&opp->mem, &kvm_openpic_mem_ops, opp,
+                          "kvm-openpic", 0x40000);
+}
+
+static void kvm_openpic_realize(DeviceState *dev, Error **errp)
 {
+    SysBusDevice *d = SYS_BUS_DEVICE(dev);
+    KVMOpenPICState *opp = KVM_OPENPIC(dev);
     KVMState *s = kvm_state;
-    KVMOpenPICState *opp = FROM_SYSBUS(typeof(*opp), dev);
     int kvm_openpic_model;
     struct kvm_create_device cd = {0};
     int ret, i;
 
     if (!kvm_check_extension(s, KVM_CAP_DEVICE_CTRL)) {
-        return -EINVAL;
+        error_setg(errp, "Kernel is lacking Device Control API");
+        return;
     }
 
     switch (opp->model) {
@@ -167,23 +183,21 @@ static int kvm_openpic_init(SysBusDevice *dev)
         break;
 
     default:
-        return -EINVAL;
+        error_setg(errp, "Unsupported OpenPIC model %" PRIu32, opp->model);
+        return;
     }
 
     cd.type = kvm_openpic_model;
     ret = kvm_vm_ioctl(s, KVM_CREATE_DEVICE, &cd);
     if (ret < 0) {
-        qemu_log_mask(LOG_UNIMP, "%s: can't create device %d: %s\n",
-                      __func__, cd.type, strerror(errno));
-        return -EINVAL;
+        error_setg(errp, "Can't create device %d: %s",
+                   cd.type, strerror(errno));
+        return;
     }
     opp->fd = cd.fd;
 
-    memory_region_init_io(&opp->mem, &kvm_openpic_mem_ops, opp,
-                          "kvm-openpic", 0x40000);
-
-    sysbus_init_mmio(dev, &opp->mem);
-    qdev_init_gpio_in(&dev->qdev, kvm_openpic_set_irq, OPENPIC_MAX_IRQ);
+    sysbus_init_mmio(d, &opp->mem);
+    qdev_init_gpio_in(dev, kvm_openpic_set_irq, OPENPIC_MAX_IRQ);
 
     opp->mem_listener.region_add = kvm_openpic_region_add;
     opp->mem_listener.region_add = kvm_openpic_region_del;
@@ -205,13 +219,11 @@ static int kvm_openpic_init(SysBusDevice *dev)
     kvm_gsi_routing_allowed = true;
 
     kvm_irqchip_commit_routes(s);
-
-    return 0;
 }
 
 int kvm_openpic_connect_vcpu(DeviceState *d, CPUState *cs)
 {
-    KVMOpenPICState *opp = FROM_SYSBUS(typeof(*opp), SYS_BUS_DEVICE(d));
+    KVMOpenPICState *opp = KVM_OPENPIC(d);
     struct kvm_enable_cap encap = {};
 
     encap.cap = KVM_CAP_IRQ_MPIC;
@@ -227,20 +239,20 @@ static Property kvm_openpic_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
-static void kvm_openpic_class_init(ObjectClass *klass, void *data)
+static void kvm_openpic_class_init(ObjectClass *oc, void *data)
 {
-    DeviceClass *dc = DEVICE_CLASS(klass);
-    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
+    DeviceClass *dc = DEVICE_CLASS(oc);
 
-    k->init = kvm_openpic_init;
+    dc->realize = kvm_openpic_realize;
     dc->props = kvm_openpic_properties;
     dc->reset = kvm_openpic_reset;
 }
 
 static const TypeInfo kvm_openpic_info = {
-    .name          = "kvm-openpic",
+    .name          = TYPE_KVM_OPENPIC,
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(KVMOpenPICState),
+    .instance_init = kvm_openpic_init,
     .class_init    = kvm_openpic_class_init,
 };
 
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index d38a688..47e8cb6 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -504,7 +504,7 @@ static DeviceState *ppce500_init_mpic_kvm(PPCE500Params *params,
     CPUState *cs;
     int r;
 
-    dev = qdev_create(NULL, "kvm-openpic");
+    dev = qdev_create(NULL, TYPE_KVM_OPENPIC);
     qdev_prop_set_uint32(dev, "model", params->mpic_version);
 
     r = qdev_init(dev);
diff --git a/include/hw/ppc/openpic.h b/include/hw/ppc/openpic.h
index 1fe4865..9628277 100644
--- a/include/hw/ppc/openpic.h
+++ b/include/hw/ppc/openpic.h
@@ -24,6 +24,7 @@ enum {
 #define OPENPIC_MAX_IRQ     (OPENPIC_MAX_SRC + OPENPIC_MAX_IPI + \
                              OPENPIC_MAX_TMR)
 
+#define TYPE_KVM_OPENPIC "kvm-openpic"
 int kvm_openpic_connect_vcpu(DeviceState *d, CPUState *cs);
 
 #endif /* __OPENPIC_H__ */
-- 
1.8.1.4

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH ppc-next fixup] intc/openpic_kvm: Fix QOM and build issues
  2013-06-16 18:32 ` [Qemu-devel] [PATCH ppc-next fixup] intc/openpic_kvm: Fix QOM and build issues Andreas Färber
@ 2013-06-16 19:34   ` Andreas Färber
  0 siblings, 0 replies; 15+ messages in thread
From: Andreas Färber @ 2013-06-16 19:34 UTC (permalink / raw)
  To: Alexander Graf; +Cc: Scott Wood, qemu-ppc, qemu-devel

Am 16.06.2013 20:32, schrieb Andreas Färber:
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  default-configs/ppcemb-softmmu.mak |  1 +
>  hw/intc/openpic_kvm.c              | 50 +++++++++++++++++++++++---------------
>  hw/ppc/e500.c                      |  2 +-
>  include/hw/ppc/openpic.h           |  1 +
>  4 files changed, 34 insertions(+), 20 deletions(-)
> 
> diff --git a/default-configs/ppcemb-softmmu.mak b/default-configs/ppcemb-softmmu.mak
> index be93e03..97be9f6 100644
> --- a/default-configs/ppcemb-softmmu.mak
> +++ b/default-configs/ppcemb-softmmu.mak
> @@ -38,5 +38,6 @@ CONFIG_XILINX=y
>  CONFIG_XILINX_ETHLITE=y
>  CONFIG_OPENPIC=y
>  CONFIG_E500=$(CONFIG_FDT)
> +CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
>  # For PReP
>  CONFIG_MC146818RTC=y
> diff --git a/hw/intc/openpic_kvm.c b/hw/intc/openpic_kvm.c
> index 17d0a35..8818b5d 100644
> --- a/hw/intc/openpic_kvm.c
> +++ b/hw/intc/openpic_kvm.c
> @@ -31,8 +31,14 @@
>  #include "sysemu/kvm.h"
>  #include "qemu/log.h"
>  
> +#define KVM_OPENPIC(obj) \
> +    OBJECT_CHECK(KVMOpenPICState, (obj), TYPE_KVM_OPENPIC)
> +
>  typedef struct KVMOpenPICState {
> +    /*< private >*/
>      SysBusDevice busdev;

Missed the field name, hiding another occurrence of FROM_SYSBUS() in
kvm_openpic_connect_vcpu(). v2 sent.

Andreas

> +    /*< public >*/
> +
>      MemoryRegion mem;
>      MemoryListener mem_listener;
>      uint32_t fd;
[snip]

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH ppc-next fixup v2] intc/openpic_kvm: Fix QOM and build issues
  2013-06-16 19:30 ` [Qemu-devel] [PATCH ppc-next fixup v2] intc/openpic_kvm: Fix QOM and build issues Andreas Färber
@ 2013-06-18 12:39   ` Alexander Graf
  0 siblings, 0 replies; 15+ messages in thread
From: Alexander Graf @ 2013-06-18 12:39 UTC (permalink / raw)
  To: Andreas Färber; +Cc: Scott Wood, qemu-ppc, qemu-devel


On 16.06.2013, at 21:30, Andreas Färber wrote:

> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
> v1 -> v2:
> * Renamed busdev to parent_obj;
> * Fixed one remaining occurrence of FROM_SYSBUS().

Thanks, applied to ppc-next.


Alex

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v2] kvm/openpic: in-kernel mpic support
  2013-06-16 19:25 ` [Qemu-devel] [PATCH v2] kvm/openpic: in-kernel mpic support Andreas Färber
@ 2013-06-19 21:49   ` Scott Wood
  2013-07-01 16:54   ` Andreas Färber
  1 sibling, 0 replies; 15+ messages in thread
From: Scott Wood @ 2013-06-19 21:49 UTC (permalink / raw)
  To: Andreas Färber; +Cc: qemu-ppc, Alexander Graf, qemu-devel

On 06/16/2013 02:25:04 PM, Andreas Färber wrote:
> Subject is misleading: it's intc/openpic_kvm, not kvm/openpic. Alex,
> please fix when squashing.

I meant it as a general description of the functional area, not as a  
literal pathname.  It looks like that format is more of a Linux kernel  
thing and not used by QEMU much (if at all) -- sorry about that.   
Should have been "kvm: openpic: in-kernel mpic support".

-Scott

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v2] kvm/openpic: in-kernel mpic support
  2013-06-16 19:11         ` Andreas Färber
@ 2013-06-19 22:25           ` Scott Wood
  0 siblings, 0 replies; 15+ messages in thread
From: Scott Wood @ 2013-06-19 22:25 UTC (permalink / raw)
  To: Andreas Färber; +Cc: qemu-ppc, Alexander Graf, qemu-devel

On 06/16/2013 02:11:58 PM, Andreas Färber wrote:
> Am 15.06.2013 00:57, schrieb Scott Wood:
> > ...which of those would make me think "hmm, there's something in  
> here
> > that I need to read before submitting patches for in-kernel mpic"?
> >
> > I'm not trying to be difficult -- I'm just trying to point out that
> > there's room for improvement in how the QEMU community communicates  
> to
> > developers what is expected.  Maybe an announcement list that just
> > contains important updates and summaries?
> 
> +1 - but surely not all changes would get communicated on such a list.

Sure.  It doesn't need to be perfect to be a big help.

> > And "making other people even more work" goes both ways...
> 
> Right. But if you're complaining about QOM and QOM realize, address  
> your
> complaints to Anthony instead. :)
> We're a community, and sending patches in write-only mode conflicts  
> with
> my understanding of being e500 co-maintainer.

It wasn't "write-only mode" -- I've accepted and acted on plenty of  
other feedback in this and other patchsets (in fact, some of that  
feedback told me specifically to use things like qdev_init_nofail,  
which apparently is deprecated).  And I'm not opposed to cleaning  
up/modernizing existing e500 code (or delegating it to a coworker,  
which includes Alex, who works for us part-time) if it's clear what is  
expected.  I was just looking for help with a part of QEMU that I find  
pretty opaque, and was put off by the tone of the initial complaint.

> I have no personal
> advantage of this e500 KVM PIC, it just makes more work for me. So  
> it's
> your job to keep the code from bitrotting, especially when not  
> everyone
> can actually compile-test it.
> 
> Anyway, I have just sent Alex a fixup patch to squash as code says  
> more
> than a thousand words - now you have no more excuses for the future.  
> :P

Thanks.

> > When would kvm_openpic_realize/unrealize/finalize get called?
> 
> Today realize is called as part of your qdev_init_nofail() or via
> object_property_set_bool().
> In the future it will be called last thing before the machine starts
> executing - therefore moving basic initializations into instance_init.
> Documented in include/hw/qdev-core.h.
> 
> > Note that we must create the kernel side of the device in
> > kvm_openpic_init(), because we need to return failure if it's not
> > supported, so that the platform can fall back onto creating a normal
> > QEMU openpic instead.
> 
> No, you don't have to and you shouldn't. SysBusDeviceClass::init is
> legacy cruft. As you can see from my patch, kvm_openpic_realize allows
> for even better error reporting.

My concern isn't how good the error reporting is, but how early we  
detect the error.  If qdev_init_nofail() goes away, then when will  
platform code have a chance to create a non-KVM openpic device  
instead?  From "devices must not create children during @realize" it  
sounds like it might be too late at that point.

Originally, I had the creation of the kernel MPIC object done by a  
factory function.  If the kernel object was able to be created, then  
the function created a qdev object and passed the kernel ID as a  
property.  Otherwise, it returned NULL so that platform code knew to  
create a "normal" openpic device (or error out if the user explicitly  
asked for an in-kernel pic).  Alex asked me to change this to the  
qdev_init_nofail() approach.  It sounds like we may need to change back.

-Scott

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v2] kvm/openpic: in-kernel mpic support
  2013-06-16 19:25 ` [Qemu-devel] [PATCH v2] kvm/openpic: in-kernel mpic support Andreas Färber
  2013-06-19 21:49   ` Scott Wood
@ 2013-07-01 16:54   ` Andreas Färber
  1 sibling, 0 replies; 15+ messages in thread
From: Andreas Färber @ 2013-07-01 16:54 UTC (permalink / raw)
  To: Stefan Hajnoczi, Christian Berendt
  Cc: Peter Maydell, Alexander Graf, qemu-devel, qemu-ppc,
	Gerd Hoffmann, Anthony Liguori, Scott Wood

Am 16.06.2013 21:25, schrieb Andreas Färber:
> Am 12.06.2013 22:32, schrieb Scott Wood:
>> Enables support for the in-kernel MPIC that thas been merged into the
>> KVM next branch.  This includes irqfd/KVM_IRQ_LINE support from Alex
>> Graf (along with some other improvements).
>>
>> Note from Alex regarding kvm_irqchip_create():
>>
>>   On x86, one would call kvm_irqchip_create() to initialize an
>>   in-kernel interrupt controller.  That function then goes ahead and
>>   initializes global capability variables as well as the default irq
>>   routing table.
>>
>>   On ppc, we can't call kvm_irqchip_create() because we can have
>>   different types of interrupt controllers.  So we want to do all the
>>   things that function would do for us in the in-kernel device init
>>   handler.
>>
>> Signed-off-by: Scott Wood <scottwood@freescale.com>
[...]
>> ---
>>  default-configs/ppc-softmmu.mak   |    1 +
>>  default-configs/ppc64-softmmu.mak |    1 +
> 
> This breaks KVM-enabled ppcemb-softmmu build with unresolved symbol
> kvm_openpic_connect_vcpu() in e500.o. Fix in my patch:
> 
> http://patchwork.ozlabs.org/patch/251731/

This issue brought up an interesting question:
Why didn't the buildbots report this build breakage on ppc-next branch?

Both of us didn't see any buildbot emails for quite a while - was this
disabled for some reason?

I didn't find the answer in
https://github.com/b1-systems/buildbot/blob/master/qemu-master.cfg
but I noticed that it still has Stefan's rather than Michael's trivial
tree URL.

Looking at
http://buildbot.b1-systems.de/qemu/one_line_per_build
I see that most builders fail due to configure - which I guess points at
lack of compatible libfdt or dtc submodule.

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2013-07-01 17:24 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-06-12 20:32 [Qemu-devel] [PATCH v2] kvm/openpic: in-kernel mpic support Scott Wood
2013-06-12 20:56 ` Alexander Graf
2013-06-13 11:01 ` Andreas Färber
2013-06-13 17:32   ` Scott Wood
2013-06-14 14:59     ` Andreas Färber
2013-06-14 22:57       ` Scott Wood
2013-06-16 19:11         ` Andreas Färber
2013-06-19 22:25           ` Scott Wood
2013-06-16 18:32 ` [Qemu-devel] [PATCH ppc-next fixup] intc/openpic_kvm: Fix QOM and build issues Andreas Färber
2013-06-16 19:34   ` Andreas Färber
2013-06-16 19:25 ` [Qemu-devel] [PATCH v2] kvm/openpic: in-kernel mpic support Andreas Färber
2013-06-19 21:49   ` Scott Wood
2013-07-01 16:54   ` Andreas Färber
2013-06-16 19:30 ` [Qemu-devel] [PATCH ppc-next fixup v2] intc/openpic_kvm: Fix QOM and build issues Andreas Färber
2013-06-18 12:39   ` Alexander Graf

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.