[Qemu-devel] [PATCH 0/5] Platform device support

[Qemu-devel] [PATCH 0/5] Platform device support

[Alexander Graf]

Platforms without ISA and/or PCI have had a seriously hard time in the dynamic
device creation world of QEMU. Devices on these were modeled as SysBus devices
which can only be instantiated in machine files, not through -device.

Why is that so?

Well, SysBus is trying to be incredibly generic. It allows you to plug any
interrupt sender into any other interrupt receiver. It allows you to map
a device's memory regions into any other random memory region. All of that
only works from C code or via really complicated command line arguments under
discussion upstream right now.

But do we need that level of complexity for normal devices usually? In a
normal platform world (SoCs, PV machines) we have a flat memory layout we
can plug our device memory into. We also have a flat IRQ model where we
can plug our device IRQs into.

So the idea for platform devices arose. A platform device is really just a
device that exposes its qemu_irq slots and memory regions to the world.

That allows us to write machine specific code that maps a platform device
wherever the machine thinks fits nicely. It also allows that same machine
to generate a device tree entry for platform devices easily, as it is fully
aware of the interrupt lines and places it was mapped to.

A device (read: user) may or may not explictly request to be mapped at a
specific IRQ and/or memory address. If no explicit mapping is requested,
platform devices can get mapped at convenient places by the machine.

The actual pressing issue this solves is that today it's impossible to spawn
serial ports from the command line. With this patch set, it's possible to
do so. But it lays the groundwork for much more...

Alexander Graf (5):
  Platform: Add platform device class
  Platform: Add serial device
  PPC: e500: Only create dt entries for existing serial ports
  PPC: e500: Support platform devices
  PPC: e500: Add support for platform serial devices

 default-configs/ppc-softmmu.mak   |   2 +
 default-configs/ppc64-softmmu.mak |   2 +
 hw/Makefile.objs                  |   1 +
 hw/char/Makefile.objs             |   1 +
 hw/char/serial-platform.c         | 103 +++++++++++++++
 hw/platform/Makefile.objs         |   1 +
 hw/platform/device.c              | 108 ++++++++++++++++
 hw/ppc/e500.c                     | 262 +++++++++++++++++++++++++++++++++++++-
 hw/ppc/e500.h                     |   1 +
 hw/ppc/e500plat.c                 |   1 +
 include/hw/char/serial-platform.h |  56 ++++++++
 include/hw/platform/device.h      |  45 +++++++
 12 files changed, 577 insertions(+), 6 deletions(-)
 create mode 100644 hw/char/serial-platform.c
 create mode 100644 hw/platform/Makefile.objs
 create mode 100644 hw/platform/device.c
 create mode 100644 include/hw/char/serial-platform.h
 create mode 100644 include/hw/platform/device.h

-- 
1.8.1.4

[Qemu-devel] [PATCH 1/5] Platform: Add platform device class

[Alexander Graf]

This patch adds a new device class called "platform device". This is an
abstract class for consumption of actual classes that implement devices.

The new thing about platform devices is that they have awareness of all
memory regions and IRQs that the device exposes. That gives us the ability
to manually specify thing using properties from the command line.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/Makefile.objs             |   1 +
 hw/platform/Makefile.objs    |   1 +
 hw/platform/device.c         | 108 +++++++++++++++++++++++++++++++++++++++++++
 include/hw/platform/device.h |  45 ++++++++++++++++++
 4 files changed, 155 insertions(+)
 create mode 100644 hw/platform/Makefile.objs
 create mode 100644 hw/platform/device.c
 create mode 100644 include/hw/platform/device.h

diff --git a/hw/Makefile.objs b/hw/Makefile.objs
index d178b65..f300f68 100644
--- a/hw/Makefile.objs
+++ b/hw/Makefile.objs
@@ -20,6 +20,7 @@ devices-dirs-$(CONFIG_SOFTMMU) += nvram/
 devices-dirs-$(CONFIG_SOFTMMU) += pci/
 devices-dirs-$(CONFIG_PCI) += pci-bridge/ pci-host/
 devices-dirs-$(CONFIG_SOFTMMU) += pcmcia/
+devices-dirs-$(CONFIG_PLATFORM) += platform/
 devices-dirs-$(CONFIG_SOFTMMU) += scsi/
 devices-dirs-$(CONFIG_SOFTMMU) += sd/
 devices-dirs-$(CONFIG_SOFTMMU) += ssi/
diff --git a/hw/platform/Makefile.objs b/hw/platform/Makefile.objs
new file mode 100644
index 0000000..824356b
--- /dev/null
+++ b/hw/platform/Makefile.objs
@@ -0,0 +1 @@
+common-obj-$(CONFIG_PLATFORM) += device.o
diff --git a/hw/platform/device.c b/hw/platform/device.c
new file mode 100644
index 0000000..9e23370
--- /dev/null
+++ b/hw/platform/device.c
@@ -0,0 +1,108 @@
+/*
+ * Platform Device that can expose its memory regions and IRQ lines
+ *
+ * Copyright 2014 Freescale Semiconductor, Inc.
+ *
+ * Authors: Alexander Graf,   <agraf@suse.de>
+ *
+ * This is free software; you can redistribute it and/or modify
+ * it under the terms of  the GNU General  Public License as published by
+ * the Free Software Foundation;  either version 2 of the  License, or
+ * (at your option) any later version.
+ *
+ *
+ * This is an abstract platform device, so you really only want to use it
+ * as parent class for platform devices.
+ *
+ * It ensures that all boilerplate is properly abstracted away from children
+ * and consistent across devices.
+ *
+ * When instantiating a platform device you can optionally always specify 2
+ * properties which otherwise get populated automatically:
+ *
+ *  regions: Offsets in the platform hole the device's memory regions get mapped
+ *           to.
+ *  irqs: IRQ pins in the linear platform IRQ range the device's IRQs get mapped
+ *           to.
+ */
+
+#include "qemu-common.h"
+#include "hw/platform/device.h"
+
+static void fixup_regions(PlatformDeviceState *s)
+{
+    uint64_t *addrs = g_new(uint64_t, s->num_regions);
+    int i;
+
+    /* Treat memory offsets that the user did not specify as dynamic */
+    for (i = 0; i < s->num_regions; i++) {
+        if (s->num_plat_region_addrs > i) {
+            addrs[i] = s->plat_region_addrs[i];
+        } else {
+            addrs[i] = PLATFORM_DYNAMIC;
+        }
+    }
+
+    s->plat_region_addrs = addrs;
+    s->num_plat_region_addrs = s->num_regions;
+}
+
+static void fixup_irqs(PlatformDeviceState *s)
+{
+    uint32_t *irqs = g_new(uint32_t, s->num_irqs);
+    int i;
+
+    /* Treat IRQs that the user did not specify as dynamic */
+    for (i = 0; i < s->num_irqs; i++) {
+        if (s->num_plat_irqs > i) {
+            irqs[i] = s->plat_irqs[i];
+        } else {
+            irqs[i] = PLATFORM_DYNAMIC;
+        }
+    }
+
+    s->plat_irqs = irqs;
+    s->num_plat_irqs = s->num_irqs;
+}
+
+static void platform_device_realize(DeviceState *dev, Error **errp)
+{
+    PlatformDeviceState *s = PLATFORM_DEVICE(dev);
+
+    fixup_regions(s);
+    fixup_irqs(s);
+}
+
+static Property platform_device_properties[] = {
+    /* memory regions for a device */
+    DEFINE_PROP_ARRAY("regions", PlatformDeviceState, num_plat_region_addrs,
+                      plat_region_addrs, qdev_prop_uint64, uint64_t),
+    /* interrupts for a device */
+    DEFINE_PROP_ARRAY("irqs", PlatformDeviceState, num_plat_irqs,
+                      plat_irqs, qdev_prop_uint32, uint32_t),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void platform_device_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    dc->realize = platform_device_realize;
+    dc->props = platform_device_properties;
+}
+
+static const TypeInfo platform_device_type_info = {
+    .name = TYPE_PLATFORM_DEVICE,
+    .parent = TYPE_DEVICE,
+    .instance_size = sizeof(PlatformDeviceState),
+    .abstract = true,
+    .class_init = platform_device_class_init,
+};
+
+
+static void device_register_types(void)
+{
+    type_register_static(&platform_device_type_info);
+}
+
+type_init(device_register_types)
diff --git a/include/hw/platform/device.h b/include/hw/platform/device.h
new file mode 100644
index 0000000..a17f79e
--- /dev/null
+++ b/include/hw/platform/device.h
@@ -0,0 +1,45 @@
+/*
+ * Generic platform device with IRQ and IO placement hints
+ *
+ * Copyright 2014 Freescale Semiconductor, Inc.
+ *
+ * Authors: Alexander Graf,   <agraf@suse.de>
+ *
+ * This is free software; you can redistribute it and/or modify
+ * it under the terms of  the GNU General  Public License as published by
+ * the Free Software Foundation;  either version 2 of the  License, or
+ * (at your option) any later version.
+ */
+
+#ifndef QEMU_HW_PLATFORM_DEVICE_H
+#define QEMU_HW_PLATFORM_DEVICE_H
+
+#include "qemu-common.h"
+#include "hw/qdev.h"
+
+#define TYPE_PLATFORM_DEVICE "platform-device"
+#define PLATFORM_DEVICE(obj) OBJECT_CHECK(PlatformDeviceState, (obj), TYPE_PLATFORM_DEVICE)
+#define PLATFORM_DYNAMIC -1LL
+
+typedef struct PlatformDeviceState {
+    /*< private >*/
+
+    DeviceState parent_obj;
+    QTAILQ_ENTRY(PlatformDeviceState) next;
+
+    /* these get set by children to indicate their resources */
+    uint32_t num_regions;
+    MemoryRegion **regions;
+    uint32_t num_irqs;
+    qemu_irq **irqs;
+
+    /*< public >*/
+
+    /* these get set by the user though qdev parameters */
+    uint32_t num_plat_region_addrs;
+    uint64_t *plat_region_addrs;
+    uint32_t num_plat_irqs;
+    uint32_t *plat_irqs;
+} PlatformDeviceState;
+
+#endif /* !QEMU_HW_PLATFORM_DEVICE_H */
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 1/5] Platform: Add platform device class

[Eric Auger]

On 06/04/2014 02:28 PM, Alexander Graf wrote:
> This patch adds a new device class called "platform device". This is an
> abstract class for consumption of actual classes that implement devices.
> 
> The new thing about platform devices is that they have awareness of all
> memory regions and IRQs that the device exposes. That gives us the ability
> to manually specify thing using properties from the command line.

Hi Alex,

thanks for this serie. I am currently reworking the vfio-platform device
to inherit from this device instead of SysBusDevice.

Best Regards

Eric
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  hw/Makefile.objs             |   1 +
>  hw/platform/Makefile.objs    |   1 +
>  hw/platform/device.c         | 108 +++++++++++++++++++++++++++++++++++++++++++
>  include/hw/platform/device.h |  45 ++++++++++++++++++
>  4 files changed, 155 insertions(+)
>  create mode 100644 hw/platform/Makefile.objs
>  create mode 100644 hw/platform/device.c
>  create mode 100644 include/hw/platform/device.h
> 
> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> index d178b65..f300f68 100644
> --- a/hw/Makefile.objs
> +++ b/hw/Makefile.objs
> @@ -20,6 +20,7 @@ devices-dirs-$(CONFIG_SOFTMMU) += nvram/
>  devices-dirs-$(CONFIG_SOFTMMU) += pci/
>  devices-dirs-$(CONFIG_PCI) += pci-bridge/ pci-host/
>  devices-dirs-$(CONFIG_SOFTMMU) += pcmcia/
> +devices-dirs-$(CONFIG_PLATFORM) += platform/
>  devices-dirs-$(CONFIG_SOFTMMU) += scsi/
>  devices-dirs-$(CONFIG_SOFTMMU) += sd/
>  devices-dirs-$(CONFIG_SOFTMMU) += ssi/
> diff --git a/hw/platform/Makefile.objs b/hw/platform/Makefile.objs
> new file mode 100644
> index 0000000..824356b
> --- /dev/null
> +++ b/hw/platform/Makefile.objs
> @@ -0,0 +1 @@
> +common-obj-$(CONFIG_PLATFORM) += device.o
> diff --git a/hw/platform/device.c b/hw/platform/device.c
> new file mode 100644
> index 0000000..9e23370
> --- /dev/null
> +++ b/hw/platform/device.c
> @@ -0,0 +1,108 @@
> +/*
> + * Platform Device that can expose its memory regions and IRQ lines
> + *
> + * Copyright 2014 Freescale Semiconductor, Inc.
> + *
> + * Authors: Alexander Graf,   <agraf@suse.de>
> + *
> + * This is free software; you can redistribute it and/or modify
> + * it under the terms of  the GNU General  Public License as published by
> + * the Free Software Foundation;  either version 2 of the  License, or
> + * (at your option) any later version.
> + *
> + *
> + * This is an abstract platform device, so you really only want to use it
> + * as parent class for platform devices.
> + *
> + * It ensures that all boilerplate is properly abstracted away from children
> + * and consistent across devices.
> + *
> + * When instantiating a platform device you can optionally always specify 2
> + * properties which otherwise get populated automatically:
> + *
> + *  regions: Offsets in the platform hole the device's memory regions get mapped
> + *           to.
> + *  irqs: IRQ pins in the linear platform IRQ range the device's IRQs get mapped
> + *           to.
> + */
> +
> +#include "qemu-common.h"
> +#include "hw/platform/device.h"
> +
> +static void fixup_regions(PlatformDeviceState *s)
> +{
> +    uint64_t *addrs = g_new(uint64_t, s->num_regions);
> +    int i;
> +
> +    /* Treat memory offsets that the user did not specify as dynamic */
> +    for (i = 0; i < s->num_regions; i++) {
> +        if (s->num_plat_region_addrs > i) {
> +            addrs[i] = s->plat_region_addrs[i];
> +        } else {
> +            addrs[i] = PLATFORM_DYNAMIC;
> +        }
> +    }
> +
> +    s->plat_region_addrs = addrs;
> +    s->num_plat_region_addrs = s->num_regions;
> +}
> +
> +static void fixup_irqs(PlatformDeviceState *s)
> +{
> +    uint32_t *irqs = g_new(uint32_t, s->num_irqs);
> +    int i;
> +
> +    /* Treat IRQs that the user did not specify as dynamic */
> +    for (i = 0; i < s->num_irqs; i++) {
> +        if (s->num_plat_irqs > i) {
> +            irqs[i] = s->plat_irqs[i];
> +        } else {
> +            irqs[i] = PLATFORM_DYNAMIC;
> +        }
> +    }
> +
> +    s->plat_irqs = irqs;
> +    s->num_plat_irqs = s->num_irqs;
> +}
> +
> +static void platform_device_realize(DeviceState *dev, Error **errp)
> +{
> +    PlatformDeviceState *s = PLATFORM_DEVICE(dev);
> +
> +    fixup_regions(s);
> +    fixup_irqs(s);
> +}
> +
> +static Property platform_device_properties[] = {
> +    /* memory regions for a device */
> +    DEFINE_PROP_ARRAY("regions", PlatformDeviceState, num_plat_region_addrs,
> +                      plat_region_addrs, qdev_prop_uint64, uint64_t),
> +    /* interrupts for a device */
> +    DEFINE_PROP_ARRAY("irqs", PlatformDeviceState, num_plat_irqs,
> +                      plat_irqs, qdev_prop_uint32, uint32_t),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void platform_device_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +    dc->realize = platform_device_realize;
> +    dc->props = platform_device_properties;
> +}
> +
> +static const TypeInfo platform_device_type_info = {
> +    .name = TYPE_PLATFORM_DEVICE,
> +    .parent = TYPE_DEVICE,
> +    .instance_size = sizeof(PlatformDeviceState),
> +    .abstract = true,
> +    .class_init = platform_device_class_init,
> +};
> +
> +
> +static void device_register_types(void)
> +{
> +    type_register_static(&platform_device_type_info);
> +}
> +
> +type_init(device_register_types)
> diff --git a/include/hw/platform/device.h b/include/hw/platform/device.h
> new file mode 100644
> index 0000000..a17f79e
> --- /dev/null
> +++ b/include/hw/platform/device.h
> @@ -0,0 +1,45 @@
> +/*
> + * Generic platform device with IRQ and IO placement hints
> + *
> + * Copyright 2014 Freescale Semiconductor, Inc.
> + *
> + * Authors: Alexander Graf,   <agraf@suse.de>
> + *
> + * This is free software; you can redistribute it and/or modify
> + * it under the terms of  the GNU General  Public License as published by
> + * the Free Software Foundation;  either version 2 of the  License, or
> + * (at your option) any later version.
> + */
> +
> +#ifndef QEMU_HW_PLATFORM_DEVICE_H
> +#define QEMU_HW_PLATFORM_DEVICE_H
> +
> +#include "qemu-common.h"
> +#include "hw/qdev.h"
> +
> +#define TYPE_PLATFORM_DEVICE "platform-device"
> +#define PLATFORM_DEVICE(obj) OBJECT_CHECK(PlatformDeviceState, (obj), TYPE_PLATFORM_DEVICE)
> +#define PLATFORM_DYNAMIC -1LL
> +
> +typedef struct PlatformDeviceState {
> +    /*< private >*/
> +
> +    DeviceState parent_obj;
> +    QTAILQ_ENTRY(PlatformDeviceState) next;
> +
> +    /* these get set by children to indicate their resources */
> +    uint32_t num_regions;
> +    MemoryRegion **regions;
> +    uint32_t num_irqs;
> +    qemu_irq **irqs;
> +
> +    /*< public >*/
> +
> +    /* these get set by the user though qdev parameters */
> +    uint32_t num_plat_region_addrs;
> +    uint64_t *plat_region_addrs;
> +    uint32_t num_plat_irqs;
> +    uint32_t *plat_irqs;
> +} PlatformDeviceState;
> +
> +#endif /* !QEMU_HW_PLATFORM_DEVICE_H */
> 

[Qemu-devel] [PATCH 2/5] Platform: Add serial device

[Alexander Graf]

The serial port is a simple device that users (read: libvirt) are happy to
create using -device. Unfortunately that only works for ISA and PCI devices,
since sysbus devices can not get spawned using -device without intimate
knowledge of the address layout and interrupt mapping topoligy of the board.

This patch introduces a new platform serial port device that exposes its
interrupt line and memory region via platform device properties. That allows
us to spawn such a device to default locations by simply saying -device.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/char/Makefile.objs             |   1 +
 hw/char/serial-platform.c         | 103 ++++++++++++++++++++++++++++++++++++++
 include/hw/char/serial-platform.h |  56 +++++++++++++++++++++
 3 files changed, 160 insertions(+)
 create mode 100644 hw/char/serial-platform.c
 create mode 100644 include/hw/char/serial-platform.h

diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs
index 317385d..ffb49e5 100644
--- a/hw/char/Makefile.objs
+++ b/hw/char/Makefile.objs
@@ -3,6 +3,7 @@ common-obj-$(CONFIG_ESCC) += escc.o
 common-obj-$(CONFIG_PARALLEL) += parallel.o
 common-obj-$(CONFIG_PL011) += pl011.o
 common-obj-$(CONFIG_SERIAL) += serial.o serial-isa.o
+common-obj-$(CONFIG_SERIAL_PLATFORM) += serial-platform.o
 common-obj-$(CONFIG_SERIAL_PCI) += serial-pci.o
 common-obj-$(CONFIG_VIRTIO) += virtio-console.o
 common-obj-$(CONFIG_XILINX) += xilinx_uartlite.o
diff --git a/hw/char/serial-platform.c b/hw/char/serial-platform.c
new file mode 100644
index 0000000..9edee93
--- /dev/null
+++ b/hw/char/serial-platform.c
@@ -0,0 +1,103 @@
+/*
+ * QEMU 16550A UART emulation
+ *
+ * Copyright (c) 2003-2004 Fabrice Bellard
+ * Copyright (c) 2008 Citrix Systems, 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 "hw/char/serial.h"
+#include "hw/char/serial-platform.h"
+#include "hw/platform/device.h"
+
+static void serial_platform_realizefn(DeviceState *dev, Error **errp)
+{
+    PlatformSerialClass *psc = PLATFORM_SERIAL_GET_CLASS(dev);
+    PlatformSerialState *p = PLATFORM_SERIAL(dev);
+    SerialState *s = &p->state;
+
+    /* super() */
+    psc->parent_realize(dev, errp);
+
+    /* Initialize serial port */
+    s->baudbase = 115200;
+    serial_realize_core(s, errp);
+}
+
+static void serial_platform_init(Object *obj)
+{
+    PlatformSerialState *p = PLATFORM_SERIAL(obj);
+    SerialState *s = &p->state;
+    PlatformDeviceState *pdev = &p->parent_obj;
+
+    /* Initialize static data */
+    memory_region_init_io(&s->io, OBJECT(p), &serial_io_ops, s, "serial", 8);
+
+    /* Populate parent fields */
+    pdev->num_regions = 1;
+    pdev->regions = g_new(MemoryRegion *, 1);
+    pdev->regions[0] = &s->io;
+    pdev->num_irqs = 1;
+    pdev->irqs = g_new(qemu_irq *, 1);
+    pdev->irqs[0] = &s->irq;
+}
+
+static const VMStateDescription vmstate_platform_serial = {
+    .name = "serial",
+    .version_id = 3,
+    .minimum_version_id = 2,
+    .fields = (VMStateField[]) {
+        VMSTATE_STRUCT(state, PlatformSerialState, 0, vmstate_serial, SerialState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static Property serial_platform_properties[] = {
+    DEFINE_PROP_CHR("chardev",   PlatformSerialState, state.chr),
+    DEFINE_PROP_UINT32("wakeup", PlatformSerialState, state.wakeup, 0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void serial_platform_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+    PlatformSerialClass *psc = PLATFORM_SERIAL_CLASS(oc);
+
+    psc->parent_realize = dc->realize;
+    dc->realize = serial_platform_realizefn;
+    dc->vmsd = &vmstate_platform_serial;
+    dc->props = serial_platform_properties;
+}
+
+static const TypeInfo serial_platform_info = {
+    .name          = TYPE_PLATFORM_SERIAL,
+    .parent        = TYPE_PLATFORM_DEVICE,
+    .instance_size = sizeof(PlatformSerialState),
+    .instance_init = serial_platform_init,
+    .class_init    = serial_platform_class_init,
+    .class_size    = sizeof(PlatformSerialClass),
+};
+
+static void serial_register_types(void)
+{
+    type_register_static(&serial_platform_info);
+}
+
+type_init(serial_register_types)
diff --git a/include/hw/char/serial-platform.h b/include/hw/char/serial-platform.h
new file mode 100644
index 0000000..b3650e7
--- /dev/null
+++ b/include/hw/char/serial-platform.h
@@ -0,0 +1,56 @@
+/*
+ * QEMU 16550A UART emulation
+ *
+ * Copyright (c) 2003-2004 Fabrice Bellard
+ * Copyright (c) 2008 Citrix Systems, 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.
+ */
+#ifndef HW_CHAR_SERIAL_PLATFORM_H
+#define HW_CHAR_SERIAL_PLATFORM_H
+
+#include "hw/char/serial.h"
+#include "hw/platform/device.h"
+
+#define TYPE_PLATFORM_SERIAL "platform-serial"
+#define PLATFORM_SERIAL(obj) OBJECT_CHECK(PlatformSerialState, (obj), \
+                                         TYPE_PLATFORM_SERIAL)
+
+typedef struct PlatformSerialState {
+    /*< private >*/
+    PlatformDeviceState parent_obj;
+    /*< public >*/
+
+    SerialState state;
+} PlatformSerialState;
+
+#define PLATFORM_SERIAL_CLASS(class) \
+    OBJECT_CLASS_CHECK(PlatformSerialClass, (class), TYPE_PLATFORM_SERIAL)
+#define PLATFORM_SERIAL_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(PlatformSerialClass, (obj), TYPE_PLATFORM_SERIAL)
+
+typedef struct PlatformSerialClass {
+    /*< private >*/
+    DeviceClass parent_class;
+    /*< public >*/
+
+    DeviceRealize parent_realize;
+} PlatformSerialClass;
+
+#endif /* !HW_CHAR_SERIAL_PLATFORM_H */
-- 
1.8.1.4

[Qemu-devel] [PATCH 3/5] PPC: e500: Only create dt entries for existing serial ports

[Alexander Graf]

When the user specifies -nodefaults he can tell us that he doesn't want any
serial ports spawned by default. While we do honor that wish, we still create
device tree entries for those non-existent devices.

Make device tree generation depend on whether the device is actually available.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/ppc/e500.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index a973c18..33d54b3 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -316,10 +316,15 @@ static int ppce500_load_device_tree(MachineState *machine,
      * device it finds in the dt as serial output device. And we generate
      * devices in reverse order to the dt.
      */
-    dt_serial_create(fdt, MPC8544_SERIAL1_REGS_OFFSET,
-                     soc, mpic, "serial1", 1, false);
-    dt_serial_create(fdt, MPC8544_SERIAL0_REGS_OFFSET,
-                     soc, mpic, "serial0", 0, true);
+    if (serial_hds[1]) {
+        dt_serial_create(fdt, MPC8544_SERIAL1_REGS_OFFSET,
+                         soc, mpic, 42, "serial1", 1, false);
+    }
+
+    if (serial_hds[0]) {
+        dt_serial_create(fdt, MPC8544_SERIAL0_REGS_OFFSET,
+                         soc, mpic, 42, "serial0", 0, true);
+    }
 
     snprintf(gutil, sizeof(gutil), "%s/global-utilities@%llx", soc,
              MPC8544_UTIL_OFFSET);
-- 
1.8.1.4

[Qemu-devel] [PATCH 4/5] PPC: e500: Support platform devices

[Alexander Graf]

For e500 our approach to supporting platform devices is to create a simple
bus from the guest's point of view within which we map platform devices
dynamically.

We allocate memory regions always within the "platform" hole in address
space and map IRQs to predetermined IRQ lines that are reserved for platform
device usage.

This maps really nicely into device tree logic, so we can just tell the
guest about our virtual simple bus in device tree as well.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 default-configs/ppc-softmmu.mak   |   1 +
 default-configs/ppc64-softmmu.mak |   1 +
 hw/ppc/e500.c                     | 221 ++++++++++++++++++++++++++++++++++++++
 hw/ppc/e500.h                     |   1 +
 hw/ppc/e500plat.c                 |   1 +
 5 files changed, 225 insertions(+)

diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
index 33f8d84..d6ec8b9 100644
--- a/default-configs/ppc-softmmu.mak
+++ b/default-configs/ppc-softmmu.mak
@@ -45,6 +45,7 @@ CONFIG_PREP=y
 CONFIG_MAC=y
 CONFIG_E500=y
 CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
+CONFIG_PLATFORM=y
 # For PReP
 CONFIG_MC146818RTC=y
 CONFIG_ETSEC=y
diff --git a/default-configs/ppc64-softmmu.mak b/default-configs/ppc64-softmmu.mak
index 37a15b7..06677bf 100644
--- a/default-configs/ppc64-softmmu.mak
+++ b/default-configs/ppc64-softmmu.mak
@@ -45,6 +45,7 @@ CONFIG_PSERIES=y
 CONFIG_PREP=y
 CONFIG_MAC=y
 CONFIG_E500=y
+CONFIG_PLATFORM=y
 CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
 # For pSeries
 CONFIG_XICS=$(CONFIG_PSERIES)
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 33d54b3..bc26215 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -36,6 +36,7 @@
 #include "exec/address-spaces.h"
 #include "qemu/host-utils.h"
 #include "hw/pci-host/ppce500.h"
+#include "hw/platform/device.h"
 
 #define EPAPR_MAGIC                (0x45504150)
 #define BINARY_DEVICE_TREE_FILE    "mpc8544ds.dtb"
@@ -47,6 +48,14 @@
 
 #define RAM_SIZES_ALIGN            (64UL << 20)
 
+#define E500_PLATFORM_BASE         0xF0000000ULL
+#define E500_PLATFORM_HOLE         (128ULL * 1024 * 1024) /* 128 MB */
+#define E500_PLATFORM_PAGE_SHIFT   12
+#define E500_PLATFORM_HOLE_PAGES   (E500_PLATFORM_HOLE >> \
+                                    E500_PLATFORM_PAGE_SHIFT)
+#define E500_PLATFORM_FIRST_IRQ    5
+#define E500_PLATFORM_NUM_IRQS     10
+
 /* TODO: parameterize */
 #define MPC8544_CCSRBAR_BASE       0xE0000000ULL
 #define MPC8544_CCSRBAR_SIZE       0x00100000ULL
@@ -122,6 +131,62 @@ static void dt_serial_create(void *fdt, unsigned long long offset,
     }
 }
 
+typedef struct PlatformDevtreeData {
+    void *fdt;
+    const char *mpic;
+    int irq_start;
+    const char *node;
+} PlatformDevtreeData;
+
+static int platform_device_create_devtree(Object *obj, void *opaque)
+{
+    PlatformDevtreeData *data = opaque;
+    Object *dev;
+    PlatformDeviceState *pdev;
+
+    dev = object_dynamic_cast(obj, TYPE_PLATFORM_DEVICE);
+    pdev = (PlatformDeviceState *)dev;
+
+    if (!pdev) {
+        /* Container, traverse it for children */
+        return object_child_foreach(obj, platform_device_create_devtree, data);
+    }
+
+    return 0;
+}
+
+static void platform_create_devtree(void *fdt, const char *node, uint64_t addr,
+                                    const char *mpic, int irq_start,
+                                    int nr_irqs)
+{
+    const char platcomp[] = "qemu,platform\0simple-bus";
+    PlatformDevtreeData data;
+
+    /* Create a /platform node that we can put all devices into */
+
+    qemu_fdt_add_subnode(fdt, node);
+    qemu_fdt_setprop(fdt, node, "compatible", platcomp, sizeof(platcomp));
+    qemu_fdt_setprop_string(fdt, node, "device_type", "platform");
+
+    /* Our platform hole is less than 32bit big, so 1 cell is enough for address
+       and size */
+    qemu_fdt_setprop_cells(fdt, node, "#size-cells", 1);
+    qemu_fdt_setprop_cells(fdt, node, "#address-cells", 1);
+    qemu_fdt_setprop_cells(fdt, node, "ranges", 0, addr >> 32, addr,
+                           E500_PLATFORM_HOLE);
+
+    qemu_fdt_setprop_phandle(fdt, node, "interrupt-parent", mpic);
+
+    /* Loop through all devices and create nodes for known ones */
+
+    data.fdt = fdt;
+    data.mpic = mpic;
+    data.irq_start = irq_start;
+    data.node = node;
+
+    platform_device_create_devtree(qdev_get_machine(), &data);
+}
+
 static int ppce500_load_device_tree(MachineState *machine,
                                     PPCE500Params *params,
                                     hwaddr addr,
@@ -379,6 +444,12 @@ static int ppce500_load_device_tree(MachineState *machine,
     qemu_fdt_setprop_cell(fdt, pci, "#address-cells", 3);
     qemu_fdt_setprop_string(fdt, "/aliases", "pci0", pci);
 
+    if (params->has_platform) {
+        platform_create_devtree(fdt, "/platform", E500_PLATFORM_BASE,
+                               mpic, E500_PLATFORM_FIRST_IRQ,
+                               E500_PLATFORM_NUM_IRQS);
+    }
+
     params->fixup_devtree(params, fdt);
 
     if (toplevel_compat) {
@@ -618,6 +689,147 @@ static qemu_irq *ppce500_init_mpic(PPCE500Params *params, MemoryRegion *ccsr,
     return mpic;
 }
 
+typedef struct PlatformNotifier {
+    Notifier notifier;
+    MemoryRegion *address_space_mem;
+    qemu_irq *mpic;
+} PlatformNotifier;
+
+typedef struct PlatformInitData {
+    unsigned long *used_irqs;
+    unsigned long *used_mem;
+    MemoryRegion *mem;
+    qemu_irq *irqs;
+    int device_count;
+} PlatformInitData;
+
+static int platform_map_irq(unsigned long *used_irqs, qemu_irq *platform_irqs,
+                            uint32_t *device_irqn, qemu_irq *device_irq)
+{
+    int max_irqs = E500_PLATFORM_NUM_IRQS;
+    int irqn = *device_irqn;
+
+    if (irqn == (uint32_t)PLATFORM_DYNAMIC) {
+        /* Find the first available IRQ */
+        irqn = find_first_zero_bit(used_irqs, max_irqs);
+    }
+
+    if ((irqn >= max_irqs) || test_and_set_bit(irqn, used_irqs)) {
+        hw_error("e500: IRQ %d is already allocated or no free IRQ left", irqn);
+    }
+
+    *device_irq = platform_irqs[irqn];
+    *device_irqn = irqn;
+
+    return 0;
+}
+
+static int platform_map_region(unsigned long *used_mem, MemoryRegion *pmem,
+                               uint64_t *device_addr, MemoryRegion *device_mem)
+{
+    uint64_t size = memory_region_size(device_mem);
+    uint64_t page_size = (1 << E500_PLATFORM_PAGE_SHIFT);
+    uint64_t page_mask = page_size - 1;
+    uint64_t size_pages = (size + page_mask) >> E500_PLATFORM_PAGE_SHIFT;
+    hwaddr addr = *device_addr;
+    int page;
+    int i;
+
+    page = addr >> E500_PLATFORM_PAGE_SHIFT;
+    if (addr == (uint64_t)PLATFORM_DYNAMIC) {
+        uint64_t size_pages_align;
+
+        /* Align the region to at least its own size granularity */
+        if (is_power_of_2(size_pages)) {
+            size_pages_align = size_pages;
+        } else {
+            size_pages_align = pow2floor(size_pages) << 1;
+        }
+
+        /* Find the first available region that fits */
+        page = bitmap_find_next_zero_area(used_mem, E500_PLATFORM_HOLE_PAGES, 0,
+                                          size_pages, size_pages_align);
+
+        addr = (uint64_t)page << E500_PLATFORM_PAGE_SHIFT;
+    }
+
+    if (page >= E500_PLATFORM_HOLE_PAGES || test_bit(page, used_mem) ||
+        (find_next_bit(used_mem, E500_PLATFORM_HOLE_PAGES, page) < size_pages)) {
+        hw_error("e500: Memory [%"PRIx64":%"PRIx64" is already allocated or "
+                 "no slot left", addr, size);
+    }
+
+    for (i = page; i < (page + size_pages); i++) {
+        set_bit(i, used_mem);
+    }
+
+    memory_region_add_subregion(pmem, addr, device_mem);
+    *device_addr = addr;
+
+    return 0;
+}
+
+static int platform_device_check(Object *obj, void *opaque)
+{
+    PlatformInitData *init = opaque;
+    Object *dev;
+    PlatformDeviceState *pdev;
+    int i;
+
+    dev = object_dynamic_cast(obj, TYPE_PLATFORM_DEVICE);
+    pdev = (PlatformDeviceState *)dev;
+
+    if (!pdev) {
+        /* Container, traverse it for children */
+        return object_child_foreach(obj, platform_device_check, opaque);
+    }
+
+    /* Connect platform device to machine */
+    for (i = 0; i < pdev->num_irqs; i++) {
+        platform_map_irq(init->used_irqs, init->irqs, &pdev->plat_irqs[i],
+                         pdev->irqs[i]);
+    }
+
+    for (i = 0; i < pdev->num_regions; i++) {
+        platform_map_region(init->used_mem, init->mem,
+                            &pdev->plat_region_addrs[i], pdev->regions[i]);
+    }
+
+    return 0;
+}
+
+static void platform_devices_init(MemoryRegion *address_space_mem,
+                                  qemu_irq *mpic)
+{
+    DECLARE_BITMAP(used_irqs, E500_PLATFORM_NUM_IRQS);
+    DECLARE_BITMAP(used_mem, E500_PLATFORM_HOLE_PAGES);
+    MemoryRegion *platform_region = g_new(MemoryRegion, 1);
+    PlatformInitData init = {
+        .used_irqs = used_irqs,
+        .used_mem = used_mem,
+        .mem = platform_region,
+        .irqs = &mpic[E500_PLATFORM_FIRST_IRQ],
+    };
+
+    memory_region_init(platform_region, NULL, "platform devices",
+                       E500_PLATFORM_HOLE);
+
+    bitmap_clear(used_irqs, 0, E500_PLATFORM_NUM_IRQS);
+    bitmap_clear(used_mem, 0, E500_PLATFORM_HOLE_PAGES);
+
+    /* Loop through our internal device tree and attach any dangling device */
+    platform_device_check(qdev_get_machine(), &init);
+
+    memory_region_add_subregion(address_space_mem, E500_PLATFORM_BASE,
+                                platform_region);
+}
+
+static void platform_devices_init_notify(Notifier *notifier, void *data)
+{
+    PlatformNotifier *pn = (PlatformNotifier *)notifier;
+    platform_devices_init(pn->address_space_mem, pn->mpic);
+}
+
 void ppce500_init(MachineState *machine, PPCE500Params *params)
 {
     MemoryRegion *address_space_mem = get_system_memory();
@@ -770,6 +982,15 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
         cur_base = (32 * 1024 * 1024);
     }
 
+    /* Platform Devices */
+    if (params->has_platform) {
+        PlatformNotifier *notifier = g_new(PlatformNotifier, 1);
+        notifier->notifier.notify = platform_devices_init_notify;
+        notifier->address_space_mem = address_space_mem;
+        notifier->mpic = mpic;
+        qemu_add_machine_init_done_notifier(&notifier->notifier);
+    }
+
     /* Load kernel. */
     if (machine->kernel_filename) {
         kernel_base = cur_base;
diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
index 08b25fa..3a588ed 100644
--- a/hw/ppc/e500.h
+++ b/hw/ppc/e500.h
@@ -11,6 +11,7 @@ typedef struct PPCE500Params {
     void (*fixup_devtree)(struct PPCE500Params *params, void *fdt);
 
     int mpic_version;
+    bool has_platform;
 } PPCE500Params;
 
 void ppce500_init(MachineState *machine, PPCE500Params *params);
diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c
index 27df31d..bb84283 100644
--- a/hw/ppc/e500plat.c
+++ b/hw/ppc/e500plat.c
@@ -35,6 +35,7 @@ static void e500plat_init(MachineState *machine)
         .pci_nr_slots = PCI_SLOT_MAX - 1,
         .fixup_devtree = e500plat_fixup_devtree,
         .mpic_version = OPENPIC_MODEL_FSL_MPIC_42,
+        .has_platform = true,
     };
 
     /* Older KVM versions don't support EPR which breaks guests when we announce
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 4/5] PPC: e500: Support platform devices

[Bharat.Bhushan]

> -----Original Message-----
> From: qemu-devel-bounces+bharat.bhushan=freescale.com@nongnu.org [mailto:qemu-
> devel-bounces+bharat.bhushan=freescale.com@nongnu.org] On Behalf Of Alexander
> Graf
> Sent: Wednesday, June 04, 2014 5:59 PM
> To: qemu-ppc@nongnu.org
> Cc: qemu-devel@nongnu.org; eric.auger@linaro.org
> Subject: [Qemu-devel] [PATCH 4/5] PPC: e500: Support platform devices
> 
> For e500 our approach to supporting platform devices is to create a simple
> bus from the guest's point of view within which we map platform devices
> dynamically.
> 
> We allocate memory regions always within the "platform" hole in address
> space and map IRQs to predetermined IRQ lines that are reserved for platform
> device usage.
> 
> This maps really nicely into device tree logic, so we can just tell the
> guest about our virtual simple bus in device tree as well.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  default-configs/ppc-softmmu.mak   |   1 +
>  default-configs/ppc64-softmmu.mak |   1 +
>  hw/ppc/e500.c                     | 221 ++++++++++++++++++++++++++++++++++++++
>  hw/ppc/e500.h                     |   1 +
>  hw/ppc/e500plat.c                 |   1 +
>  5 files changed, 225 insertions(+)
> 
> diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
> index 33f8d84..d6ec8b9 100644
> --- a/default-configs/ppc-softmmu.mak
> +++ b/default-configs/ppc-softmmu.mak
> @@ -45,6 +45,7 @@ CONFIG_PREP=y
>  CONFIG_MAC=y
>  CONFIG_E500=y
>  CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
> +CONFIG_PLATFORM=y
>  # For PReP
>  CONFIG_MC146818RTC=y
>  CONFIG_ETSEC=y
> diff --git a/default-configs/ppc64-softmmu.mak b/default-configs/ppc64-
> softmmu.mak
> index 37a15b7..06677bf 100644
> --- a/default-configs/ppc64-softmmu.mak
> +++ b/default-configs/ppc64-softmmu.mak
> @@ -45,6 +45,7 @@ CONFIG_PSERIES=y
>  CONFIG_PREP=y
>  CONFIG_MAC=y
>  CONFIG_E500=y
> +CONFIG_PLATFORM=y
>  CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
>  # For pSeries
>  CONFIG_XICS=$(CONFIG_PSERIES)
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 33d54b3..bc26215 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -36,6 +36,7 @@
>  #include "exec/address-spaces.h"
>  #include "qemu/host-utils.h"
>  #include "hw/pci-host/ppce500.h"
> +#include "hw/platform/device.h"
> 
>  #define EPAPR_MAGIC                (0x45504150)
>  #define BINARY_DEVICE_TREE_FILE    "mpc8544ds.dtb"
> @@ -47,6 +48,14 @@
> 
>  #define RAM_SIZES_ALIGN            (64UL << 20)
> 
> +#define E500_PLATFORM_BASE         0xF0000000ULL
> +#define E500_PLATFORM_HOLE         (128ULL * 1024 * 1024) /* 128 MB */
> +#define E500_PLATFORM_PAGE_SHIFT   12
> +#define E500_PLATFORM_HOLE_PAGES   (E500_PLATFORM_HOLE >> \
> +                                    E500_PLATFORM_PAGE_SHIFT)
> +#define E500_PLATFORM_FIRST_IRQ    5

How we come to value "5" ?

How it is ensured that this does not overlap with IRQ numbers taken by PCI devices (with no-msi case)?

> +#define E500_PLATFORM_NUM_IRQS     10
> +
>  /* TODO: parameterize */
>  #define MPC8544_CCSRBAR_BASE       0xE0000000ULL
>  #define MPC8544_CCSRBAR_SIZE       0x00100000ULL
> @@ -122,6 +131,62 @@ static void dt_serial_create(void *fdt, unsigned long long
> offset,
>      }
>  }
> 
> +typedef struct PlatformDevtreeData {
> +    void *fdt;
> +    const char *mpic;
> +    int irq_start;
> +    const char *node;
> +} PlatformDevtreeData;
> +
> +static int platform_device_create_devtree(Object *obj, void *opaque)
> +{
> +    PlatformDevtreeData *data = opaque;
> +    Object *dev;
> +    PlatformDeviceState *pdev;
> +
> +    dev = object_dynamic_cast(obj, TYPE_PLATFORM_DEVICE);
> +    pdev = (PlatformDeviceState *)dev;
> +
> +    if (!pdev) {
> +        /* Container, traverse it for children */
> +        return object_child_foreach(obj, platform_device_create_devtree, data);
> +    }
> +
> +    return 0;
> +}
> +
> +static void platform_create_devtree(void *fdt, const char *node, uint64_t addr,
> +                                    const char *mpic, int irq_start,
> +                                    int nr_irqs)
> +{
> +    const char platcomp[] = "qemu,platform\0simple-bus";
> +    PlatformDevtreeData data;
> +
> +    /* Create a /platform node that we can put all devices into */
> +
> +    qemu_fdt_add_subnode(fdt, node);
> +    qemu_fdt_setprop(fdt, node, "compatible", platcomp, sizeof(platcomp));
> +    qemu_fdt_setprop_string(fdt, node, "device_type", "platform");
> +
> +    /* Our platform hole is less than 32bit big, so 1 cell is enough for
> address
> +       and size */
> +    qemu_fdt_setprop_cells(fdt, node, "#size-cells", 1);
> +    qemu_fdt_setprop_cells(fdt, node, "#address-cells", 1);
> +    qemu_fdt_setprop_cells(fdt, node, "ranges", 0, addr >> 32, addr,
> +                           E500_PLATFORM_HOLE);
> +
> +    qemu_fdt_setprop_phandle(fdt, node, "interrupt-parent", mpic);
> +
> +    /* Loop through all devices and create nodes for known ones */
> +
> +    data.fdt = fdt;
> +    data.mpic = mpic;
> +    data.irq_start = irq_start;
> +    data.node = node;
> +
> +    platform_device_create_devtree(qdev_get_machine(), &data);

So I see dma device with two dm-channel then h/w device tree looks like :
	dma@100300 {
                        #address-cells = <0x1>;
                        #size-cells = <0x1>;
                        compatible = "fsl,elo3-dma";
                        reg = <0x100300 0x4 0x100600 0x4>;
                        ranges = <0x0 0x100100 0x500>;

                        dma-channel@0 {
                                compatible = "fsl,eloplus-dma-channel";
                                reg = <0x0 0x80>;
                                interrupts = <0x1c 0x2 0x0 0x0>;
                        };

                        dma-channel@80 {
                                compatible = "fsl,eloplus-dma-channel";
                                reg = <0x80 0x80>;
                                interrupts = <0x1d 0x2 0x0 0x0>;
                        };

		}


Now for assigning same device to guest, We will unbind dma@ device from kernel and then bind with vfio-platform.

1) What will be my qemu command line for this ? I think this will be like:
	-device vfio-platform,vfio_device="dma@100300",compat="fsl,elo3-dma"

Then how similar device tree will be created? It can happen then some dma have 1 channel while other have more "n" channel. 

That also reminds me that should we update documents for same " docs/qdev-device-use.txt"

> +}
> +
>  static int ppce500_load_device_tree(MachineState *machine,
>                                      PPCE500Params *params,
>                                      hwaddr addr,
> @@ -379,6 +444,12 @@ static int ppce500_load_device_tree(MachineState *machine,
>      qemu_fdt_setprop_cell(fdt, pci, "#address-cells", 3);
>      qemu_fdt_setprop_string(fdt, "/aliases", "pci0", pci);
> 
> +    if (params->has_platform) {
> +        platform_create_devtree(fdt, "/platform", E500_PLATFORM_BASE,
> +                               mpic, E500_PLATFORM_FIRST_IRQ,
> +                               E500_PLATFORM_NUM_IRQS);
> +    }
> +
>      params->fixup_devtree(params, fdt);
> 
>      if (toplevel_compat) {
> @@ -618,6 +689,147 @@ static qemu_irq *ppce500_init_mpic(PPCE500Params *params,
> MemoryRegion *ccsr,
>      return mpic;
>  }
> 
> +typedef struct PlatformNotifier {
> +    Notifier notifier;
> +    MemoryRegion *address_space_mem;
> +    qemu_irq *mpic;
> +} PlatformNotifier;
> +
> +typedef struct PlatformInitData {
> +    unsigned long *used_irqs;
> +    unsigned long *used_mem;
> +    MemoryRegion *mem;
> +    qemu_irq *irqs;
> +    int device_count;
> +} PlatformInitData;
> +
> +static int platform_map_irq(unsigned long *used_irqs, qemu_irq *platform_irqs,
> +                            uint32_t *device_irqn, qemu_irq *device_irq)
> +{
> +    int max_irqs = E500_PLATFORM_NUM_IRQS;
> +    int irqn = *device_irqn;
> +
> +    if (irqn == (uint32_t)PLATFORM_DYNAMIC) {
> +        /* Find the first available IRQ */
> +        irqn = find_first_zero_bit(used_irqs, max_irqs);
> +    }
> +
> +    if ((irqn >= max_irqs) || test_and_set_bit(irqn, used_irqs)) {
> +        hw_error("e500: IRQ %d is already allocated or no free IRQ left",
> irqn);
> +    }
> +
> +    *device_irq = platform_irqs[irqn];
> +    *device_irqn = irqn;
> +
> +    return 0;
> +}
> +
> +static int platform_map_region(unsigned long *used_mem, MemoryRegion *pmem,
> +                               uint64_t *device_addr, MemoryRegion *device_mem)
> +{
> +    uint64_t size = memory_region_size(device_mem);
> +    uint64_t page_size = (1 << E500_PLATFORM_PAGE_SHIFT);
> +    uint64_t page_mask = page_size - 1;
> +    uint64_t size_pages = (size + page_mask) >> E500_PLATFORM_PAGE_SHIFT;
> +    hwaddr addr = *device_addr;
> +    int page;
> +    int i;
> +
> +    page = addr >> E500_PLATFORM_PAGE_SHIFT;
> +    if (addr == (uint64_t)PLATFORM_DYNAMIC) {
> +        uint64_t size_pages_align;
> +
> +        /* Align the region to at least its own size granularity */
> +        if (is_power_of_2(size_pages)) {
> +            size_pages_align = size_pages;
> +        } else {
> +            size_pages_align = pow2floor(size_pages) << 1;
> +        }
> +
> +        /* Find the first available region that fits */
> +        page = bitmap_find_next_zero_area(used_mem, E500_PLATFORM_HOLE_PAGES,
> 0,
> +                                          size_pages, size_pages_align);
> +
> +        addr = (uint64_t)page << E500_PLATFORM_PAGE_SHIFT;
> +    }
> +
> +    if (page >= E500_PLATFORM_HOLE_PAGES || test_bit(page, used_mem) ||
> +        (find_next_bit(used_mem, E500_PLATFORM_HOLE_PAGES, page) < size_pages))
> {
> +        hw_error("e500: Memory [%"PRIx64":%"PRIx64" is already allocated or "
> +                 "no slot left", addr, size);
> +    }
> +
> +    for (i = page; i < (page + size_pages); i++) {
> +        set_bit(i, used_mem);
> +    }
> +
> +    memory_region_add_subregion(pmem, addr, device_mem);
> +    *device_addr = addr;
> +
> +    return 0;
> +}
> +
> +static int platform_device_check(Object *obj, void *opaque)
> +{
> +    PlatformInitData *init = opaque;
> +    Object *dev;
> +    PlatformDeviceState *pdev;
> +    int i;
> +
> +    dev = object_dynamic_cast(obj, TYPE_PLATFORM_DEVICE);
> +    pdev = (PlatformDeviceState *)dev;
> +
> +    if (!pdev) {
> +        /* Container, traverse it for children */
> +        return object_child_foreach(obj, platform_device_check, opaque);
> +    }
> +
> +    /* Connect platform device to machine */
> +    for (i = 0; i < pdev->num_irqs; i++) {
> +        platform_map_irq(init->used_irqs, init->irqs, &pdev->plat_irqs[i],
> +                         pdev->irqs[i]);
> +    }
> +
> +    for (i = 0; i < pdev->num_regions; i++) {
> +        platform_map_region(init->used_mem, init->mem,
> +                            &pdev->plat_region_addrs[i], pdev->regions[i]);
> +    }
> +
> +    return 0;
> +}
> +
> +static void platform_devices_init(MemoryRegion *address_space_mem,
> +                                  qemu_irq *mpic)
> +{
> +    DECLARE_BITMAP(used_irqs, E500_PLATFORM_NUM_IRQS);
> +    DECLARE_BITMAP(used_mem, E500_PLATFORM_HOLE_PAGES);
> +    MemoryRegion *platform_region = g_new(MemoryRegion, 1);
> +    PlatformInitData init = {
> +        .used_irqs = used_irqs,
> +        .used_mem = used_mem,
> +        .mem = platform_region,
> +        .irqs = &mpic[E500_PLATFORM_FIRST_IRQ],
> +    };
> +
> +    memory_region_init(platform_region, NULL, "platform devices",
> +                       E500_PLATFORM_HOLE);
> +
> +    bitmap_clear(used_irqs, 0, E500_PLATFORM_NUM_IRQS);
> +    bitmap_clear(used_mem, 0, E500_PLATFORM_HOLE_PAGES);
> +
> +    /* Loop through our internal device tree and attach any dangling device */
> +    platform_device_check(qdev_get_machine(), &init);

Can you please describe what we are trying to do here, what could be a "dangling device" ?

Thanks
-Bharat

> +
> +    memory_region_add_subregion(address_space_mem, E500_PLATFORM_BASE,
> +                                platform_region);
> +}
> +
> +static void platform_devices_init_notify(Notifier *notifier, void *data)
> +{
> +    PlatformNotifier *pn = (PlatformNotifier *)notifier;
> +    platform_devices_init(pn->address_space_mem, pn->mpic);
> +}
> +
>  void ppce500_init(MachineState *machine, PPCE500Params *params)
>  {
>      MemoryRegion *address_space_mem = get_system_memory();
> @@ -770,6 +982,15 @@ void ppce500_init(MachineState *machine, PPCE500Params
> *params)
>          cur_base = (32 * 1024 * 1024);
>      }
> 
> +    /* Platform Devices */
> +    if (params->has_platform) {
> +        PlatformNotifier *notifier = g_new(PlatformNotifier, 1);
> +        notifier->notifier.notify = platform_devices_init_notify;
> +        notifier->address_space_mem = address_space_mem;
> +        notifier->mpic = mpic;
> +        qemu_add_machine_init_done_notifier(&notifier->notifier);
> +    }
> +
>      /* Load kernel. */
>      if (machine->kernel_filename) {
>          kernel_base = cur_base;
> diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
> index 08b25fa..3a588ed 100644
> --- a/hw/ppc/e500.h
> +++ b/hw/ppc/e500.h
> @@ -11,6 +11,7 @@ typedef struct PPCE500Params {
>      void (*fixup_devtree)(struct PPCE500Params *params, void *fdt);
> 
>      int mpic_version;
> +    bool has_platform;
>  } PPCE500Params;
> 
>  void ppce500_init(MachineState *machine, PPCE500Params *params);
> diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c
> index 27df31d..bb84283 100644
> --- a/hw/ppc/e500plat.c
> +++ b/hw/ppc/e500plat.c
> @@ -35,6 +35,7 @@ static void e500plat_init(MachineState *machine)
>          .pci_nr_slots = PCI_SLOT_MAX - 1,
>          .fixup_devtree = e500plat_fixup_devtree,
>          .mpic_version = OPENPIC_MODEL_FSL_MPIC_42,
> +        .has_platform = true,
>      };
> 
>      /* Older KVM versions don't support EPR which breaks guests when we
> announce
> --
> 1.8.1.4
> 

Re: [Qemu-devel] [PATCH 4/5] PPC: e500: Support platform devices

[Alexander Graf]

On 13.06.14 10:58, Bharat.Bhushan@freescale.com wrote:
>
>> -----Original Message-----
>> From: qemu-devel-bounces+bharat.bhushan=freescale.com@nongnu.org [mailto:qemu-
>> devel-bounces+bharat.bhushan=freescale.com@nongnu.org] On Behalf Of Alexander
>> Graf
>> Sent: Wednesday, June 04, 2014 5:59 PM
>> To: qemu-ppc@nongnu.org
>> Cc: qemu-devel@nongnu.org; eric.auger@linaro.org
>> Subject: [Qemu-devel] [PATCH 4/5] PPC: e500: Support platform devices
>>
>> For e500 our approach to supporting platform devices is to create a simple
>> bus from the guest's point of view within which we map platform devices
>> dynamically.
>>
>> We allocate memory regions always within the "platform" hole in address
>> space and map IRQs to predetermined IRQ lines that are reserved for platform
>> device usage.
>>
>> This maps really nicely into device tree logic, so we can just tell the
>> guest about our virtual simple bus in device tree as well.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>>   default-configs/ppc-softmmu.mak   |   1 +
>>   default-configs/ppc64-softmmu.mak |   1 +
>>   hw/ppc/e500.c                     | 221 ++++++++++++++++++++++++++++++++++++++
>>   hw/ppc/e500.h                     |   1 +
>>   hw/ppc/e500plat.c                 |   1 +
>>   5 files changed, 225 insertions(+)
>>
>> diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
>> index 33f8d84..d6ec8b9 100644
>> --- a/default-configs/ppc-softmmu.mak
>> +++ b/default-configs/ppc-softmmu.mak
>> @@ -45,6 +45,7 @@ CONFIG_PREP=y
>>   CONFIG_MAC=y
>>   CONFIG_E500=y
>>   CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
>> +CONFIG_PLATFORM=y
>>   # For PReP
>>   CONFIG_MC146818RTC=y
>>   CONFIG_ETSEC=y
>> diff --git a/default-configs/ppc64-softmmu.mak b/default-configs/ppc64-
>> softmmu.mak
>> index 37a15b7..06677bf 100644
>> --- a/default-configs/ppc64-softmmu.mak
>> +++ b/default-configs/ppc64-softmmu.mak
>> @@ -45,6 +45,7 @@ CONFIG_PSERIES=y
>>   CONFIG_PREP=y
>>   CONFIG_MAC=y
>>   CONFIG_E500=y
>> +CONFIG_PLATFORM=y
>>   CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
>>   # For pSeries
>>   CONFIG_XICS=$(CONFIG_PSERIES)
>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>> index 33d54b3..bc26215 100644
>> --- a/hw/ppc/e500.c
>> +++ b/hw/ppc/e500.c
>> @@ -36,6 +36,7 @@
>>   #include "exec/address-spaces.h"
>>   #include "qemu/host-utils.h"
>>   #include "hw/pci-host/ppce500.h"
>> +#include "hw/platform/device.h"
>>
>>   #define EPAPR_MAGIC                (0x45504150)
>>   #define BINARY_DEVICE_TREE_FILE    "mpc8544ds.dtb"
>> @@ -47,6 +48,14 @@
>>
>>   #define RAM_SIZES_ALIGN            (64UL << 20)
>>
>> +#define E500_PLATFORM_BASE         0xF0000000ULL
>> +#define E500_PLATFORM_HOLE         (128ULL * 1024 * 1024) /* 128 MB */
>> +#define E500_PLATFORM_PAGE_SHIFT   12
>> +#define E500_PLATFORM_HOLE_PAGES   (E500_PLATFORM_HOLE >> \
>> +                                    E500_PLATFORM_PAGE_SHIFT)
>> +#define E500_PLATFORM_FIRST_IRQ    5
> How we come to value "5" ?
>
> How it is ensured that this does not overlap with IRQ numbers taken by PCI devices (with no-msi case)?

UART = 42
PCI = 1, 2, 3, 4

So I figured 5 is right behind the PCI IRQs, but before the UART ;)

>
>> +#define E500_PLATFORM_NUM_IRQS     10
>> +
>>   /* TODO: parameterize */
>>   #define MPC8544_CCSRBAR_BASE       0xE0000000ULL
>>   #define MPC8544_CCSRBAR_SIZE       0x00100000ULL
>> @@ -122,6 +131,62 @@ static void dt_serial_create(void *fdt, unsigned long long
>> offset,
>>       }
>>   }
>>
>> +typedef struct PlatformDevtreeData {
>> +    void *fdt;
>> +    const char *mpic;
>> +    int irq_start;
>> +    const char *node;
>> +} PlatformDevtreeData;
>> +
>> +static int platform_device_create_devtree(Object *obj, void *opaque)
>> +{
>> +    PlatformDevtreeData *data = opaque;
>> +    Object *dev;
>> +    PlatformDeviceState *pdev;
>> +
>> +    dev = object_dynamic_cast(obj, TYPE_PLATFORM_DEVICE);
>> +    pdev = (PlatformDeviceState *)dev;
>> +
>> +    if (!pdev) {
>> +        /* Container, traverse it for children */
>> +        return object_child_foreach(obj, platform_device_create_devtree, data);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static void platform_create_devtree(void *fdt, const char *node, uint64_t addr,
>> +                                    const char *mpic, int irq_start,
>> +                                    int nr_irqs)
>> +{
>> +    const char platcomp[] = "qemu,platform\0simple-bus";
>> +    PlatformDevtreeData data;
>> +
>> +    /* Create a /platform node that we can put all devices into */
>> +
>> +    qemu_fdt_add_subnode(fdt, node);
>> +    qemu_fdt_setprop(fdt, node, "compatible", platcomp, sizeof(platcomp));
>> +    qemu_fdt_setprop_string(fdt, node, "device_type", "platform");
>> +
>> +    /* Our platform hole is less than 32bit big, so 1 cell is enough for
>> address
>> +       and size */
>> +    qemu_fdt_setprop_cells(fdt, node, "#size-cells", 1);
>> +    qemu_fdt_setprop_cells(fdt, node, "#address-cells", 1);
>> +    qemu_fdt_setprop_cells(fdt, node, "ranges", 0, addr >> 32, addr,
>> +                           E500_PLATFORM_HOLE);
>> +
>> +    qemu_fdt_setprop_phandle(fdt, node, "interrupt-parent", mpic);
>> +
>> +    /* Loop through all devices and create nodes for known ones */
>> +
>> +    data.fdt = fdt;
>> +    data.mpic = mpic;
>> +    data.irq_start = irq_start;
>> +    data.node = node;
>> +
>> +    platform_device_create_devtree(qdev_get_machine(), &data);
> So I see dma device with two dm-channel then h/w device tree looks like :
> 	dma@100300 {
>                          #address-cells = <0x1>;
>                          #size-cells = <0x1>;
>                          compatible = "fsl,elo3-dma";
>                          reg = <0x100300 0x4 0x100600 0x4>;
>                          ranges = <0x0 0x100100 0x500>;
>
>                          dma-channel@0 {
>                                  compatible = "fsl,eloplus-dma-channel";
>                                  reg = <0x0 0x80>;
>                                  interrupts = <0x1c 0x2 0x0 0x0>;
>                          };
>
>                          dma-channel@80 {
>                                  compatible = "fsl,eloplus-dma-channel";
>                                  reg = <0x80 0x80>;
>                                  interrupts = <0x1d 0x2 0x0 0x0>;
>                          };
>
> 		}
>
>
> Now for assigning same device to guest, We will unbind dma@ device from kernel and then bind with vfio-platform.
>
> 1) What will be my qemu command line for this ? I think this will be like:
> 	-device vfio-platform,vfio_device="dma@100300",compat="fsl,elo3-dma"
>
> Then how similar device tree will be created? It can happen then some dma have 1 channel while other have more "n" channel.

I don't see any point in only forwarding a DMA channel on its own, but 
if you really have to, it would work like

   -device vfio-platform-elo3-dma,vfio=device="dma@100300"

This creates an object of the VFIO_PLATFORM_ELO3_DMA type in QEMU which 
the board file can look up in its device tree generation and create 
device tree entries accordingly.

>
> That also reminds me that should we update documents for same " docs/qdev-device-use.txt"
>
>> +}
>> +
>>   static int ppce500_load_device_tree(MachineState *machine,
>>                                       PPCE500Params *params,
>>                                       hwaddr addr,
>> @@ -379,6 +444,12 @@ static int ppce500_load_device_tree(MachineState *machine,
>>       qemu_fdt_setprop_cell(fdt, pci, "#address-cells", 3);
>>       qemu_fdt_setprop_string(fdt, "/aliases", "pci0", pci);
>>
>> +    if (params->has_platform) {
>> +        platform_create_devtree(fdt, "/platform", E500_PLATFORM_BASE,
>> +                               mpic, E500_PLATFORM_FIRST_IRQ,
>> +                               E500_PLATFORM_NUM_IRQS);
>> +    }
>> +
>>       params->fixup_devtree(params, fdt);
>>
>>       if (toplevel_compat) {
>> @@ -618,6 +689,147 @@ static qemu_irq *ppce500_init_mpic(PPCE500Params *params,
>> MemoryRegion *ccsr,
>>       return mpic;
>>   }
>>
>> +typedef struct PlatformNotifier {
>> +    Notifier notifier;
>> +    MemoryRegion *address_space_mem;
>> +    qemu_irq *mpic;
>> +} PlatformNotifier;
>> +
>> +typedef struct PlatformInitData {
>> +    unsigned long *used_irqs;
>> +    unsigned long *used_mem;
>> +    MemoryRegion *mem;
>> +    qemu_irq *irqs;
>> +    int device_count;
>> +} PlatformInitData;
>> +
>> +static int platform_map_irq(unsigned long *used_irqs, qemu_irq *platform_irqs,
>> +                            uint32_t *device_irqn, qemu_irq *device_irq)
>> +{
>> +    int max_irqs = E500_PLATFORM_NUM_IRQS;
>> +    int irqn = *device_irqn;
>> +
>> +    if (irqn == (uint32_t)PLATFORM_DYNAMIC) {
>> +        /* Find the first available IRQ */
>> +        irqn = find_first_zero_bit(used_irqs, max_irqs);
>> +    }
>> +
>> +    if ((irqn >= max_irqs) || test_and_set_bit(irqn, used_irqs)) {
>> +        hw_error("e500: IRQ %d is already allocated or no free IRQ left",
>> irqn);
>> +    }
>> +
>> +    *device_irq = platform_irqs[irqn];
>> +    *device_irqn = irqn;
>> +
>> +    return 0;
>> +}
>> +
>> +static int platform_map_region(unsigned long *used_mem, MemoryRegion *pmem,
>> +                               uint64_t *device_addr, MemoryRegion *device_mem)
>> +{
>> +    uint64_t size = memory_region_size(device_mem);
>> +    uint64_t page_size = (1 << E500_PLATFORM_PAGE_SHIFT);
>> +    uint64_t page_mask = page_size - 1;
>> +    uint64_t size_pages = (size + page_mask) >> E500_PLATFORM_PAGE_SHIFT;
>> +    hwaddr addr = *device_addr;
>> +    int page;
>> +    int i;
>> +
>> +    page = addr >> E500_PLATFORM_PAGE_SHIFT;
>> +    if (addr == (uint64_t)PLATFORM_DYNAMIC) {
>> +        uint64_t size_pages_align;
>> +
>> +        /* Align the region to at least its own size granularity */
>> +        if (is_power_of_2(size_pages)) {
>> +            size_pages_align = size_pages;
>> +        } else {
>> +            size_pages_align = pow2floor(size_pages) << 1;
>> +        }
>> +
>> +        /* Find the first available region that fits */
>> +        page = bitmap_find_next_zero_area(used_mem, E500_PLATFORM_HOLE_PAGES,
>> 0,
>> +                                          size_pages, size_pages_align);
>> +
>> +        addr = (uint64_t)page << E500_PLATFORM_PAGE_SHIFT;
>> +    }
>> +
>> +    if (page >= E500_PLATFORM_HOLE_PAGES || test_bit(page, used_mem) ||
>> +        (find_next_bit(used_mem, E500_PLATFORM_HOLE_PAGES, page) < size_pages))
>> {
>> +        hw_error("e500: Memory [%"PRIx64":%"PRIx64" is already allocated or "
>> +                 "no slot left", addr, size);
>> +    }
>> +
>> +    for (i = page; i < (page + size_pages); i++) {
>> +        set_bit(i, used_mem);
>> +    }
>> +
>> +    memory_region_add_subregion(pmem, addr, device_mem);
>> +    *device_addr = addr;
>> +
>> +    return 0;
>> +}
>> +
>> +static int platform_device_check(Object *obj, void *opaque)
>> +{
>> +    PlatformInitData *init = opaque;
>> +    Object *dev;
>> +    PlatformDeviceState *pdev;
>> +    int i;
>> +
>> +    dev = object_dynamic_cast(obj, TYPE_PLATFORM_DEVICE);
>> +    pdev = (PlatformDeviceState *)dev;
>> +
>> +    if (!pdev) {
>> +        /* Container, traverse it for children */
>> +        return object_child_foreach(obj, platform_device_check, opaque);
>> +    }
>> +
>> +    /* Connect platform device to machine */
>> +    for (i = 0; i < pdev->num_irqs; i++) {
>> +        platform_map_irq(init->used_irqs, init->irqs, &pdev->plat_irqs[i],
>> +                         pdev->irqs[i]);
>> +    }
>> +
>> +    for (i = 0; i < pdev->num_regions; i++) {
>> +        platform_map_region(init->used_mem, init->mem,
>> +                            &pdev->plat_region_addrs[i], pdev->regions[i]);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static void platform_devices_init(MemoryRegion *address_space_mem,
>> +                                  qemu_irq *mpic)
>> +{
>> +    DECLARE_BITMAP(used_irqs, E500_PLATFORM_NUM_IRQS);
>> +    DECLARE_BITMAP(used_mem, E500_PLATFORM_HOLE_PAGES);
>> +    MemoryRegion *platform_region = g_new(MemoryRegion, 1);
>> +    PlatformInitData init = {
>> +        .used_irqs = used_irqs,
>> +        .used_mem = used_mem,
>> +        .mem = platform_region,
>> +        .irqs = &mpic[E500_PLATFORM_FIRST_IRQ],
>> +    };
>> +
>> +    memory_region_init(platform_region, NULL, "platform devices",
>> +                       E500_PLATFORM_HOLE);
>> +
>> +    bitmap_clear(used_irqs, 0, E500_PLATFORM_NUM_IRQS);
>> +    bitmap_clear(used_mem, 0, E500_PLATFORM_HOLE_PAGES);
>> +
>> +    /* Loop through our internal device tree and attach any dangling device */
>> +    platform_device_check(qdev_get_machine(), &init);
> Can you please describe what we are trying to do here, what could be a "dangling device" ?

A "dangling device" is a device that's not attached to any bus. Since 
there is no platform bus, all devices of the DEVICE_PLATFORM type are 
"dangling" when they get created.


Alex

Re: [Qemu-devel] [PATCH 4/5] PPC: e500: Support platform devices

[Eric Auger]

On 06/04/2014 02:28 PM, Alexander Graf wrote:
> For e500 our approach to supporting platform devices is to create a simple
> bus from the guest's point of view within which we map platform devices
> dynamically.
> 
> We allocate memory regions always within the "platform" hole in address
> space and map IRQs to predetermined IRQ lines that are reserved for platform
> device usage.
> 
> This maps really nicely into device tree logic, so we can just tell the
> guest about our virtual simple bus in device tree as well.
Hi Alex,

this "qemu_add_machine_init_done_notifier" was the qemu mechanism I
missed in my patch. You light my way ;-)

One first comment is it would make much sense to reuse your code in arm
virt.c too. I am currently doing the exercise. Do you think it would be
possible to share a common helper code, outside of e500 machine code?

Thank you in advance

Best Regards

Eric
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  default-configs/ppc-softmmu.mak   |   1 +
>  default-configs/ppc64-softmmu.mak |   1 +
>  hw/ppc/e500.c                     | 221 ++++++++++++++++++++++++++++++++++++++
>  hw/ppc/e500.h                     |   1 +
>  hw/ppc/e500plat.c                 |   1 +
>  5 files changed, 225 insertions(+)
> 
> diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
> index 33f8d84..d6ec8b9 100644
> --- a/default-configs/ppc-softmmu.mak
> +++ b/default-configs/ppc-softmmu.mak
> @@ -45,6 +45,7 @@ CONFIG_PREP=y
>  CONFIG_MAC=y
>  CONFIG_E500=y
>  CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
> +CONFIG_PLATFORM=y
>  # For PReP
>  CONFIG_MC146818RTC=y
>  CONFIG_ETSEC=y
> diff --git a/default-configs/ppc64-softmmu.mak b/default-configs/ppc64-softmmu.mak
> index 37a15b7..06677bf 100644
> --- a/default-configs/ppc64-softmmu.mak
> +++ b/default-configs/ppc64-softmmu.mak
> @@ -45,6 +45,7 @@ CONFIG_PSERIES=y
>  CONFIG_PREP=y
>  CONFIG_MAC=y
>  CONFIG_E500=y
> +CONFIG_PLATFORM=y
>  CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
>  # For pSeries
>  CONFIG_XICS=$(CONFIG_PSERIES)
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 33d54b3..bc26215 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -36,6 +36,7 @@
>  #include "exec/address-spaces.h"
>  #include "qemu/host-utils.h"
>  #include "hw/pci-host/ppce500.h"
> +#include "hw/platform/device.h"
>  
>  #define EPAPR_MAGIC                (0x45504150)
>  #define BINARY_DEVICE_TREE_FILE    "mpc8544ds.dtb"
> @@ -47,6 +48,14 @@
>  
>  #define RAM_SIZES_ALIGN            (64UL << 20)
>  
> +#define E500_PLATFORM_BASE         0xF0000000ULL
> +#define E500_PLATFORM_HOLE         (128ULL * 1024 * 1024) /* 128 MB */
> +#define E500_PLATFORM_PAGE_SHIFT   12
> +#define E500_PLATFORM_HOLE_PAGES   (E500_PLATFORM_HOLE >> \
> +                                    E500_PLATFORM_PAGE_SHIFT)
> +#define E500_PLATFORM_FIRST_IRQ    5
> +#define E500_PLATFORM_NUM_IRQS     10
> +
>  /* TODO: parameterize */
>  #define MPC8544_CCSRBAR_BASE       0xE0000000ULL
>  #define MPC8544_CCSRBAR_SIZE       0x00100000ULL
> @@ -122,6 +131,62 @@ static void dt_serial_create(void *fdt, unsigned long long offset,
>      }
>  }
>  
> +typedef struct PlatformDevtreeData {
> +    void *fdt;
> +    const char *mpic;
> +    int irq_start;
> +    const char *node;
> +} PlatformDevtreeData;
> +
> +static int platform_device_create_devtree(Object *obj, void *opaque)
> +{
> +    PlatformDevtreeData *data = opaque;
> +    Object *dev;
> +    PlatformDeviceState *pdev;
> +
> +    dev = object_dynamic_cast(obj, TYPE_PLATFORM_DEVICE);
> +    pdev = (PlatformDeviceState *)dev;
> +
> +    if (!pdev) {
> +        /* Container, traverse it for children */
> +        return object_child_foreach(obj, platform_device_create_devtree, data);
> +    }
> +
> +    return 0;
> +}
> +
> +static void platform_create_devtree(void *fdt, const char *node, uint64_t addr,
> +                                    const char *mpic, int irq_start,
> +                                    int nr_irqs)
> +{
> +    const char platcomp[] = "qemu,platform\0simple-bus";
> +    PlatformDevtreeData data;
> +
> +    /* Create a /platform node that we can put all devices into */
> +
> +    qemu_fdt_add_subnode(fdt, node);
> +    qemu_fdt_setprop(fdt, node, "compatible", platcomp, sizeof(platcomp));
> +    qemu_fdt_setprop_string(fdt, node, "device_type", "platform");
> +
> +    /* Our platform hole is less than 32bit big, so 1 cell is enough for address
> +       and size */
> +    qemu_fdt_setprop_cells(fdt, node, "#size-cells", 1);
> +    qemu_fdt_setprop_cells(fdt, node, "#address-cells", 1);
> +    qemu_fdt_setprop_cells(fdt, node, "ranges", 0, addr >> 32, addr,
> +                           E500_PLATFORM_HOLE);
> +
> +    qemu_fdt_setprop_phandle(fdt, node, "interrupt-parent", mpic);
> +
> +    /* Loop through all devices and create nodes for known ones */
> +
> +    data.fdt = fdt;
> +    data.mpic = mpic;
> +    data.irq_start = irq_start;
> +    data.node = node;
> +
> +    platform_device_create_devtree(qdev_get_machine(), &data);
> +}
> +
>  static int ppce500_load_device_tree(MachineState *machine,
>                                      PPCE500Params *params,
>                                      hwaddr addr,
> @@ -379,6 +444,12 @@ static int ppce500_load_device_tree(MachineState *machine,
>      qemu_fdt_setprop_cell(fdt, pci, "#address-cells", 3);
>      qemu_fdt_setprop_string(fdt, "/aliases", "pci0", pci);
>  
> +    if (params->has_platform) {
> +        platform_create_devtree(fdt, "/platform", E500_PLATFORM_BASE,
> +                               mpic, E500_PLATFORM_FIRST_IRQ,
> +                               E500_PLATFORM_NUM_IRQS);
> +    }
> +
>      params->fixup_devtree(params, fdt);
>  
>      if (toplevel_compat) {
> @@ -618,6 +689,147 @@ static qemu_irq *ppce500_init_mpic(PPCE500Params *params, MemoryRegion *ccsr,
>      return mpic;
>  }
>  
> +typedef struct PlatformNotifier {
> +    Notifier notifier;
> +    MemoryRegion *address_space_mem;
> +    qemu_irq *mpic;
> +} PlatformNotifier;
> +
> +typedef struct PlatformInitData {
> +    unsigned long *used_irqs;
> +    unsigned long *used_mem;
> +    MemoryRegion *mem;
> +    qemu_irq *irqs;
> +    int device_count;
> +} PlatformInitData;
> +
> +static int platform_map_irq(unsigned long *used_irqs, qemu_irq *platform_irqs,
> +                            uint32_t *device_irqn, qemu_irq *device_irq)
> +{
> +    int max_irqs = E500_PLATFORM_NUM_IRQS;
> +    int irqn = *device_irqn;
> +
> +    if (irqn == (uint32_t)PLATFORM_DYNAMIC) {
> +        /* Find the first available IRQ */
> +        irqn = find_first_zero_bit(used_irqs, max_irqs);
> +    }
> +
> +    if ((irqn >= max_irqs) || test_and_set_bit(irqn, used_irqs)) {
> +        hw_error("e500: IRQ %d is already allocated or no free IRQ left", irqn);
> +    }
> +
> +    *device_irq = platform_irqs[irqn];
> +    *device_irqn = irqn;
> +
> +    return 0;
> +}
> +
> +static int platform_map_region(unsigned long *used_mem, MemoryRegion *pmem,
> +                               uint64_t *device_addr, MemoryRegion *device_mem)
> +{
> +    uint64_t size = memory_region_size(device_mem);
> +    uint64_t page_size = (1 << E500_PLATFORM_PAGE_SHIFT);
> +    uint64_t page_mask = page_size - 1;
> +    uint64_t size_pages = (size + page_mask) >> E500_PLATFORM_PAGE_SHIFT;
> +    hwaddr addr = *device_addr;
> +    int page;
> +    int i;
> +
> +    page = addr >> E500_PLATFORM_PAGE_SHIFT;
> +    if (addr == (uint64_t)PLATFORM_DYNAMIC) {
> +        uint64_t size_pages_align;
> +
> +        /* Align the region to at least its own size granularity */
> +        if (is_power_of_2(size_pages)) {
> +            size_pages_align = size_pages;
> +        } else {
> +            size_pages_align = pow2floor(size_pages) << 1;
> +        }
> +
> +        /* Find the first available region that fits */
> +        page = bitmap_find_next_zero_area(used_mem, E500_PLATFORM_HOLE_PAGES, 0,
> +                                          size_pages, size_pages_align);
> +
> +        addr = (uint64_t)page << E500_PLATFORM_PAGE_SHIFT;
> +    }
> +
> +    if (page >= E500_PLATFORM_HOLE_PAGES || test_bit(page, used_mem) ||
> +        (find_next_bit(used_mem, E500_PLATFORM_HOLE_PAGES, page) < size_pages)) {
> +        hw_error("e500: Memory [%"PRIx64":%"PRIx64" is already allocated or "
> +                 "no slot left", addr, size);
> +    }
> +
> +    for (i = page; i < (page + size_pages); i++) {
> +        set_bit(i, used_mem);
> +    }
> +
> +    memory_region_add_subregion(pmem, addr, device_mem);
> +    *device_addr = addr;
> +
> +    return 0;
> +}
> +
> +static int platform_device_check(Object *obj, void *opaque)
> +{
> +    PlatformInitData *init = opaque;
> +    Object *dev;
> +    PlatformDeviceState *pdev;
> +    int i;
> +
> +    dev = object_dynamic_cast(obj, TYPE_PLATFORM_DEVICE);
> +    pdev = (PlatformDeviceState *)dev;
> +
> +    if (!pdev) {
> +        /* Container, traverse it for children */
> +        return object_child_foreach(obj, platform_device_check, opaque);
> +    }
> +
> +    /* Connect platform device to machine */
> +    for (i = 0; i < pdev->num_irqs; i++) {
> +        platform_map_irq(init->used_irqs, init->irqs, &pdev->plat_irqs[i],
> +                         pdev->irqs[i]);
> +    }
> +
> +    for (i = 0; i < pdev->num_regions; i++) {
> +        platform_map_region(init->used_mem, init->mem,
> +                            &pdev->plat_region_addrs[i], pdev->regions[i]);
> +    }
> +
> +    return 0;
> +}
> +
> +static void platform_devices_init(MemoryRegion *address_space_mem,
> +                                  qemu_irq *mpic)
> +{
> +    DECLARE_BITMAP(used_irqs, E500_PLATFORM_NUM_IRQS);
> +    DECLARE_BITMAP(used_mem, E500_PLATFORM_HOLE_PAGES);
> +    MemoryRegion *platform_region = g_new(MemoryRegion, 1);
> +    PlatformInitData init = {
> +        .used_irqs = used_irqs,
> +        .used_mem = used_mem,
> +        .mem = platform_region,
> +        .irqs = &mpic[E500_PLATFORM_FIRST_IRQ],
> +    };
> +
> +    memory_region_init(platform_region, NULL, "platform devices",
> +                       E500_PLATFORM_HOLE);
> +
> +    bitmap_clear(used_irqs, 0, E500_PLATFORM_NUM_IRQS);
> +    bitmap_clear(used_mem, 0, E500_PLATFORM_HOLE_PAGES);
> +
> +    /* Loop through our internal device tree and attach any dangling device */
> +    platform_device_check(qdev_get_machine(), &init);
> +
> +    memory_region_add_subregion(address_space_mem, E500_PLATFORM_BASE,
> +                                platform_region);
> +}
> +
> +static void platform_devices_init_notify(Notifier *notifier, void *data)
> +{
> +    PlatformNotifier *pn = (PlatformNotifier *)notifier;
> +    platform_devices_init(pn->address_space_mem, pn->mpic);
> +}
> +
>  void ppce500_init(MachineState *machine, PPCE500Params *params)
>  {
>      MemoryRegion *address_space_mem = get_system_memory();
> @@ -770,6 +982,15 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
>          cur_base = (32 * 1024 * 1024);
>      }
>  
> +    /* Platform Devices */
> +    if (params->has_platform) {
> +        PlatformNotifier *notifier = g_new(PlatformNotifier, 1);
> +        notifier->notifier.notify = platform_devices_init_notify;
> +        notifier->address_space_mem = address_space_mem;
> +        notifier->mpic = mpic;
> +        qemu_add_machine_init_done_notifier(&notifier->notifier);
> +    }
> +
>      /* Load kernel. */
>      if (machine->kernel_filename) {
>          kernel_base = cur_base;
> diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
> index 08b25fa..3a588ed 100644
> --- a/hw/ppc/e500.h
> +++ b/hw/ppc/e500.h
> @@ -11,6 +11,7 @@ typedef struct PPCE500Params {
>      void (*fixup_devtree)(struct PPCE500Params *params, void *fdt);
>  
>      int mpic_version;
> +    bool has_platform;
>  } PPCE500Params;
>  
>  void ppce500_init(MachineState *machine, PPCE500Params *params);
> diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c
> index 27df31d..bb84283 100644
> --- a/hw/ppc/e500plat.c
> +++ b/hw/ppc/e500plat.c
> @@ -35,6 +35,7 @@ static void e500plat_init(MachineState *machine)
>          .pci_nr_slots = PCI_SLOT_MAX - 1,
>          .fixup_devtree = e500plat_fixup_devtree,
>          .mpic_version = OPENPIC_MODEL_FSL_MPIC_42,
> +        .has_platform = true,
>      };
>  
>      /* Older KVM versions don't support EPR which breaks guests when we announce
> 

Re: [Qemu-devel] [PATCH 4/5] PPC: e500: Support platform devices

[Alexander Graf]

On 19.06.14 16:56, Eric Auger wrote:
> On 06/04/2014 02:28 PM, Alexander Graf wrote:
>> For e500 our approach to supporting platform devices is to create a simple
>> bus from the guest's point of view within which we map platform devices
>> dynamically.
>>
>> We allocate memory regions always within the "platform" hole in address
>> space and map IRQs to predetermined IRQ lines that are reserved for platform
>> device usage.
>>
>> This maps really nicely into device tree logic, so we can just tell the
>> guest about our virtual simple bus in device tree as well.
> Hi Alex,
>
> this "qemu_add_machine_init_done_notifier" was the qemu mechanism I
> missed in my patch. You light my way ;-)
>
> One first comment is it would make much sense to reuse your code in arm
> virt.c too. I am currently doing the exercise. Do you think it would be
> possible to share a common helper code, outside of e500 machine code?

Yes, definitely :). I was mostly doing it in e500.c as a starting point. 
The second implementation (virt.c apparently) would then go in and 
commonalize everything it thinks should be common.

I'm not 100% sure where the right line is here though. For the 
implementation today I just throw everything into a new virtual platform 
bus for the guest. But there's no reason I couldn't detect "this is the 
first UART in my system" and put it in place where the real first UART 
would be in CCSR.

So yeah, again, I'm not 100% sure where the right line of distinction 
between board specific and generic code is here :).


Alex

Re: [Qemu-devel] [PATCH 4/5] PPC: e500: Support platform devices

[Eric Auger]

On 06/04/2014 02:28 PM, Alexander Graf wrote:
> For e500 our approach to supporting platform devices is to create a simple
> bus from the guest's point of view within which we map platform devices
> dynamically.
> 
> We allocate memory regions always within the "platform" hole in address
> space and map IRQs to predetermined IRQ lines that are reserved for platform
> device usage.
> 
> This maps really nicely into device tree logic, so we can just tell the
> guest about our virtual simple bus in device tree as well.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  default-configs/ppc-softmmu.mak   |   1 +
>  default-configs/ppc64-softmmu.mak |   1 +
>  hw/ppc/e500.c                     | 221 ++++++++++++++++++++++++++++++++++++++
>  hw/ppc/e500.h                     |   1 +
>  hw/ppc/e500plat.c                 |   1 +
>  5 files changed, 225 insertions(+)
> 
> diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
> index 33f8d84..d6ec8b9 100644
> --- a/default-configs/ppc-softmmu.mak
> +++ b/default-configs/ppc-softmmu.mak
> @@ -45,6 +45,7 @@ CONFIG_PREP=y
>  CONFIG_MAC=y
>  CONFIG_E500=y
>  CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
> +CONFIG_PLATFORM=y
>  # For PReP
>  CONFIG_MC146818RTC=y
>  CONFIG_ETSEC=y
> diff --git a/default-configs/ppc64-softmmu.mak b/default-configs/ppc64-softmmu.mak
> index 37a15b7..06677bf 100644
> --- a/default-configs/ppc64-softmmu.mak
> +++ b/default-configs/ppc64-softmmu.mak
> @@ -45,6 +45,7 @@ CONFIG_PSERIES=y
>  CONFIG_PREP=y
>  CONFIG_MAC=y
>  CONFIG_E500=y
> +CONFIG_PLATFORM=y
>  CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
>  # For pSeries
>  CONFIG_XICS=$(CONFIG_PSERIES)
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 33d54b3..bc26215 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -36,6 +36,7 @@
>  #include "exec/address-spaces.h"
>  #include "qemu/host-utils.h"
>  #include "hw/pci-host/ppce500.h"
> +#include "hw/platform/device.h"
>  
>  #define EPAPR_MAGIC                (0x45504150)
>  #define BINARY_DEVICE_TREE_FILE    "mpc8544ds.dtb"
> @@ -47,6 +48,14 @@
>  
>  #define RAM_SIZES_ALIGN            (64UL << 20)
>  
> +#define E500_PLATFORM_BASE         0xF0000000ULL
> +#define E500_PLATFORM_HOLE         (128ULL * 1024 * 1024) /* 128 MB */
> +#define E500_PLATFORM_PAGE_SHIFT   12
> +#define E500_PLATFORM_HOLE_PAGES   (E500_PLATFORM_HOLE >> \
> +                                    E500_PLATFORM_PAGE_SHIFT)
> +#define E500_PLATFORM_FIRST_IRQ    5
> +#define E500_PLATFORM_NUM_IRQS     10
> +
>  /* TODO: parameterize */
>  #define MPC8544_CCSRBAR_BASE       0xE0000000ULL
>  #define MPC8544_CCSRBAR_SIZE       0x00100000ULL
> @@ -122,6 +131,62 @@ static void dt_serial_create(void *fdt, unsigned long long offset,
>      }
>  }
>  
> +typedef struct PlatformDevtreeData {
> +    void *fdt;
> +    const char *mpic;
> +    int irq_start;
> +    const char *node;
> +} PlatformDevtreeData;
> +
> +static int platform_device_create_devtree(Object *obj, void *opaque)
> +{
> +    PlatformDevtreeData *data = opaque;
> +    Object *dev;
> +    PlatformDeviceState *pdev;
> +
> +    dev = object_dynamic_cast(obj, TYPE_PLATFORM_DEVICE);
> +    pdev = (PlatformDeviceState *)dev;
> +
> +    if (!pdev) {
> +        /* Container, traverse it for children */
> +        return object_child_foreach(obj, platform_device_create_devtree, data);
> +    }
> +
> +    return 0;
> +}
> +
> +static void platform_create_devtree(void *fdt, const char *node, uint64_t addr,
> +                                    const char *mpic, int irq_start,
> +                                    int nr_irqs)
> +{
> +    const char platcomp[] = "qemu,platform\0simple-bus";
> +    PlatformDevtreeData data;
> +
> +    /* Create a /platform node that we can put all devices into */
> +
> +    qemu_fdt_add_subnode(fdt, node);
> +    qemu_fdt_setprop(fdt, node, "compatible", platcomp, sizeof(platcomp));
> +    qemu_fdt_setprop_string(fdt, node, "device_type", "platform");
> +
> +    /* Our platform hole is less than 32bit big, so 1 cell is enough for address
> +       and size */
> +    qemu_fdt_setprop_cells(fdt, node, "#size-cells", 1);
> +    qemu_fdt_setprop_cells(fdt, node, "#address-cells", 1);
> +    qemu_fdt_setprop_cells(fdt, node, "ranges", 0, addr >> 32, addr,
> +                           E500_PLATFORM_HOLE);
> +
> +    qemu_fdt_setprop_phandle(fdt, node, "interrupt-parent", mpic);
> +
> +    /* Loop through all devices and create nodes for known ones */
> +
> +    data.fdt = fdt;
> +    data.mpic = mpic;
> +    data.irq_start = irq_start;
> +    data.node = node;
> +
> +    platform_device_create_devtree(qdev_get_machine(), &data);
> +}
> +
>  static int ppce500_load_device_tree(MachineState *machine,
>                                      PPCE500Params *params,
>                                      hwaddr addr,
> @@ -379,6 +444,12 @@ static int ppce500_load_device_tree(MachineState *machine,
>      qemu_fdt_setprop_cell(fdt, pci, "#address-cells", 3);
>      qemu_fdt_setprop_string(fdt, "/aliases", "pci0", pci);
>  
> +    if (params->has_platform) {
> +        platform_create_devtree(fdt, "/platform", E500_PLATFORM_BASE,
> +                               mpic, E500_PLATFORM_FIRST_IRQ,
> +                               E500_PLATFORM_NUM_IRQS);
> +    }
> +
>      params->fixup_devtree(params, fdt);
>  
>      if (toplevel_compat) {
> @@ -618,6 +689,147 @@ static qemu_irq *ppce500_init_mpic(PPCE500Params *params, MemoryRegion *ccsr,
>      return mpic;
>  }
>  
> +typedef struct PlatformNotifier {
> +    Notifier notifier;
> +    MemoryRegion *address_space_mem;
> +    qemu_irq *mpic;
> +} PlatformNotifier;
> +
> +typedef struct PlatformInitData {
> +    unsigned long *used_irqs;
> +    unsigned long *used_mem;
> +    MemoryRegion *mem;
> +    qemu_irq *irqs;
> +    int device_count;
> +} PlatformInitData;
> +
> +static int platform_map_irq(unsigned long *used_irqs, qemu_irq *platform_irqs,
> +                            uint32_t *device_irqn, qemu_irq *device_irq)
> +{
> +    int max_irqs = E500_PLATFORM_NUM_IRQS;
> +    int irqn = *device_irqn;
> +
> +    if (irqn == (uint32_t)PLATFORM_DYNAMIC) {
> +        /* Find the first available IRQ */
> +        irqn = find_first_zero_bit(used_irqs, max_irqs);
> +    }
> +
> +    if ((irqn >= max_irqs) || test_and_set_bit(irqn, used_irqs)) {
> +        hw_error("e500: IRQ %d is already allocated or no free IRQ left", irqn);
> +    }
> +
> +    *device_irq = platform_irqs[irqn];
> +    *device_irqn = irqn;
Hi Alex,

Wouldn' it be more natural to add IRQ_START here, given the semantic of
plat_irqs? Besides, I saw you added it dt_serial_create.
> +
> +    return 0;
> +}
> +
> +static int platform_map_region(unsigned long *used_mem, MemoryRegion *pmem,
> +                               uint64_t *device_addr, MemoryRegion *device_mem)
> +{
> +    uint64_t size = memory_region_size(device_mem);
> +    uint64_t page_size = (1 << E500_PLATFORM_PAGE_SHIFT);
> +    uint64_t page_mask = page_size - 1;
> +    uint64_t size_pages = (size + page_mask) >> E500_PLATFORM_PAGE_SHIFT;
> +    hwaddr addr = *device_addr;
> +    int page;
> +    int i;
> +
> +    page = addr >> E500_PLATFORM_PAGE_SHIFT;
> +    if (addr == (uint64_t)PLATFORM_DYNAMIC) {
> +        uint64_t size_pages_align;
> +
> +        /* Align the region to at least its own size granularity */
> +        if (is_power_of_2(size_pages)) {
> +            size_pages_align = size_pages;
> +        } else {
> +            size_pages_align = pow2floor(size_pages) << 1;
> +        }
> +
> +        /* Find the first available region that fits */
> +        page = bitmap_find_next_zero_area(used_mem, E500_PLATFORM_HOLE_PAGES, 0,
> +                                          size_pages, size_pages_align);
> +
> +        addr = (uint64_t)page << E500_PLATFORM_PAGE_SHIFT;
> +    }
> +
> +    if (page >= E500_PLATFORM_HOLE_PAGES || test_bit(page, used_mem) ||
> +        (find_next_bit(used_mem, E500_PLATFORM_HOLE_PAGES, page) < size_pages)) {
> +        hw_error("e500: Memory [%"PRIx64":%"PRIx64" is already allocated or "
> +                 "no slot left", addr, size);
> +    }
> +
> +    for (i = page; i < (page + size_pages); i++) {
> +        set_bit(i, used_mem);
> +    }
> +
> +    memory_region_add_subregion(pmem, addr, device_mem);
> +    *device_addr = addr;
same for E500_PLATFORM_BASE? Actually you use plat_region_addrs[0] as an
offset wrt platorm parent region in dt_serial_create. The exact semantic
may be clarified.

Otherwise, the full patch works fine for vfio & virt too. I just moved
that code in a separate file and moved E500_* in a PlatformSettings
struct included in both PlatformNotifier and PlatformInitData.

Best Regards

Eric
> +
> +    return 0;
> +}
> +
> +static int platform_device_check(Object *obj, void *opaque)
> +{
> +    PlatformInitData *init = opaque;
> +    Object *dev;
> +    PlatformDeviceState *pdev;
> +    int i;
> +
> +    dev = object_dynamic_cast(obj, TYPE_PLATFORM_DEVICE);
> +    pdev = (PlatformDeviceState *)dev;
> +
> +    if (!pdev) {
> +        /* Container, traverse it for children */
> +        return object_child_foreach(obj, platform_device_check, opaque);
> +    }
> +
> +    /* Connect platform device to machine */
> +    for (i = 0; i < pdev->num_irqs; i++) {
> +        platform_map_irq(init->used_irqs, init->irqs, &pdev->plat_irqs[i],
> +                         pdev->irqs[i]);
> +    }
> +
> +    for (i = 0; i < pdev->num_regions; i++) {
> +        platform_map_region(init->used_mem, init->mem,
> +                            &pdev->plat_region_addrs[i], pdev->regions[i]);
> +    }
> +
> +    return 0;
> +}
> +
> +static void platform_devices_init(MemoryRegion *address_space_mem,
> +                                  qemu_irq *mpic)
> +{
> +    DECLARE_BITMAP(used_irqs, E500_PLATFORM_NUM_IRQS);
> +    DECLARE_BITMAP(used_mem, E500_PLATFORM_HOLE_PAGES);
> +    MemoryRegion *platform_region = g_new(MemoryRegion, 1);
> +    PlatformInitData init = {
> +        .used_irqs = used_irqs,
> +        .used_mem = used_mem,
> +        .mem = platform_region,
> +        .irqs = &mpic[E500_PLATFORM_FIRST_IRQ],
> +    };
> +
> +    memory_region_init(platform_region, NULL, "platform devices",
> +                       E500_PLATFORM_HOLE);
> +
> +    bitmap_clear(used_irqs, 0, E500_PLATFORM_NUM_IRQS);
> +    bitmap_clear(used_mem, 0, E500_PLATFORM_HOLE_PAGES);
> +
> +    /* Loop through our internal device tree and attach any dangling device */
> +    platform_device_check(qdev_get_machine(), &init);
> +
> +    memory_region_add_subregion(address_space_mem, E500_PLATFORM_BASE,
> +                                platform_region);
> +}
> +
> +static void platform_devices_init_notify(Notifier *notifier, void *data)
> +{
> +    PlatformNotifier *pn = (PlatformNotifier *)notifier;
> +    platform_devices_init(pn->address_space_mem, pn->mpic);
> +}
> +
>  void ppce500_init(MachineState *machine, PPCE500Params *params)
>  {
>      MemoryRegion *address_space_mem = get_system_memory();
> @@ -770,6 +982,15 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
>          cur_base = (32 * 1024 * 1024);
>      }
>  
> +    /* Platform Devices */
> +    if (params->has_platform) {
> +        PlatformNotifier *notifier = g_new(PlatformNotifier, 1);
> +        notifier->notifier.notify = platform_devices_init_notify;
> +        notifier->address_space_mem = address_space_mem;
> +        notifier->mpic = mpic;
> +        qemu_add_machine_init_done_notifier(&notifier->notifier);
> +    }
> +
>      /* Load kernel. */
>      if (machine->kernel_filename) {
>          kernel_base = cur_base;
> diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
> index 08b25fa..3a588ed 100644
> --- a/hw/ppc/e500.h
> +++ b/hw/ppc/e500.h
> @@ -11,6 +11,7 @@ typedef struct PPCE500Params {
>      void (*fixup_devtree)(struct PPCE500Params *params, void *fdt);
>  
>      int mpic_version;
> +    bool has_platform;
>  } PPCE500Params;
>  
>  void ppce500_init(MachineState *machine, PPCE500Params *params);
> diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c
> index 27df31d..bb84283 100644
> --- a/hw/ppc/e500plat.c
> +++ b/hw/ppc/e500plat.c
> @@ -35,6 +35,7 @@ static void e500plat_init(MachineState *machine)
>          .pci_nr_slots = PCI_SLOT_MAX - 1,
>          .fixup_devtree = e500plat_fixup_devtree,
>          .mpic_version = OPENPIC_MODEL_FSL_MPIC_42,
> +        .has_platform = true,
>      };
>  
>      /* Older KVM versions don't support EPR which breaks guests when we announce
> 

Re: [Qemu-devel] [PATCH 4/5] PPC: e500: Support platform devices

[Alexander Graf]

On 27.06.14 11:29, Eric Auger wrote:
> On 06/04/2014 02:28 PM, Alexander Graf wrote:
>> For e500 our approach to supporting platform devices is to create a simple
>> bus from the guest's point of view within which we map platform devices
>> dynamically.
>>
>> We allocate memory regions always within the "platform" hole in address
>> space and map IRQs to predetermined IRQ lines that are reserved for platform
>> device usage.
>>
>> This maps really nicely into device tree logic, so we can just tell the
>> guest about our virtual simple bus in device tree as well.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>>   default-configs/ppc-softmmu.mak   |   1 +
>>   default-configs/ppc64-softmmu.mak |   1 +
>>   hw/ppc/e500.c                     | 221 ++++++++++++++++++++++++++++++++++++++
>>   hw/ppc/e500.h                     |   1 +
>>   hw/ppc/e500plat.c                 |   1 +
>>   5 files changed, 225 insertions(+)
>>
>> diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
>> index 33f8d84..d6ec8b9 100644
>> --- a/default-configs/ppc-softmmu.mak
>> +++ b/default-configs/ppc-softmmu.mak
>> @@ -45,6 +45,7 @@ CONFIG_PREP=y
>>   CONFIG_MAC=y
>>   CONFIG_E500=y
>>   CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
>> +CONFIG_PLATFORM=y
>>   # For PReP
>>   CONFIG_MC146818RTC=y
>>   CONFIG_ETSEC=y
>> diff --git a/default-configs/ppc64-softmmu.mak b/default-configs/ppc64-softmmu.mak
>> index 37a15b7..06677bf 100644
>> --- a/default-configs/ppc64-softmmu.mak
>> +++ b/default-configs/ppc64-softmmu.mak
>> @@ -45,6 +45,7 @@ CONFIG_PSERIES=y
>>   CONFIG_PREP=y
>>   CONFIG_MAC=y
>>   CONFIG_E500=y
>> +CONFIG_PLATFORM=y
>>   CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
>>   # For pSeries
>>   CONFIG_XICS=$(CONFIG_PSERIES)
>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>> index 33d54b3..bc26215 100644
>> --- a/hw/ppc/e500.c
>> +++ b/hw/ppc/e500.c
>> @@ -36,6 +36,7 @@
>>   #include "exec/address-spaces.h"
>>   #include "qemu/host-utils.h"
>>   #include "hw/pci-host/ppce500.h"
>> +#include "hw/platform/device.h"
>>   
>>   #define EPAPR_MAGIC                (0x45504150)
>>   #define BINARY_DEVICE_TREE_FILE    "mpc8544ds.dtb"
>> @@ -47,6 +48,14 @@
>>   
>>   #define RAM_SIZES_ALIGN            (64UL << 20)
>>   
>> +#define E500_PLATFORM_BASE         0xF0000000ULL
>> +#define E500_PLATFORM_HOLE         (128ULL * 1024 * 1024) /* 128 MB */
>> +#define E500_PLATFORM_PAGE_SHIFT   12
>> +#define E500_PLATFORM_HOLE_PAGES   (E500_PLATFORM_HOLE >> \
>> +                                    E500_PLATFORM_PAGE_SHIFT)
>> +#define E500_PLATFORM_FIRST_IRQ    5
>> +#define E500_PLATFORM_NUM_IRQS     10
>> +
>>   /* TODO: parameterize */
>>   #define MPC8544_CCSRBAR_BASE       0xE0000000ULL
>>   #define MPC8544_CCSRBAR_SIZE       0x00100000ULL
>> @@ -122,6 +131,62 @@ static void dt_serial_create(void *fdt, unsigned long long offset,
>>       }
>>   }
>>   
>> +typedef struct PlatformDevtreeData {
>> +    void *fdt;
>> +    const char *mpic;
>> +    int irq_start;
>> +    const char *node;
>> +} PlatformDevtreeData;
>> +
>> +static int platform_device_create_devtree(Object *obj, void *opaque)
>> +{
>> +    PlatformDevtreeData *data = opaque;
>> +    Object *dev;
>> +    PlatformDeviceState *pdev;
>> +
>> +    dev = object_dynamic_cast(obj, TYPE_PLATFORM_DEVICE);
>> +    pdev = (PlatformDeviceState *)dev;
>> +
>> +    if (!pdev) {
>> +        /* Container, traverse it for children */
>> +        return object_child_foreach(obj, platform_device_create_devtree, data);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static void platform_create_devtree(void *fdt, const char *node, uint64_t addr,
>> +                                    const char *mpic, int irq_start,
>> +                                    int nr_irqs)
>> +{
>> +    const char platcomp[] = "qemu,platform\0simple-bus";
>> +    PlatformDevtreeData data;
>> +
>> +    /* Create a /platform node that we can put all devices into */
>> +
>> +    qemu_fdt_add_subnode(fdt, node);
>> +    qemu_fdt_setprop(fdt, node, "compatible", platcomp, sizeof(platcomp));
>> +    qemu_fdt_setprop_string(fdt, node, "device_type", "platform");
>> +
>> +    /* Our platform hole is less than 32bit big, so 1 cell is enough for address
>> +       and size */
>> +    qemu_fdt_setprop_cells(fdt, node, "#size-cells", 1);
>> +    qemu_fdt_setprop_cells(fdt, node, "#address-cells", 1);
>> +    qemu_fdt_setprop_cells(fdt, node, "ranges", 0, addr >> 32, addr,
>> +                           E500_PLATFORM_HOLE);
>> +
>> +    qemu_fdt_setprop_phandle(fdt, node, "interrupt-parent", mpic);
>> +
>> +    /* Loop through all devices and create nodes for known ones */
>> +
>> +    data.fdt = fdt;
>> +    data.mpic = mpic;
>> +    data.irq_start = irq_start;
>> +    data.node = node;
>> +
>> +    platform_device_create_devtree(qdev_get_machine(), &data);
>> +}
>> +
>>   static int ppce500_load_device_tree(MachineState *machine,
>>                                       PPCE500Params *params,
>>                                       hwaddr addr,
>> @@ -379,6 +444,12 @@ static int ppce500_load_device_tree(MachineState *machine,
>>       qemu_fdt_setprop_cell(fdt, pci, "#address-cells", 3);
>>       qemu_fdt_setprop_string(fdt, "/aliases", "pci0", pci);
>>   
>> +    if (params->has_platform) {
>> +        platform_create_devtree(fdt, "/platform", E500_PLATFORM_BASE,
>> +                               mpic, E500_PLATFORM_FIRST_IRQ,
>> +                               E500_PLATFORM_NUM_IRQS);
>> +    }
>> +
>>       params->fixup_devtree(params, fdt);
>>   
>>       if (toplevel_compat) {
>> @@ -618,6 +689,147 @@ static qemu_irq *ppce500_init_mpic(PPCE500Params *params, MemoryRegion *ccsr,
>>       return mpic;
>>   }
>>   
>> +typedef struct PlatformNotifier {
>> +    Notifier notifier;
>> +    MemoryRegion *address_space_mem;
>> +    qemu_irq *mpic;
>> +} PlatformNotifier;
>> +
>> +typedef struct PlatformInitData {
>> +    unsigned long *used_irqs;
>> +    unsigned long *used_mem;
>> +    MemoryRegion *mem;
>> +    qemu_irq *irqs;
>> +    int device_count;
>> +} PlatformInitData;
>> +
>> +static int platform_map_irq(unsigned long *used_irqs, qemu_irq *platform_irqs,
>> +                            uint32_t *device_irqn, qemu_irq *device_irq)
>> +{
>> +    int max_irqs = E500_PLATFORM_NUM_IRQS;
>> +    int irqn = *device_irqn;
>> +
>> +    if (irqn == (uint32_t)PLATFORM_DYNAMIC) {
>> +        /* Find the first available IRQ */
>> +        irqn = find_first_zero_bit(used_irqs, max_irqs);
>> +    }
>> +
>> +    if ((irqn >= max_irqs) || test_and_set_bit(irqn, used_irqs)) {
>> +        hw_error("e500: IRQ %d is already allocated or no free IRQ left", irqn);
>> +    }
>> +
>> +    *device_irq = platform_irqs[irqn];
>> +    *device_irqn = irqn;
> Hi Alex,
>
> Wouldn' it be more natural to add IRQ_START here, given the semantic of
> plat_irqs? Besides, I saw you added it dt_serial_create.

It depends on what you call "natural". I personally find it natural to 
only expose the limited number space that is actually available to us - 
that's the way device tree also does it.

It's incredibly hard to define a flat interrupt numbering scheme for the 
whole machine. What do you do when you have 2 interrupt controllers? 
What do you do with different types of interrupts, such as critical 
interrupt lines?

>> +
>> +    return 0;
>> +}
>> +
>> +static int platform_map_region(unsigned long *used_mem, MemoryRegion *pmem,
>> +                               uint64_t *device_addr, MemoryRegion *device_mem)
>> +{
>> +    uint64_t size = memory_region_size(device_mem);
>> +    uint64_t page_size = (1 << E500_PLATFORM_PAGE_SHIFT);
>> +    uint64_t page_mask = page_size - 1;
>> +    uint64_t size_pages = (size + page_mask) >> E500_PLATFORM_PAGE_SHIFT;
>> +    hwaddr addr = *device_addr;
>> +    int page;
>> +    int i;
>> +
>> +    page = addr >> E500_PLATFORM_PAGE_SHIFT;
>> +    if (addr == (uint64_t)PLATFORM_DYNAMIC) {
>> +        uint64_t size_pages_align;
>> +
>> +        /* Align the region to at least its own size granularity */
>> +        if (is_power_of_2(size_pages)) {
>> +            size_pages_align = size_pages;
>> +        } else {
>> +            size_pages_align = pow2floor(size_pages) << 1;
>> +        }
>> +
>> +        /* Find the first available region that fits */
>> +        page = bitmap_find_next_zero_area(used_mem, E500_PLATFORM_HOLE_PAGES, 0,
>> +                                          size_pages, size_pages_align);
>> +
>> +        addr = (uint64_t)page << E500_PLATFORM_PAGE_SHIFT;
>> +    }
>> +
>> +    if (page >= E500_PLATFORM_HOLE_PAGES || test_bit(page, used_mem) ||
>> +        (find_next_bit(used_mem, E500_PLATFORM_HOLE_PAGES, page) < size_pages)) {
>> +        hw_error("e500: Memory [%"PRIx64":%"PRIx64" is already allocated or "
>> +                 "no slot left", addr, size);
>> +    }
>> +
>> +    for (i = page; i < (page + size_pages); i++) {
>> +        set_bit(i, used_mem);
>> +    }
>> +
>> +    memory_region_add_subregion(pmem, addr, device_mem);
>> +    *device_addr = addr;
> same for E500_PLATFORM_BASE? Actually you use plat_region_addrs[0] as an
> offset wrt platorm parent region in dt_serial_create. The exact semantic
> may be clarified.

Not sure I understand :). A bus in device tree logic maps its address 
space to a subset of its parent address space. So we configure the 
device tree address space to be in line with the virtual "platform bus" 
we model.

>
> Otherwise, the full patch works fine for vfio & virt too. I just moved
> that code in a separate file and moved E500_* in a PlatformSettings
> struct included in both PlatformNotifier and PlatformInitData.

Cool, I'm glad to hear that it works :). I'm currently reworking the 
patches to add hints to SysBus instead - let's see how that works out. 
But at the end of the day, the idea stays the same and converting from 
one approach to the other should be minimal mechanical work.


Alex

Re: [Qemu-devel] [PATCH 4/5] PPC: e500: Support platform devices

[Eric Auger]

On 06/27/2014 01:30 PM, Alexander Graf wrote:
> 
> On 27.06.14 11:29, Eric Auger wrote:
>> On 06/04/2014 02:28 PM, Alexander Graf wrote:
>>> For e500 our approach to supporting platform devices is to create a
>>> simple
>>> bus from the guest's point of view within which we map platform devices
>>> dynamically.
>>>
>>> We allocate memory regions always within the "platform" hole in address
>>> space and map IRQs to predetermined IRQ lines that are reserved for
>>> platform
>>> device usage.
>>>
>>> This maps really nicely into device tree logic, so we can just tell the
>>> guest about our virtual simple bus in device tree as well.
>>>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>> ---
>>>   default-configs/ppc-softmmu.mak   |   1 +
>>>   default-configs/ppc64-softmmu.mak |   1 +
>>>   hw/ppc/e500.c                     | 221
>>> ++++++++++++++++++++++++++++++++++++++
>>>   hw/ppc/e500.h                     |   1 +
>>>   hw/ppc/e500plat.c                 |   1 +
>>>   5 files changed, 225 insertions(+)
>>>
>>> diff --git a/default-configs/ppc-softmmu.mak
>>> b/default-configs/ppc-softmmu.mak
>>> index 33f8d84..d6ec8b9 100644
>>> --- a/default-configs/ppc-softmmu.mak
>>> +++ b/default-configs/ppc-softmmu.mak
>>> @@ -45,6 +45,7 @@ CONFIG_PREP=y
>>>   CONFIG_MAC=y
>>>   CONFIG_E500=y
>>>   CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
>>> +CONFIG_PLATFORM=y
>>>   # For PReP
>>>   CONFIG_MC146818RTC=y
>>>   CONFIG_ETSEC=y
>>> diff --git a/default-configs/ppc64-softmmu.mak
>>> b/default-configs/ppc64-softmmu.mak
>>> index 37a15b7..06677bf 100644
>>> --- a/default-configs/ppc64-softmmu.mak
>>> +++ b/default-configs/ppc64-softmmu.mak
>>> @@ -45,6 +45,7 @@ CONFIG_PSERIES=y
>>>   CONFIG_PREP=y
>>>   CONFIG_MAC=y
>>>   CONFIG_E500=y
>>> +CONFIG_PLATFORM=y
>>>   CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
>>>   # For pSeries
>>>   CONFIG_XICS=$(CONFIG_PSERIES)
>>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>>> index 33d54b3..bc26215 100644
>>> --- a/hw/ppc/e500.c
>>> +++ b/hw/ppc/e500.c
>>> @@ -36,6 +36,7 @@
>>>   #include "exec/address-spaces.h"
>>>   #include "qemu/host-utils.h"
>>>   #include "hw/pci-host/ppce500.h"
>>> +#include "hw/platform/device.h"
>>>     #define EPAPR_MAGIC                (0x45504150)
>>>   #define BINARY_DEVICE_TREE_FILE    "mpc8544ds.dtb"
>>> @@ -47,6 +48,14 @@
>>>     #define RAM_SIZES_ALIGN            (64UL << 20)
>>>   +#define E500_PLATFORM_BASE         0xF0000000ULL
>>> +#define E500_PLATFORM_HOLE         (128ULL * 1024 * 1024) /* 128 MB */
>>> +#define E500_PLATFORM_PAGE_SHIFT   12
>>> +#define E500_PLATFORM_HOLE_PAGES   (E500_PLATFORM_HOLE >> \
>>> +                                    E500_PLATFORM_PAGE_SHIFT)
>>> +#define E500_PLATFORM_FIRST_IRQ    5
>>> +#define E500_PLATFORM_NUM_IRQS     10
>>> +
>>>   /* TODO: parameterize */
>>>   #define MPC8544_CCSRBAR_BASE       0xE0000000ULL
>>>   #define MPC8544_CCSRBAR_SIZE       0x00100000ULL
>>> @@ -122,6 +131,62 @@ static void dt_serial_create(void *fdt, unsigned
>>> long long offset,
>>>       }
>>>   }
>>>   +typedef struct PlatformDevtreeData {
>>> +    void *fdt;
>>> +    const char *mpic;
>>> +    int irq_start;
>>> +    const char *node;
>>> +} PlatformDevtreeData;
>>> +
>>> +static int platform_device_create_devtree(Object *obj, void *opaque)
>>> +{
>>> +    PlatformDevtreeData *data = opaque;
>>> +    Object *dev;
>>> +    PlatformDeviceState *pdev;
>>> +
>>> +    dev = object_dynamic_cast(obj, TYPE_PLATFORM_DEVICE);
>>> +    pdev = (PlatformDeviceState *)dev;
>>> +
>>> +    if (!pdev) {
>>> +        /* Container, traverse it for children */
>>> +        return object_child_foreach(obj,
>>> platform_device_create_devtree, data);
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static void platform_create_devtree(void *fdt, const char *node,
>>> uint64_t addr,
>>> +                                    const char *mpic, int irq_start,
>>> +                                    int nr_irqs)
>>> +{
>>> +    const char platcomp[] = "qemu,platform\0simple-bus";
>>> +    PlatformDevtreeData data;
>>> +
>>> +    /* Create a /platform node that we can put all devices into */
>>> +
>>> +    qemu_fdt_add_subnode(fdt, node);
>>> +    qemu_fdt_setprop(fdt, node, "compatible", platcomp,
>>> sizeof(platcomp));
>>> +    qemu_fdt_setprop_string(fdt, node, "device_type", "platform");
>>> +
>>> +    /* Our platform hole is less than 32bit big, so 1 cell is enough
>>> for address
>>> +       and size */
>>> +    qemu_fdt_setprop_cells(fdt, node, "#size-cells", 1);
>>> +    qemu_fdt_setprop_cells(fdt, node, "#address-cells", 1);
>>> +    qemu_fdt_setprop_cells(fdt, node, "ranges", 0, addr >> 32, addr,
>>> +                           E500_PLATFORM_HOLE);
>>> +
>>> +    qemu_fdt_setprop_phandle(fdt, node, "interrupt-parent", mpic);
>>> +
>>> +    /* Loop through all devices and create nodes for known ones */
>>> +
>>> +    data.fdt = fdt;
>>> +    data.mpic = mpic;
>>> +    data.irq_start = irq_start;
>>> +    data.node = node;
>>> +
>>> +    platform_device_create_devtree(qdev_get_machine(), &data);
>>> +}
>>> +
>>>   static int ppce500_load_device_tree(MachineState *machine,
>>>                                       PPCE500Params *params,
>>>                                       hwaddr addr,
>>> @@ -379,6 +444,12 @@ static int ppce500_load_device_tree(MachineState
>>> *machine,
>>>       qemu_fdt_setprop_cell(fdt, pci, "#address-cells", 3);
>>>       qemu_fdt_setprop_string(fdt, "/aliases", "pci0", pci);
>>>   +    if (params->has_platform) {
>>> +        platform_create_devtree(fdt, "/platform", E500_PLATFORM_BASE,
>>> +                               mpic, E500_PLATFORM_FIRST_IRQ,
>>> +                               E500_PLATFORM_NUM_IRQS);
>>> +    }
>>> +
>>>       params->fixup_devtree(params, fdt);
>>>         if (toplevel_compat) {
>>> @@ -618,6 +689,147 @@ static qemu_irq
>>> *ppce500_init_mpic(PPCE500Params *params, MemoryRegion *ccsr,
>>>       return mpic;
>>>   }
>>>   +typedef struct PlatformNotifier {
>>> +    Notifier notifier;
>>> +    MemoryRegion *address_space_mem;
>>> +    qemu_irq *mpic;
>>> +} PlatformNotifier;
>>> +
>>> +typedef struct PlatformInitData {
>>> +    unsigned long *used_irqs;
>>> +    unsigned long *used_mem;
>>> +    MemoryRegion *mem;
>>> +    qemu_irq *irqs;
>>> +    int device_count;
>>> +} PlatformInitData;
>>> +
>>> +static int platform_map_irq(unsigned long *used_irqs, qemu_irq
>>> *platform_irqs,
>>> +                            uint32_t *device_irqn, qemu_irq
>>> *device_irq)
>>> +{
>>> +    int max_irqs = E500_PLATFORM_NUM_IRQS;
>>> +    int irqn = *device_irqn;
>>> +
>>> +    if (irqn == (uint32_t)PLATFORM_DYNAMIC) {
>>> +        /* Find the first available IRQ */
>>> +        irqn = find_first_zero_bit(used_irqs, max_irqs);
>>> +    }
>>> +
>>> +    if ((irqn >= max_irqs) || test_and_set_bit(irqn, used_irqs)) {
>>> +        hw_error("e500: IRQ %d is already allocated or no free IRQ
>>> left", irqn);
>>> +    }
>>> +
>>> +    *device_irq = platform_irqs[irqn];
>>> +    *device_irqn = irqn;
>> Hi Alex,
>>
>> Wouldn' it be more natural to add IRQ_START here, given the semantic of
>> plat_irqs? Besides, I saw you added it dt_serial_create.
> 
> It depends on what you call "natural". I personally find it natural to
> only expose the limited number space that is actually available to us -
> that's the way device tree also does it.
> 
> It's incredibly hard to define a flat interrupt numbering scheme for the
> whole machine. What do you do when you have 2 interrupt controllers?
> What do you do with different types of interrupts, such as critical
> interrupt lines?
Hi Alex

I agree with you for both irqs and region_addrs. With your explanations
it makes sense but for me this semantic was not self-explanatory from
the device.h.

BR

Eric
> 
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int platform_map_region(unsigned long *used_mem, MemoryRegion
>>> *pmem,
>>> +                               uint64_t *device_addr, MemoryRegion
>>> *device_mem)
>>> +{
>>> +    uint64_t size = memory_region_size(device_mem);
>>> +    uint64_t page_size = (1 << E500_PLATFORM_PAGE_SHIFT);
>>> +    uint64_t page_mask = page_size - 1;
>>> +    uint64_t size_pages = (size + page_mask) >>
>>> E500_PLATFORM_PAGE_SHIFT;
>>> +    hwaddr addr = *device_addr;
>>> +    int page;
>>> +    int i;
>>> +
>>> +    page = addr >> E500_PLATFORM_PAGE_SHIFT;
>>> +    if (addr == (uint64_t)PLATFORM_DYNAMIC) {
>>> +        uint64_t size_pages_align;
>>> +
>>> +        /* Align the region to at least its own size granularity */
>>> +        if (is_power_of_2(size_pages)) {
>>> +            size_pages_align = size_pages;
>>> +        } else {
>>> +            size_pages_align = pow2floor(size_pages) << 1;
>>> +        }
>>> +
>>> +        /* Find the first available region that fits */
>>> +        page = bitmap_find_next_zero_area(used_mem,
>>> E500_PLATFORM_HOLE_PAGES, 0,
>>> +                                          size_pages,
>>> size_pages_align);
>>> +
>>> +        addr = (uint64_t)page << E500_PLATFORM_PAGE_SHIFT;
>>> +    }
>>> +
>>> +    if (page >= E500_PLATFORM_HOLE_PAGES || test_bit(page, used_mem) ||
>>> +        (find_next_bit(used_mem, E500_PLATFORM_HOLE_PAGES, page) <
>>> size_pages)) {
>>> +        hw_error("e500: Memory [%"PRIx64":%"PRIx64" is already
>>> allocated or "
>>> +                 "no slot left", addr, size);
>>> +    }
>>> +
>>> +    for (i = page; i < (page + size_pages); i++) {
>>> +        set_bit(i, used_mem);
>>> +    }
>>> +
>>> +    memory_region_add_subregion(pmem, addr, device_mem);
>>> +    *device_addr = addr;
>> same for E500_PLATFORM_BASE? Actually you use plat_region_addrs[0] as an
>> offset wrt platorm parent region in dt_serial_create. The exact semantic
>> may be clarified.
> 
> Not sure I understand :). A bus in device tree logic maps its address
> space to a subset of its parent address space. So we configure the
> device tree address space to be in line with the virtual "platform bus"
> we model.
> 
>>
>> Otherwise, the full patch works fine for vfio & virt too. I just moved
>> that code in a separate file and moved E500_* in a PlatformSettings
>> struct included in both PlatformNotifier and PlatformInitData.
> 
> Cool, I'm glad to hear that it works :). I'm currently reworking the
> patches to add hints to SysBus instead - let's see how that works out.
> But at the end of the day, the idea stays the same and converting from
> one approach to the other should be minimal mechanical work.
> 
> 
> Alex
> 

[Qemu-devel] [PATCH 5/5] PPC: e500: Add support for platform serial devices

[Alexander Graf]

The guest only becomes aware of platform devices when we describe them inside
the device tree.

This patch adds description support for platform serial devices in device tree,
allowing us to specify a serial port on the command line with -device.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 default-configs/ppc-softmmu.mak   |  1 +
 default-configs/ppc64-softmmu.mak |  1 +
 hw/ppc/e500.c                     | 28 ++++++++++++++++++++++++++--
 3 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
index d6ec8b9..63e7949 100644
--- a/default-configs/ppc-softmmu.mak
+++ b/default-configs/ppc-softmmu.mak
@@ -46,6 +46,7 @@ CONFIG_MAC=y
 CONFIG_E500=y
 CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
 CONFIG_PLATFORM=y
+CONFIG_SERIAL_PLATFORM=y
 # For PReP
 CONFIG_MC146818RTC=y
 CONFIG_ETSEC=y
diff --git a/default-configs/ppc64-softmmu.mak b/default-configs/ppc64-softmmu.mak
index 06677bf..b72fb45 100644
--- a/default-configs/ppc64-softmmu.mak
+++ b/default-configs/ppc64-softmmu.mak
@@ -46,6 +46,7 @@ CONFIG_PREP=y
 CONFIG_MAC=y
 CONFIG_E500=y
 CONFIG_PLATFORM=y
+CONFIG_SERIAL_PLATFORM=y
 CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
 # For pSeries
 CONFIG_XICS=$(CONFIG_PSERIES)
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index bc26215..39f028f 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -37,6 +37,7 @@
 #include "qemu/host-utils.h"
 #include "hw/pci-host/ppce500.h"
 #include "hw/platform/device.h"
+#include "hw/char/serial-platform.h"
 
 #define EPAPR_MAGIC                (0x45504150)
 #define BINARY_DEVICE_TREE_FILE    "mpc8544ds.dtb"
@@ -110,7 +111,7 @@ static uint32_t *pci_map_create(void *fdt, uint32_t mpic, int first_slot,
 }
 
 static void dt_serial_create(void *fdt, unsigned long long offset,
-                             const char *soc, const char *mpic,
+                             const char *soc, const char *mpic, int irq,
                              const char *alias, int idx, bool defcon)
 {
     char ser[128];
@@ -122,7 +123,7 @@ static void dt_serial_create(void *fdt, unsigned long long offset,
     qemu_fdt_setprop_cells(fdt, ser, "reg", offset, 0x100);
     qemu_fdt_setprop_cell(fdt, ser, "cell-index", idx);
     qemu_fdt_setprop_cell(fdt, ser, "clock-frequency", 0);
-    qemu_fdt_setprop_cells(fdt, ser, "interrupts", 42, 2);
+    qemu_fdt_setprop_cells(fdt, ser, "interrupts", irq, 2);
     qemu_fdt_setprop_phandle(fdt, ser, "interrupt-parent", mpic);
     qemu_fdt_setprop_string(fdt, "/aliases", alias, ser);
 
@@ -135,6 +136,7 @@ typedef struct PlatformDevtreeData {
     void *fdt;
     const char *mpic;
     int irq_start;
+    int serial_idx;
     const char *node;
 } PlatformDevtreeData;
 
@@ -152,9 +154,30 @@ static int platform_device_create_devtree(Object *obj, void *opaque)
         return object_child_foreach(obj, platform_device_create_devtree, data);
     }
 
+    if (PLATFORM_SERIAL(dev)) {
+        char *alias = g_strdup_printf("serial%d", data->serial_idx);
+        dt_serial_create(data->fdt, pdev->plat_region_addrs[0], data->node,
+                         data->mpic, data->irq_start + pdev->plat_irqs[0],
+                         alias, data->serial_idx, data->serial_idx == 0);
+        data->serial_idx++;
+    }
+
     return 0;
 }
 
+static int count_serial_hds(void)
+{
+    int r = 0, i;
+
+    for (i = 0; i < 2; i++) {
+        if (serial_hds[i]) {
+            r++;
+        }
+    }
+
+    return r;
+}
+
 static void platform_create_devtree(void *fdt, const char *node, uint64_t addr,
                                     const char *mpic, int irq_start,
                                     int nr_irqs)
@@ -180,6 +203,7 @@ static void platform_create_devtree(void *fdt, const char *node, uint64_t addr,
     /* Loop through all devices and create nodes for known ones */
 
     data.fdt = fdt;
+    data.serial_idx = count_serial_hds();
     data.mpic = mpic;
     data.irq_start = irq_start;
     data.node = node;
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 0/5] Platform device support

[Paolo Bonzini]

Il 04/06/2014 14:28, Alexander Graf ha scritto:
>
> But do we need that level of complexity for normal devices usually? In a
> normal platform world (SoCs, PV machines) we have a flat memory layout we
> can plug our device memory into. We also have a flat IRQ model where we
> can plug our device IRQs into.
>
> So the idea for platform devices arose. A platform device is really just a
> device that exposes its qemu_irq slots and memory regions to the world.
>
> That allows us to write machine specific code that maps a platform device
> wherever the machine thinks fits nicely. It also allows that same machine
> to generate a device tree entry for platform devices easily, as it is fully
> aware of the interrupt lines and places it was mapped to.
>
> A device (read: user) may or may not explictly request to be mapped at a
> specific IRQ and/or memory address. If no explicit mapping is requested,
> platform devices can get mapped at convenient places by the machine.
>
> The actual pressing issue this solves is that today it's impossible to spawn
> serial ports from the command line. With this patch set, it's possible to
> do so. But it lays the groundwork for much more...

I have more than a suspect that you and Peter Crosthwaite are trying to 
do the same.  I hope that memory region QOMification can get into 2.1, 
that can be a starting point that we can work from.

Paolo

Re: [Qemu-devel] [PATCH 0/5] Platform device support

[Alexander Graf]

On 19.06.14 22:54, Paolo Bonzini wrote:
> Il 04/06/2014 14:28, Alexander Graf ha scritto:
>>
>> But do we need that level of complexity for normal devices usually? In a
>> normal platform world (SoCs, PV machines) we have a flat memory 
>> layout we
>> can plug our device memory into. We also have a flat IRQ model where we
>> can plug our device IRQs into.
>>
>> So the idea for platform devices arose. A platform device is really 
>> just a
>> device that exposes its qemu_irq slots and memory regions to the world.
>>
>> That allows us to write machine specific code that maps a platform 
>> device
>> wherever the machine thinks fits nicely. It also allows that same 
>> machine
>> to generate a device tree entry for platform devices easily, as it is 
>> fully
>> aware of the interrupt lines and places it was mapped to.
>>
>> A device (read: user) may or may not explictly request to be mapped at a
>> specific IRQ and/or memory address. If no explicit mapping is requested,
>> platform devices can get mapped at convenient places by the machine.
>>
>> The actual pressing issue this solves is that today it's impossible 
>> to spawn
>> serial ports from the command line. With this patch set, it's 
>> possible to
>> do so. But it lays the groundwork for much more...
>
> I have more than a suspect that you and Peter Crosthwaite are trying 
> to do the same.  I hope that memory region QOMification can get into 
> 2.1, that can be a starting point that we can work from.

The major difference between the two of us is that Peter wants to have 
the user specify everything on the command line so he can build a VM 
from the device tree description of a system.

I want to make sure the user does not have to specify anything that 
could go wrong on the command line, similar to how he can just use 
-device pci-dev today. The more we automatically allocate for him, the 
less can go wrong.


Alex

Re: [Qemu-devel] [PATCH 0/5] Platform device support

[Peter Crosthwaite]

On Wed, Jun 4, 2014 at 10:28 PM, Alexander Graf <agraf@suse.de> wrote:
> Platforms without ISA and/or PCI have had a seriously hard time in the dynamic
> device creation world of QEMU. Devices on these were modeled as SysBus devices
> which can only be instantiated in machine files, not through -device.
>
> Why is that so?
>
> Well, SysBus is trying to be incredibly generic. It allows you to plug any
> interrupt sender into any other interrupt receiver. It allows you to map
> a device's memory regions into any other random memory region. All of that
> only works from C code or via really complicated command line arguments under
> discussion upstream right now.
>

What you are doing seem to me to be an extension of SysBus - you are
defining the same interfaces as sysbus but also adding some machine
specifics wiring info. I think it's a candidate for QOM inheritance to
avoid having to dup all the sysbus device models for both regular
sysbus and platform bus. I think your functionality should be added as
one of

1: and interface that can be added to sysbus devices
2: a new abstraction that inherits from SYS_BUS_DEVICE
3: just new features to the sysbus core.

Then both of us are using the same suite of device models and the
differences between our approaches are limited to machine level
instantiation method. My gut says #2 is the cleanest.

> But do we need that level of complexity for normal devices usually? In a
> normal platform world (SoCs, PV machines) we have a flat memory layout we
> can plug our device memory into. We also have a flat IRQ model where we
> can plug our device IRQs into.
>
> So the idea for platform devices arose. A platform device is really just a
> device that exposes its qemu_irq slots and memory regions to the world.
>
> That allows us to write machine specific code that maps a platform device
> wherever the machine thinks fits nicely. It also allows that same machine
> to generate a device tree entry for platform devices easily, as it is fully
> aware of the interrupt lines and places it was mapped to.
>
> A device (read: user) may or may not explictly request to be mapped at a
> specific IRQ and/or memory address. If no explicit mapping is requested,
> platform devices can get mapped at convenient places by the machine.
>
> The actual pressing issue this solves is that today it's impossible to spawn
> serial ports from the command line. With this patch set, it's possible to
> do so. But it lays the groundwork for much more...
>
> Alexander Graf (5):
>   Platform: Add platform device class
>   Platform: Add serial device
>   PPC: e500: Only create dt entries for existing serial ports
>   PPC: e500: Support platform devices
>   PPC: e500: Add support for platform serial devices
>
>  default-configs/ppc-softmmu.mak   |   2 +
>  default-configs/ppc64-softmmu.mak |   2 +
>  hw/Makefile.objs                  |   1 +
>  hw/char/Makefile.objs             |   1 +
>  hw/char/serial-platform.c         | 103 +++++++++++++++

I think a big point of confusion here is you have picked a
not-even-qdevified device for conversion. Boards are still calling
serial_mm_init() directly due to lack of a proper device for Sysbus
serial.

Do you have another device that already QOMified (a MAC or something
perhaps?) that you can convert more minimally to demonstrate the
approach for existing sysbus devs?

Sysbusification of serial would then be a step towards platform-device serial.

Regards,
Peter

>  hw/platform/Makefile.objs         |   1 +
>  hw/platform/device.c              | 108 ++++++++++++++++
>  hw/ppc/e500.c                     | 262 +++++++++++++++++++++++++++++++++++++-
>  hw/ppc/e500.h                     |   1 +
>  hw/ppc/e500plat.c                 |   1 +
>  include/hw/char/serial-platform.h |  56 ++++++++
>  include/hw/platform/device.h      |  45 +++++++
>  12 files changed, 577 insertions(+), 6 deletions(-)
>  create mode 100644 hw/char/serial-platform.c
>  create mode 100644 hw/platform/Makefile.objs
>  create mode 100644 hw/platform/device.c
>  create mode 100644 include/hw/char/serial-platform.h
>  create mode 100644 include/hw/platform/device.h
>
> --
> 1.8.1.4
>
>

Re: [Qemu-devel] [PATCH 0/5] Platform device support

[Paolo Bonzini]

Il 20/06/2014 08:43, Peter Crosthwaite ha scritto:
> I think a big point of confusion here is you have picked a
> not-even-qdevified device for conversion. Boards are still calling
> serial_mm_init() directly due to lack of a proper device for Sysbus
> serial.
>
> Do you have another device that already QOMified (a MAC or something
> perhaps?) that you can convert more minimally to demonstrate the
> approach for existing sysbus devs?
>
> Sysbusification of serial would then be a step towards platform-device serial.

Indeed, I would first start with patches that make serial_mm_init a 
sysbus wrapper.  Then, with techniques similar to Peter's work, it 
should be possible to extend -device and do something like

    -device serial-mmio,memory.addr=0x1f000000,irq=gpio[5]

or something like that.

The only difference is whether you get the information from device tree 
or command line.

Paolo

Re: [Qemu-devel] [PATCH 0/5] Platform device support

[Alexander Graf]

On 20.06.14 08:43, Peter Crosthwaite wrote:
> On Wed, Jun 4, 2014 at 10:28 PM, Alexander Graf <agraf@suse.de> wrote:
>> Platforms without ISA and/or PCI have had a seriously hard time in the dynamic
>> device creation world of QEMU. Devices on these were modeled as SysBus devices
>> which can only be instantiated in machine files, not through -device.
>>
>> Why is that so?
>>
>> Well, SysBus is trying to be incredibly generic. It allows you to plug any
>> interrupt sender into any other interrupt receiver. It allows you to map
>> a device's memory regions into any other random memory region. All of that
>> only works from C code or via really complicated command line arguments under
>> discussion upstream right now.
>>
> What you are doing seem to me to be an extension of SysBus - you are
> defining the same interfaces as sysbus but also adding some machine
> specifics wiring info. I think it's a candidate for QOM inheritance to
> avoid having to dup all the sysbus device models for both regular
> sysbus and platform bus. I think your functionality should be added as
> one of
>
> 1: and interface that can be added to sysbus devices
> 2: a new abstraction that inherits from SYS_BUS_DEVICE
> 3: just new features to the sysbus core.
>
> Then both of us are using the same suite of device models and the
> differences between our approaches are limited to machine level
> instantiation method. My gut says #2 is the cleanest.

The more I think about it the more I believe #3 would be the cleanest. 
The only thing my platform devices do in addition to sysbus devices is 
that it exposes qdev properties to give mapping code hints where a 
device wants to be mapped.

If we just add qdev properties for all the possible hints in generic 
sysbus core code, we should be able to automatically convert all devices 
into dynamically allocatable devices. Whether they actually do get 
mapped and the generation of device tree chunks still stays in the the 
machine file's court.


Alex

Re: [Qemu-devel] [PATCH 0/5] Platform device support

[Andreas Färber]

Am 26.06.2014 14:01, schrieb Alexander Graf:
> 
> On 20.06.14 08:43, Peter Crosthwaite wrote:
>> On Wed, Jun 4, 2014 at 10:28 PM, Alexander Graf <agraf@suse.de> wrote:
>>> Platforms without ISA and/or PCI have had a seriously hard time in
>>> the dynamic
>>> device creation world of QEMU. Devices on these were modeled as
>>> SysBus devices
>>> which can only be instantiated in machine files, not through -device.
>>>
>>> Why is that so?
>>>
>>> Well, SysBus is trying to be incredibly generic. It allows you to
>>> plug any
>>> interrupt sender into any other interrupt receiver. It allows you to map
>>> a device's memory regions into any other random memory region. All of
>>> that
>>> only works from C code or via really complicated command line
>>> arguments under
>>> discussion upstream right now.
>>>
>> What you are doing seem to me to be an extension of SysBus - you are
>> defining the same interfaces as sysbus but also adding some machine
>> specifics wiring info. I think it's a candidate for QOM inheritance to
>> avoid having to dup all the sysbus device models for both regular
>> sysbus and platform bus. I think your functionality should be added as
>> one of
>>
>> 1: and interface that can be added to sysbus devices
>> 2: a new abstraction that inherits from SYS_BUS_DEVICE
>> 3: just new features to the sysbus core.
>>
>> Then both of us are using the same suite of device models and the
>> differences between our approaches are limited to machine level
>> instantiation method. My gut says #2 is the cleanest.
> 
> The more I think about it the more I believe #3 would be the cleanest.
> The only thing my platform devices do in addition to sysbus devices is
> that it exposes qdev properties to give mapping code hints where a
> device wants to be mapped.
> 
> If we just add qdev properties for all the possible hints in generic
> sysbus core code, we should be able to automatically convert all devices
> into dynamically allocatable devices. Whether they actually do get
> mapped and the generation of device tree chunks still stays in the the
> machine file's court.

As discussed offline with Alex, one issue I see is that this would be
encouraging people to add more devices to an artificial global bus in
/machine/unassigned that we've been trying to obsolete, rather than
sitting down and please creating an e500 SoC object as a start. Maybe we
should start generating a list of shame for 2.1. ;)
Instantiating a new [Sys/AXI/AMBA/...]Bus inside that SoC object would
make me much happier than using SysBus as is.

The pure QOM approach would be link<> properties instead of a bus, but
then the machine needs to know how many "slots" there shall be in
advance. Note that the "docking procedure" is always initiated from the
realizing device, whether bus or no bus.

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

Re: [Qemu-devel] [PATCH 0/5] Platform device support

[Peter Crosthwaite]

On Fri, Jun 27, 2014 at 8:30 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 26.06.2014 14:01, schrieb Alexander Graf:
>>
>> On 20.06.14 08:43, Peter Crosthwaite wrote:
>>> On Wed, Jun 4, 2014 at 10:28 PM, Alexander Graf <agraf@suse.de> wrote:
>>>> Platforms without ISA and/or PCI have had a seriously hard time in
>>>> the dynamic
>>>> device creation world of QEMU. Devices on these were modeled as
>>>> SysBus devices
>>>> which can only be instantiated in machine files, not through -device.
>>>>
>>>> Why is that so?
>>>>
>>>> Well, SysBus is trying to be incredibly generic. It allows you to
>>>> plug any
>>>> interrupt sender into any other interrupt receiver. It allows you to map
>>>> a device's memory regions into any other random memory region. All of
>>>> that
>>>> only works from C code or via really complicated command line
>>>> arguments under
>>>> discussion upstream right now.
>>>>
>>> What you are doing seem to me to be an extension of SysBus - you are
>>> defining the same interfaces as sysbus but also adding some machine
>>> specifics wiring info. I think it's a candidate for QOM inheritance to
>>> avoid having to dup all the sysbus device models for both regular
>>> sysbus and platform bus. I think your functionality should be added as
>>> one of
>>>
>>> 1: and interface that can be added to sysbus devices
>>> 2: a new abstraction that inherits from SYS_BUS_DEVICE
>>> 3: just new features to the sysbus core.
>>>
>>> Then both of us are using the same suite of device models and the
>>> differences between our approaches are limited to machine level
>>> instantiation method. My gut says #2 is the cleanest.
>>
>> The more I think about it the more I believe #3 would be the cleanest.
>> The only thing my platform devices do in addition to sysbus devices is
>> that it exposes qdev properties to give mapping code hints where a
>> device wants to be mapped.
>>
>> If we just add qdev properties for all the possible hints in generic
>> sysbus core code, we should be able to automatically convert all devices
>> into dynamically allocatable devices. Whether they actually do get
>> mapped and the generation of device tree chunks still stays in the the
>> machine file's court.
>
> As discussed offline with Alex, one issue I see is that this would be
> encouraging people to add more devices to an artificial global bus in
> /machine/unassigned that we've been trying to obsolete, rather than
> sitting down and please creating an e500 SoC object as a start. Maybe we
> should start generating a list of shame for 2.1. ;)
> Instantiating a new [Sys/AXI/AMBA/...]Bus inside that SoC object would
> make me much happier than using SysBus as is.
>

Do you mean &address_space_memory (as used by sysbus_mmio_map)? We all
hate that global singleton, but can we decouple it from sysbus which
is not the root cause of that problem? sysbus_mmio_map usages just
need to be replaced with sysbus_mmio_get_region and you can create
whatever heirachy you want using unchanged sysbus devices.

Even if we phase out the global singleton and the SysBus "bus", the
sysbus "device" abstraction is still sound and should be usable
busless. Then theres no need a for a tree-wide to implement Alex's
feature for all devs (assuming his plugger can be made to work
hintless?).

Regards,
Peter

> The pure QOM approach would be link<> properties instead of a bus, but
> then the machine needs to know how many "slots" there shall be in
> advance. Note that the "docking procedure" is always initiated from the
> realizing device, whether bus or no bus.
>
> 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
>

Re: [Qemu-devel] [PATCH 0/5] Platform device support

[Andreas Färber]

Am 27.06.2014 12:54, schrieb Peter Crosthwaite:
> On Fri, Jun 27, 2014 at 8:30 PM, Andreas Färber <afaerber@suse.de> wrote:
>> Am 26.06.2014 14:01, schrieb Alexander Graf:
>>> On 20.06.14 08:43, Peter Crosthwaite wrote:
>>>> On Wed, Jun 4, 2014 at 10:28 PM, Alexander Graf <agraf@suse.de> wrote:
>>>>> Platforms without ISA and/or PCI have had a seriously hard time in
>>>>> the dynamic
>>>>> device creation world of QEMU. Devices on these were modeled as
>>>>> SysBus devices
>>>>> which can only be instantiated in machine files, not through -device.
>>>>>
>>>>> Why is that so?
>>>>>
>>>>> Well, SysBus is trying to be incredibly generic. It allows you to
>>>>> plug any
>>>>> interrupt sender into any other interrupt receiver. It allows you to map
>>>>> a device's memory regions into any other random memory region. All of
>>>>> that
>>>>> only works from C code or via really complicated command line
>>>>> arguments under
>>>>> discussion upstream right now.
>>>>>
>>>> What you are doing seem to me to be an extension of SysBus - you are
>>>> defining the same interfaces as sysbus but also adding some machine
>>>> specifics wiring info. I think it's a candidate for QOM inheritance to
>>>> avoid having to dup all the sysbus device models for both regular
>>>> sysbus and platform bus. I think your functionality should be added as
>>>> one of
>>>>
>>>> 1: and interface that can be added to sysbus devices
>>>> 2: a new abstraction that inherits from SYS_BUS_DEVICE
>>>> 3: just new features to the sysbus core.
>>>>
>>>> Then both of us are using the same suite of device models and the
>>>> differences between our approaches are limited to machine level
>>>> instantiation method. My gut says #2 is the cleanest.
>>>
>>> The more I think about it the more I believe #3 would be the cleanest.
>>> The only thing my platform devices do in addition to sysbus devices is
>>> that it exposes qdev properties to give mapping code hints where a
>>> device wants to be mapped.
>>>
>>> If we just add qdev properties for all the possible hints in generic
>>> sysbus core code, we should be able to automatically convert all devices
>>> into dynamically allocatable devices. Whether they actually do get
>>> mapped and the generation of device tree chunks still stays in the the
>>> machine file's court.
>>
>> As discussed offline with Alex, one issue I see is that this would be
>> encouraging people to add more devices to an artificial global bus in
>> /machine/unassigned that we've been trying to obsolete, rather than
>> sitting down and please creating an e500 SoC object as a start. Maybe we
>> should start generating a list of shame for 2.1. ;)
>> Instantiating a new [Sys/AXI/AMBA/...]Bus inside that SoC object would
>> make me much happier than using SysBus as is.
>>
> 
> Do you mean &address_space_memory (as used by sysbus_mmio_map)?

No, I mean the QOM composition model. When we think of using -device,
then they will go to /machine/peripheral/<id> or
/machine/peripheral-anon/device[n]; in your case that means that you get
a flat list of devices rather than a structure matching your device
tree. And like I said above, in both your and Alex' case SysBus is
something that has no real place in the composition tree unless we go
from that single unholy qdev-required bus to buses as they exist in the
hardware, like Anthony suggested long time ago. Alex' problem with that
is that he doesn't want to implement the same UART logic for 50
different-but-same buses, so some form of reuse or inheritance would be
needed.

Disclaimer: I have not yet reviewed this series, I was commenting on
abstract ideas that Alex requested feedback for.

Cheers,
Andreas

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

Re: [Qemu-devel] [PATCH 0/5] Platform device support

[Alexander Graf]

On 27.06.14 13:17, Andreas Färber wrote:
> Am 27.06.2014 12:54, schrieb Peter Crosthwaite:
>> On Fri, Jun 27, 2014 at 8:30 PM, Andreas Färber <afaerber@suse.de> wrote:
>>> Am 26.06.2014 14:01, schrieb Alexander Graf:
>>>> On 20.06.14 08:43, Peter Crosthwaite wrote:
>>>>> On Wed, Jun 4, 2014 at 10:28 PM, Alexander Graf <agraf@suse.de> wrote:
>>>>>> Platforms without ISA and/or PCI have had a seriously hard time in
>>>>>> the dynamic
>>>>>> device creation world of QEMU. Devices on these were modeled as
>>>>>> SysBus devices
>>>>>> which can only be instantiated in machine files, not through -device.
>>>>>>
>>>>>> Why is that so?
>>>>>>
>>>>>> Well, SysBus is trying to be incredibly generic. It allows you to
>>>>>> plug any
>>>>>> interrupt sender into any other interrupt receiver. It allows you to map
>>>>>> a device's memory regions into any other random memory region. All of
>>>>>> that
>>>>>> only works from C code or via really complicated command line
>>>>>> arguments under
>>>>>> discussion upstream right now.
>>>>>>
>>>>> What you are doing seem to me to be an extension of SysBus - you are
>>>>> defining the same interfaces as sysbus but also adding some machine
>>>>> specifics wiring info. I think it's a candidate for QOM inheritance to
>>>>> avoid having to dup all the sysbus device models for both regular
>>>>> sysbus and platform bus. I think your functionality should be added as
>>>>> one of
>>>>>
>>>>> 1: and interface that can be added to sysbus devices
>>>>> 2: a new abstraction that inherits from SYS_BUS_DEVICE
>>>>> 3: just new features to the sysbus core.
>>>>>
>>>>> Then both of us are using the same suite of device models and the
>>>>> differences between our approaches are limited to machine level
>>>>> instantiation method. My gut says #2 is the cleanest.
>>>> The more I think about it the more I believe #3 would be the cleanest.
>>>> The only thing my platform devices do in addition to sysbus devices is
>>>> that it exposes qdev properties to give mapping code hints where a
>>>> device wants to be mapped.
>>>>
>>>> If we just add qdev properties for all the possible hints in generic
>>>> sysbus core code, we should be able to automatically convert all devices
>>>> into dynamically allocatable devices. Whether they actually do get
>>>> mapped and the generation of device tree chunks still stays in the the
>>>> machine file's court.
>>> As discussed offline with Alex, one issue I see is that this would be
>>> encouraging people to add more devices to an artificial global bus in
>>> /machine/unassigned that we've been trying to obsolete, rather than
>>> sitting down and please creating an e500 SoC object as a start. Maybe we
>>> should start generating a list of shame for 2.1. ;)
>>> Instantiating a new [Sys/AXI/AMBA/...]Bus inside that SoC object would
>>> make me much happier than using SysBus as is.
>>>
>> Do you mean &address_space_memory (as used by sysbus_mmio_map)?
> No, I mean the QOM composition model. When we think of using -device,
> then they will go to /machine/peripheral/<id> or
> /machine/peripheral-anon/device[n]; in your case that means that you get
> a flat list of devices rather than a structure matching your device
> tree. And like I said above, in both your and Alex' case SysBus is
> something that has no real place in the composition tree unless we go
> from that single unholy qdev-required bus to buses as they exist in the
> hardware, like Anthony suggested long time ago. Alex' problem with that
> is that he doesn't want to implement the same UART logic for 50
> different-but-same buses, so some form of reuse or inheritance would be
> needed.
>
> Disclaimer: I have not yet reviewed this series, I was commenting on
> abstract ideas that Alex requested feedback for.

I think we can all agree that the sysbus bus is not a bus per se. So 
conceptually, what's the difference between a device attached to a 
non-bus and a device not attached to a bus at all? And why can't we 
convert sysbus to not be a bus anymore?


Alex

Re: [Qemu-devel] [PATCH 0/5] Platform device support

[Paolo Bonzini]

Il 27/06/2014 13:24, Alexander Graf ha scritto:
>
> I think we can all agree that the sysbus bus is not a bus per se. So
> conceptually, what's the difference between a device attached to a
> non-bus and a device not attached to a bus at all? And why can't we
> convert sysbus to not be a bus anymore?

I think there is no difference, and I don't think moving out of sysbus 
is really a goal that we need to pursue.

I agree with Andreas that having a "SoC object" as father of sysbus 
(instead of "nothing at all") would be slightly better.

We could also make TYPE_MACHINE a subclass of TYPE_DEVICE, to have an 
obvious place for this SoC object.

Paolo

Re: [Qemu-devel] [PATCH 0/5] Platform device support

[Peter Maydell]

On 27 June 2014 12:48, Paolo Bonzini <pbonzini@redhat.com> wrote:
> We could also make TYPE_MACHINE a subclass of TYPE_DEVICE, to have an
> obvious place for this SoC object.

Why isn't TYPE_MACHINE a subclass of TYPE_DEVICE anyway?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/5] Platform device support

[Paolo Bonzini]

Il 27/06/2014 13:52, Peter Maydell ha scritto:
> On 27 June 2014 12:48, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> We could also make TYPE_MACHINE a subclass of TYPE_DEVICE, to have an
>> obvious place for this SoC object.
>
> Why isn't TYPE_MACHINE a subclass of TYPE_DEVICE anyway?

To start the conversion with an API that is more similar to the 
preexisting one of QEMUMachine.

Paolo

Re: [Qemu-devel] [PATCH 0/5] Platform device support

[Alexander Graf]

On 27.06.14 12:54, Peter Crosthwaite wrote:
> On Fri, Jun 27, 2014 at 8:30 PM, Andreas Färber <afaerber@suse.de> wrote:
>> Am 26.06.2014 14:01, schrieb Alexander Graf:
>>> On 20.06.14 08:43, Peter Crosthwaite wrote:
>>>> On Wed, Jun 4, 2014 at 10:28 PM, Alexander Graf <agraf@suse.de> wrote:
>>>>> Platforms without ISA and/or PCI have had a seriously hard time in
>>>>> the dynamic
>>>>> device creation world of QEMU. Devices on these were modeled as
>>>>> SysBus devices
>>>>> which can only be instantiated in machine files, not through -device.
>>>>>
>>>>> Why is that so?
>>>>>
>>>>> Well, SysBus is trying to be incredibly generic. It allows you to
>>>>> plug any
>>>>> interrupt sender into any other interrupt receiver. It allows you to map
>>>>> a device's memory regions into any other random memory region. All of
>>>>> that
>>>>> only works from C code or via really complicated command line
>>>>> arguments under
>>>>> discussion upstream right now.
>>>>>
>>>> What you are doing seem to me to be an extension of SysBus - you are
>>>> defining the same interfaces as sysbus but also adding some machine
>>>> specifics wiring info. I think it's a candidate for QOM inheritance to
>>>> avoid having to dup all the sysbus device models for both regular
>>>> sysbus and platform bus. I think your functionality should be added as
>>>> one of
>>>>
>>>> 1: and interface that can be added to sysbus devices
>>>> 2: a new abstraction that inherits from SYS_BUS_DEVICE
>>>> 3: just new features to the sysbus core.
>>>>
>>>> Then both of us are using the same suite of device models and the
>>>> differences between our approaches are limited to machine level
>>>> instantiation method. My gut says #2 is the cleanest.
>>> The more I think about it the more I believe #3 would be the cleanest.
>>> The only thing my platform devices do in addition to sysbus devices is
>>> that it exposes qdev properties to give mapping code hints where a
>>> device wants to be mapped.
>>>
>>> If we just add qdev properties for all the possible hints in generic
>>> sysbus core code, we should be able to automatically convert all devices
>>> into dynamically allocatable devices. Whether they actually do get
>>> mapped and the generation of device tree chunks still stays in the the
>>> machine file's court.
>> As discussed offline with Alex, one issue I see is that this would be
>> encouraging people to add more devices to an artificial global bus in
>> /machine/unassigned that we've been trying to obsolete, rather than
>> sitting down and please creating an e500 SoC object as a start. Maybe we
>> should start generating a list of shame for 2.1. ;)
>> Instantiating a new [Sys/AXI/AMBA/...]Bus inside that SoC object would
>> make me much happier than using SysBus as is.
>>
> Do you mean &address_space_memory (as used by sysbus_mmio_map)? We all
> hate that global singleton, but can we decouple it from sysbus which
> is not the root cause of that problem? sysbus_mmio_map usages just
> need to be replaced with sysbus_mmio_get_region and you can create
> whatever heirachy you want using unchanged sysbus devices.
>
> Even if we phase out the global singleton and the SysBus "bus", the
> sysbus "device" abstraction is still sound and should be usable
> busless. Then theres no need a for a tree-wide to implement Alex's
> feature for all devs (assuming his plugger can be made to work
> hintless?).

The plugger works just fine when you don't give hints - then it's up to 
dynamic allocation (same as PCI).

Yes I fully agree with you here.


Alex

Re: [Qemu-devel] [PATCH 0/5] Platform device support

[Alexander Graf]

On 27.06.14 12:30, Andreas Färber wrote:
> Am 26.06.2014 14:01, schrieb Alexander Graf:
>> On 20.06.14 08:43, Peter Crosthwaite wrote:
>>> On Wed, Jun 4, 2014 at 10:28 PM, Alexander Graf <agraf@suse.de> wrote:
>>>> Platforms without ISA and/or PCI have had a seriously hard time in
>>>> the dynamic
>>>> device creation world of QEMU. Devices on these were modeled as
>>>> SysBus devices
>>>> which can only be instantiated in machine files, not through -device.
>>>>
>>>> Why is that so?
>>>>
>>>> Well, SysBus is trying to be incredibly generic. It allows you to
>>>> plug any
>>>> interrupt sender into any other interrupt receiver. It allows you to map
>>>> a device's memory regions into any other random memory region. All of
>>>> that
>>>> only works from C code or via really complicated command line
>>>> arguments under
>>>> discussion upstream right now.
>>>>
>>> What you are doing seem to me to be an extension of SysBus - you are
>>> defining the same interfaces as sysbus but also adding some machine
>>> specifics wiring info. I think it's a candidate for QOM inheritance to
>>> avoid having to dup all the sysbus device models for both regular
>>> sysbus and platform bus. I think your functionality should be added as
>>> one of
>>>
>>> 1: and interface that can be added to sysbus devices
>>> 2: a new abstraction that inherits from SYS_BUS_DEVICE
>>> 3: just new features to the sysbus core.
>>>
>>> Then both of us are using the same suite of device models and the
>>> differences between our approaches are limited to machine level
>>> instantiation method. My gut says #2 is the cleanest.
>> The more I think about it the more I believe #3 would be the cleanest.
>> The only thing my platform devices do in addition to sysbus devices is
>> that it exposes qdev properties to give mapping code hints where a
>> device wants to be mapped.
>>
>> If we just add qdev properties for all the possible hints in generic
>> sysbus core code, we should be able to automatically convert all devices
>> into dynamically allocatable devices. Whether they actually do get
>> mapped and the generation of device tree chunks still stays in the the
>> machine file's court.
> As discussed offline with Alex, one issue I see is that this would be
> encouraging people to add more devices to an artificial global bus in
> /machine/unassigned that we've been trying to obsolete, rather than
> sitting down and please creating an e500 SoC object as a start. Maybe we
> should start generating a list of shame for 2.1. ;)
> Instantiating a new [Sys/AXI/AMBA/...]Bus inside that SoC object would
> make me much happier than using SysBus as is.
>
> The pure QOM approach would be link<> properties instead of a bus, but
> then the machine needs to know how many "slots" there shall be in
> advance. Note that the "docking procedure" is always initiated from the
> realizing device, whether bus or no bus.

So my goal is to make life easy for users, not to fulfill some wet 
Anthony dreams :). And as a user, I want to be able to say -device foo 
and have that device created, like I do with PCI devices today.

There are 2 approaches to this that I can see:

   1) A new special type of bus that allows for dynamic allocation and 
that knows a flat numbering scheme
   2) Individual devices that get attached to whatever the machine file 
thinks makes it happy (basically emulating the above bus, but with more 
flexibility)

I implemented option 1 with the "Platform bus". It's basically an 
abstraction of the Sys/AXI/AMBA idea but only with a single bus 
implementation, as everything else would just be ridiculously redundant 
(and if necessary could be implemented as a subclass on top of the 
bridge device). People didn't like it.

I implemented option 2 with the Platform devices - this patch set. 
People didn't like it because it duplicates SysBus devices - and it does.

I'm implementing 2 as an add-on of SysBusDevice now which to me really 
isn't too much different from a dangling QOM device.

Linking devices by force (set IRQ0 to MPIC IRQ 32, map region0 to 
physical address space offset 0x12300) is a nice thing to have for 
people who know what they're doing. That matches probably about 0.00001% 
of our user base - I personally am not included there. We *have* to have 
a mechanism to make device creation easy for users if we want to have any.


Alex