[v3,1/6] hw/misc/dyn_sysbus_binding: helpers for sysbus device dynamic binding

This new module implements routines which help in dynamic device
binding (mmio regions, irq). They are supposed to be used by machine
files that support dynamic sysbus instantiation.

Signed-off-by: Alexander Graf <agraf@suse.de>
Signed-off-by: Eric Auger <eric.auger@linaro.org>

---

v1 -> v2:
- platform_devices renamed into dyn_sysbus_binding
- PlatformParams renamed into DynSysbusParams
- PlatformBusNotifier renamed into DynSysbusNotifier
- platform_bus_map_irq, platform_bus_map_mmio, sysbus_device_check,
  platform_bus_init become static
- PlatformBusInitData becomes private to the module
- page_shift becomes a member of DynSysbusParams

v1: Dynamic sysbus device allocation fully written by Alex Graf.
Those functions were initially in ppc e500 machine file. Now moved to a
separate module.
PPCE500Params is replaced by a generic struct named PlatformParams
---
 hw/misc/Makefile.objs                |   1 +
 hw/misc/dyn_sysbus_binding.c         | 163 +++++++++++++++++++++++++++++++++++
 include/hw/misc/dyn_sysbus_binding.h |  24 ++++++
 3 files changed, 188 insertions(+)
 create mode 100644 hw/misc/dyn_sysbus_binding.c
 create mode 100644 include/hw/misc/dyn_sysbus_binding.h

Il 09/09/2014 09:54, Eric Auger ha scritto:
> This new module implements routines which help in dynamic device
> binding (mmio regions, irq). They are supposed to be used by machine
> files that support dynamic sysbus instantiation.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> 
> ---
> 
> v1 -> v2:
> - platform_devices renamed into dyn_sysbus_binding

Please use "dynamic" instead of dyn.

> - PlatformParams renamed into DynSysbusParams
> - PlatformBusNotifier renamed into DynSysbusNotifier
> - platform_bus_map_irq, platform_bus_map_mmio, sysbus_device_check,
>   platform_bus_init become static
> - PlatformBusInitData becomes private to the module
> - page_shift becomes a member of DynSysbusParams
> 
> v1: Dynamic sysbus device allocation fully written by Alex Graf.
> Those functions were initially in ppc e500 machine file. Now moved to a
> separate module.
> PPCE500Params is replaced by a generic struct named PlatformParams
> ---
>  hw/misc/Makefile.objs                |   1 +
>  hw/misc/dyn_sysbus_binding.c         | 163 +++++++++++++++++++++++++++++++++++

Please put these in hw/core/sysbus.c and include/hw/sysbus.h, and rename
everything called "Platform" or "platform_" to "Sysbus" or "sysbus_".

> +void platform_bus_init_notify(Notifier *notifier, void *data)
> +{
> +    DynSysbusNotifier *pn = (DynSysbusNotifier *)notifier;
> +    platform_bus_init(&pn->params, pn->address_space_mem, pn->mpic);
> +}

Please provide a wrapper like sysbus_register_dynamic that takes the
params/address_space_mem/mpic as parameter.  platform_bus_init_notify
and DynSysbusNotifier can remain hidden within the .c file.

Also, why not move the address_space_mem and mpic into the parameters
struct?

Paolo

>  include/hw/misc/dyn_sysbus_binding.h |  24 ++++++
>  3 files changed, 188 insertions(+)
>  create mode 100644 hw/misc/dyn_sysbus_binding.c
>  create mode 100644 include/hw/misc/dyn_sysbus_binding.h
> 
> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> index 979e532..86f6243 100644
> --- a/hw/misc/Makefile.objs
> +++ b/hw/misc/Makefile.objs
> @@ -41,3 +41,4 @@ obj-$(CONFIG_SLAVIO) += slavio_misc.o
>  obj-$(CONFIG_ZYNQ) += zynq_slcr.o
>  
>  obj-$(CONFIG_PVPANIC) += pvpanic.o
> +obj-y += dyn_sysbus_binding.o
> diff --git a/hw/misc/dyn_sysbus_binding.c b/hw/misc/dyn_sysbus_binding.c
> new file mode 100644
> index 0000000..0f34f0b
> --- /dev/null
> +++ b/hw/misc/dyn_sysbus_binding.c
> @@ -0,0 +1,163 @@
> +#include "hw/misc/dyn_sysbus_binding.h"
> +#include "qemu/error-report.h"
> +
> +typedef struct PlatformBusInitData {
> +    unsigned long *used_irqs;
> +    unsigned long *used_mem;
> +    MemoryRegion *mem;
> +    qemu_irq *irqs;
> +    int device_count;
> +    DynSysbusParams *params;
> +} PlatformBusInitData;
> +
> +
> +static int platform_bus_map_irq(DynSysbusParams *params,
> +                                SysBusDevice *sbdev,
> +                                int n, unsigned long *used_irqs,
> +                                qemu_irq *platform_irqs)
> +{
> +    int max_irqs = params->platform_bus_num_irqs;
> +    char *prop = g_strdup_printf("irq[%d]", n);
> +    int irqn = object_property_get_int(OBJECT(sbdev), prop, NULL);
> +
> +    if (irqn == SYSBUS_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("IRQ %d is already allocated or no free IRQ left", irqn);
> +    }
> +
> +    sysbus_connect_irq(sbdev, n, platform_irqs[irqn]);
> +    object_property_set_int(OBJECT(sbdev), irqn, prop, NULL);
> +
> +    g_free(prop);
> +    return 0;
> +}
> +
> +static int platform_bus_map_mmio(DynSysbusParams *params,
> +                                 SysBusDevice *sbdev,
> +                                 int n, unsigned long *used_mem,
> +                                 MemoryRegion *pmem)
> +{
> +    MemoryRegion *device_mem = sbdev->mmio[n].memory;
> +    uint64_t size = memory_region_size(device_mem);
> +    uint64_t page_size = (1 << params->page_shift);
> +    uint64_t page_mask = page_size - 1;
> +    uint64_t size_pages = (size + page_mask) >> params->page_shift;
> +    uint64_t max_size = params->platform_bus_size;
> +    uint64_t max_pages = max_size >> params->page_shift;
> +    char *prop = g_strdup_printf("mmio[%d]", n);
> +    hwaddr addr = object_property_get_int(OBJECT(sbdev), prop, NULL);
> +    int page;
> +    int i;
> +
> +    page = addr >> params->page_shift;
> +    if (addr == SYSBUS_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, max_pages, 0, size_pages,
> +                                          size_pages_align);
> +
> +        addr = (uint64_t)page << params->page_shift;
> +    }
> +
> +    if (page >= max_pages || test_bit(page, used_mem) ||
> +        (find_next_bit(used_mem, max_pages, page) < size_pages)) {
> +        hw_error("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);
> +    sbdev->mmio[n].addr = addr;
> +    object_property_set_int(OBJECT(sbdev), addr, prop, NULL);
> +
> +    g_free(prop);
> +    return 0;
> +}
> +
> +static int sysbus_device_check(Object *obj, void *opaque)
> +{
> +    PlatformBusInitData *init = opaque;
> +    Object *dev;
> +    SysBusDevice *sbdev;
> +    int i;
> +
> +    dev = object_dynamic_cast(obj, TYPE_SYS_BUS_DEVICE);
> +    sbdev = (SysBusDevice *)dev;
> +
> +    if (!sbdev) {
> +        /* Container, traverse it for children */
> +        return object_child_foreach(obj, sysbus_device_check, opaque);
> +    }
> +
> +    /* Connect sysbus device to virtual platform bus */
> +    for (i = 0; i < sbdev->num_irq; i++) {
> +        if (!sbdev->irqp[i]) {
> +            /* This IRQ is an incoming IRQ, we can't wire those here */
> +            continue;
> +        }
> +        platform_bus_map_irq(init->params, sbdev, i,
> +                             init->used_irqs, init->irqs);
> +    }
> +
> +    for (i = 0; i < sbdev->num_mmio; i++) {
> +        platform_bus_map_mmio(init->params, sbdev, i,
> +                              init->used_mem, init->mem);
> +    }
> +
> +    return 0;
> +}
> +
> +static void platform_bus_init(DynSysbusParams *params,
> +                       MemoryRegion *address_space_mem,
> +                       qemu_irq *mpic)
> +{
> +    uint64_t max_size = params->platform_bus_size;
> +    uint64_t max_pages = max_size >> params->page_shift;
> +    DECLARE_BITMAP(used_irqs, params->platform_bus_num_irqs);
> +    DECLARE_BITMAP(used_mem, max_pages);
> +    MemoryRegion *platform_region = g_new(MemoryRegion, 1);
> +    Object *container;
> +    PlatformBusInitData init = {
> +        .used_irqs = used_irqs,
> +        .used_mem = used_mem,
> +        .mem = platform_region,
> +        .irqs = &mpic[params->platform_bus_first_irq],
> +        .params = params,
> +    };
> +
> +    memory_region_init(platform_region, NULL, "platform devices",
> +                       params->platform_bus_size);
> +
> +    bitmap_clear(used_irqs, 0, params->platform_bus_num_irqs);
> +    bitmap_clear(used_mem, 0, max_pages);
> +
> +    /* Loop through all sysbus devices that were spawened outside the machine */
> +    container = container_get(qdev_get_machine(), "/peripheral");
> +    sysbus_device_check(container, &init);
> +    container = container_get(qdev_get_machine(), "/peripheral-anon");
> +    sysbus_device_check(container, &init);
> +
> +    memory_region_add_subregion(address_space_mem, params->platform_bus_base,
> +                                platform_region);
> +}
> +
> +void platform_bus_init_notify(Notifier *notifier, void *data)
> +{
> +    DynSysbusNotifier *pn = (DynSysbusNotifier *)notifier;
> +    platform_bus_init(&pn->params, pn->address_space_mem, pn->mpic);
> +}
> diff --git a/include/hw/misc/dyn_sysbus_binding.h b/include/hw/misc/dyn_sysbus_binding.h
> new file mode 100644
> index 0000000..961c9c7
> --- /dev/null
> +++ b/include/hw/misc/dyn_sysbus_binding.h
> @@ -0,0 +1,24 @@
> +#ifndef HW_MISC_PLATFORM_DEVICES_H
> +#define HW_MISC_PLATFORM_DEVICES_H
> +
> +#include "qemu-common.h"
> +#include "hw/sysbus.h"
> +
> +typedef struct {
> +    bool has_platform_bus;
> +    hwaddr platform_bus_base;
> +    hwaddr platform_bus_size;
> +    int platform_bus_first_irq;
> +    int platform_bus_num_irqs;
> +    int page_shift;
> +} DynSysbusParams;
> +
> +typedef struct DynSysbusNotifier {
> +    Notifier notifier;
> +    MemoryRegion *address_space_mem;
> +    qemu_irq *mpic;
> +    DynSysbusParams params;
> +} DynSysbusNotifier;
> +
> +void platform_bus_init_notify(Notifier *notifier, void *data);
> +#endif
>

On 09/09/2014 12:56 PM, Paolo Bonzini wrote:
> Il 09/09/2014 09:54, Eric Auger ha scritto:
>> This new module implements routines which help in dynamic device
>> binding (mmio regions, irq). They are supposed to be used by machine
>> files that support dynamic sysbus instantiation.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>>
>> v1 -> v2:
>> - platform_devices renamed into dyn_sysbus_binding
> 
> Please use "dynamic" instead of dyn.
Hi Paolo,
Thanks for the quick review.
I will remove such abbreviation.
> 
>> - PlatformParams renamed into DynSysbusParams
>> - PlatformBusNotifier renamed into DynSysbusNotifier
>> - platform_bus_map_irq, platform_bus_map_mmio, sysbus_device_check,
>>   platform_bus_init become static
>> - PlatformBusInitData becomes private to the module
>> - page_shift becomes a member of DynSysbusParams
>>
>> v1: Dynamic sysbus device allocation fully written by Alex Graf.
>> Those functions were initially in ppc e500 machine file. Now moved to a
>> separate module.
>> PPCE500Params is replaced by a generic struct named PlatformParams
>> ---
>>  hw/misc/Makefile.objs                |   1 +
>>  hw/misc/dyn_sysbus_binding.c         | 163 +++++++++++++++++++++++++++++++++++
> 
> Please put these in hw/core/sysbus.c and include/hw/sysbus.h, and rename
> everything called "Platform" or "platform_" to "Sysbus" or "sysbus_".
OK
> 
>> +void platform_bus_init_notify(Notifier *notifier, void *data)
>> +{
>> +    DynSysbusNotifier *pn = (DynSysbusNotifier *)notifier;
>> +    platform_bus_init(&pn->params, pn->address_space_mem, pn->mpic);
>> +}
> 
> Please provide a wrapper like sysbus_register_dynamic that takes the
> params/address_space_mem/mpic as parameter.  platform_bus_init_notify
> and DynSysbusNotifier can remain hidden within the .c file.
Sorry I do not catch what you mean here. platform_bus_init_notify &
DynSysbusNotifier are currently used in virt.c to initialize & register
the machine_init_done_notifier.
> 
> Also, why not move the address_space_mem and mpic into the parameters
> struct?
Yes sure

Best Regards

Eric
> 
> Paolo
> 
>>  include/hw/misc/dyn_sysbus_binding.h |  24 ++++++
>>  3 files changed, 188 insertions(+)
>>  create mode 100644 hw/misc/dyn_sysbus_binding.c
>>  create mode 100644 include/hw/misc/dyn_sysbus_binding.h
>>
>> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
>> index 979e532..86f6243 100644
>> --- a/hw/misc/Makefile.objs
>> +++ b/hw/misc/Makefile.objs
>> @@ -41,3 +41,4 @@ obj-$(CONFIG_SLAVIO) += slavio_misc.o
>>  obj-$(CONFIG_ZYNQ) += zynq_slcr.o
>>  
>>  obj-$(CONFIG_PVPANIC) += pvpanic.o
>> +obj-y += dyn_sysbus_binding.o
>> diff --git a/hw/misc/dyn_sysbus_binding.c b/hw/misc/dyn_sysbus_binding.c
>> new file mode 100644
>> index 0000000..0f34f0b
>> --- /dev/null
>> +++ b/hw/misc/dyn_sysbus_binding.c
>> @@ -0,0 +1,163 @@
>> +#include "hw/misc/dyn_sysbus_binding.h"
>> +#include "qemu/error-report.h"
>> +
>> +typedef struct PlatformBusInitData {
>> +    unsigned long *used_irqs;
>> +    unsigned long *used_mem;
>> +    MemoryRegion *mem;
>> +    qemu_irq *irqs;
>> +    int device_count;
>> +    DynSysbusParams *params;
>> +} PlatformBusInitData;
>> +
>> +
>> +static int platform_bus_map_irq(DynSysbusParams *params,
>> +                                SysBusDevice *sbdev,
>> +                                int n, unsigned long *used_irqs,
>> +                                qemu_irq *platform_irqs)
>> +{
>> +    int max_irqs = params->platform_bus_num_irqs;
>> +    char *prop = g_strdup_printf("irq[%d]", n);
>> +    int irqn = object_property_get_int(OBJECT(sbdev), prop, NULL);
>> +
>> +    if (irqn == SYSBUS_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("IRQ %d is already allocated or no free IRQ left", irqn);
>> +    }
>> +
>> +    sysbus_connect_irq(sbdev, n, platform_irqs[irqn]);
>> +    object_property_set_int(OBJECT(sbdev), irqn, prop, NULL);
>> +
>> +    g_free(prop);
>> +    return 0;
>> +}
>> +
>> +static int platform_bus_map_mmio(DynSysbusParams *params,
>> +                                 SysBusDevice *sbdev,
>> +                                 int n, unsigned long *used_mem,
>> +                                 MemoryRegion *pmem)
>> +{
>> +    MemoryRegion *device_mem = sbdev->mmio[n].memory;
>> +    uint64_t size = memory_region_size(device_mem);
>> +    uint64_t page_size = (1 << params->page_shift);
>> +    uint64_t page_mask = page_size - 1;
>> +    uint64_t size_pages = (size + page_mask) >> params->page_shift;
>> +    uint64_t max_size = params->platform_bus_size;
>> +    uint64_t max_pages = max_size >> params->page_shift;
>> +    char *prop = g_strdup_printf("mmio[%d]", n);
>> +    hwaddr addr = object_property_get_int(OBJECT(sbdev), prop, NULL);
>> +    int page;
>> +    int i;
>> +
>> +    page = addr >> params->page_shift;
>> +    if (addr == SYSBUS_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, max_pages, 0, size_pages,
>> +                                          size_pages_align);
>> +
>> +        addr = (uint64_t)page << params->page_shift;
>> +    }
>> +
>> +    if (page >= max_pages || test_bit(page, used_mem) ||
>> +        (find_next_bit(used_mem, max_pages, page) < size_pages)) {
>> +        hw_error("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);
>> +    sbdev->mmio[n].addr = addr;
>> +    object_property_set_int(OBJECT(sbdev), addr, prop, NULL);
>> +
>> +    g_free(prop);
>> +    return 0;
>> +}
>> +
>> +static int sysbus_device_check(Object *obj, void *opaque)
>> +{
>> +    PlatformBusInitData *init = opaque;
>> +    Object *dev;
>> +    SysBusDevice *sbdev;
>> +    int i;
>> +
>> +    dev = object_dynamic_cast(obj, TYPE_SYS_BUS_DEVICE);
>> +    sbdev = (SysBusDevice *)dev;
>> +
>> +    if (!sbdev) {
>> +        /* Container, traverse it for children */
>> +        return object_child_foreach(obj, sysbus_device_check, opaque);
>> +    }
>> +
>> +    /* Connect sysbus device to virtual platform bus */
>> +    for (i = 0; i < sbdev->num_irq; i++) {
>> +        if (!sbdev->irqp[i]) {
>> +            /* This IRQ is an incoming IRQ, we can't wire those here */
>> +            continue;
>> +        }
>> +        platform_bus_map_irq(init->params, sbdev, i,
>> +                             init->used_irqs, init->irqs);
>> +    }
>> +
>> +    for (i = 0; i < sbdev->num_mmio; i++) {
>> +        platform_bus_map_mmio(init->params, sbdev, i,
>> +                              init->used_mem, init->mem);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static void platform_bus_init(DynSysbusParams *params,
>> +                       MemoryRegion *address_space_mem,
>> +                       qemu_irq *mpic)
>> +{
>> +    uint64_t max_size = params->platform_bus_size;
>> +    uint64_t max_pages = max_size >> params->page_shift;
>> +    DECLARE_BITMAP(used_irqs, params->platform_bus_num_irqs);
>> +    DECLARE_BITMAP(used_mem, max_pages);
>> +    MemoryRegion *platform_region = g_new(MemoryRegion, 1);
>> +    Object *container;
>> +    PlatformBusInitData init = {
>> +        .used_irqs = used_irqs,
>> +        .used_mem = used_mem,
>> +        .mem = platform_region,
>> +        .irqs = &mpic[params->platform_bus_first_irq],
>> +        .params = params,
>> +    };
>> +
>> +    memory_region_init(platform_region, NULL, "platform devices",
>> +                       params->platform_bus_size);
>> +
>> +    bitmap_clear(used_irqs, 0, params->platform_bus_num_irqs);
>> +    bitmap_clear(used_mem, 0, max_pages);
>> +
>> +    /* Loop through all sysbus devices that were spawened outside the machine */
>> +    container = container_get(qdev_get_machine(), "/peripheral");
>> +    sysbus_device_check(container, &init);
>> +    container = container_get(qdev_get_machine(), "/peripheral-anon");
>> +    sysbus_device_check(container, &init);
>> +
>> +    memory_region_add_subregion(address_space_mem, params->platform_bus_base,
>> +                                platform_region);
>> +}
>> +
>> +void platform_bus_init_notify(Notifier *notifier, void *data)
>> +{
>> +    DynSysbusNotifier *pn = (DynSysbusNotifier *)notifier;
>> +    platform_bus_init(&pn->params, pn->address_space_mem, pn->mpic);
>> +}
>> diff --git a/include/hw/misc/dyn_sysbus_binding.h b/include/hw/misc/dyn_sysbus_binding.h
>> new file mode 100644
>> index 0000000..961c9c7
>> --- /dev/null
>> +++ b/include/hw/misc/dyn_sysbus_binding.h
>> @@ -0,0 +1,24 @@
>> +#ifndef HW_MISC_PLATFORM_DEVICES_H
>> +#define HW_MISC_PLATFORM_DEVICES_H
>> +
>> +#include "qemu-common.h"
>> +#include "hw/sysbus.h"
>> +
>> +typedef struct {
>> +    bool has_platform_bus;
>> +    hwaddr platform_bus_base;
>> +    hwaddr platform_bus_size;
>> +    int platform_bus_first_irq;
>> +    int platform_bus_num_irqs;
>> +    int page_shift;
>> +} DynSysbusParams;
>> +
>> +typedef struct DynSysbusNotifier {
>> +    Notifier notifier;
>> +    MemoryRegion *address_space_mem;
>> +    qemu_irq *mpic;
>> +    DynSysbusParams params;
>> +} DynSysbusNotifier;
>> +
>> +void platform_bus_init_notify(Notifier *notifier, void *data);
>> +#endif
>>
>

Il 09/09/2014 17:25, Eric Auger ha scritto:
>> > 
>> > Please provide a wrapper like sysbus_register_dynamic that takes the
>> > params/address_space_mem/mpic as parameter.  platform_bus_init_notify
>> > and DynSysbusNotifier can remain hidden within the .c file.
> Sorry I do not catch what you mean here. platform_bus_init_notify &
> DynSysbusNotifier are currently used in virt.c to initialize & register
> the machine_init_done_notifier.

Yeah, please do the registration in sysbus.c, not in virt.c.  There is
no reason to make the platform_bus_init_notify+DynSysbusNotifier
interface public.  The code in sysbus.c can fill in the fields.

Paolo

On 09/09/2014 05:59 PM, Paolo Bonzini wrote:
> Il 09/09/2014 17:25, Eric Auger ha scritto:
>>>>
>>>> Please provide a wrapper like sysbus_register_dynamic that takes the
>>>> params/address_space_mem/mpic as parameter.  platform_bus_init_notify
>>>> and DynSysbusNotifier can remain hidden within the .c file.
>> Sorry I do not catch what you mean here. platform_bus_init_notify &
>> DynSysbusNotifier are currently used in virt.c to initialize & register
>> the machine_init_done_notifier.
> 
> Yeah, please do the registration in sysbus.c, not in virt.c.  There is
> no reason to make the platform_bus_init_notify+DynSysbusNotifier
> interface public.  The code in sysbus.c can fill in the fields.
OK I will investigate that.

Thanks

Eric
> 
> Paolo
>

On 09.09.14 17:59, Paolo Bonzini wrote:
> Il 09/09/2014 17:25, Eric Auger ha scritto:
>>>>
>>>> Please provide a wrapper like sysbus_register_dynamic that takes the
>>>> params/address_space_mem/mpic as parameter.  platform_bus_init_notify
>>>> and DynSysbusNotifier can remain hidden within the .c file.
>> Sorry I do not catch what you mean here. platform_bus_init_notify &
>> DynSysbusNotifier are currently used in virt.c to initialize & register
>> the machine_init_done_notifier.
> 
> Yeah, please do the registration in sysbus.c, not in virt.c.  There is
> no reason to make the platform_bus_init_notify+DynSysbusNotifier
> interface public.  The code in sysbus.c can fill in the fields.

Sysbus != Platform bus. Sysbus is an in-QEMU representation of a
pseudo-bus that we put all devices onto that we consider unsorted.

Platform bus is a machine representation of an actual bus that devices
are attached to. These devices usually are sysbus devices.


Alex

Il 10/09/2014 11:31, Alexander Graf ha scritto:
>> > Yeah, please do the registration in sysbus.c, not in virt.c.  There is
>> > no reason to make the platform_bus_init_notify+DynSysbusNotifier
>> > interface public.  The code in sysbus.c can fill in the fields.
> Sysbus != Platform bus. Sysbus is an in-QEMU representation of a
> pseudo-bus that we put all devices onto that we consider unsorted.
> 
> Platform bus is a machine representation of an actual bus that devices
> are attached to. These devices usually are sysbus devices.

Is there any difference between the two?

Take a machine that has two chips, a SoC that does everything except
USB, and a USB controller chip.

Strictly speaking the USB controller chip would be on a "platform bus",
but we would likely put it on sysbus.

Why should it matter whether the devices are static or dynamic, for the
sake of calling something the "system" or the "platform" bus?  I would
say that QEMU calls "sysbus" the platform bus.

Some devices (e.g. the local APIC in x86, or the in-core timers and GIC
in ARM) should probably not be in sysbus at all, and should attach
directly to the CPU address space.  But that is a quirk in the modeling
of those devices, it shouldn't mean that sysbus is not a "platform" bus.

Paolo

On 10.09.14 11:43, Paolo Bonzini wrote:
> Il 10/09/2014 11:31, Alexander Graf ha scritto:
>>>> Yeah, please do the registration in sysbus.c, not in virt.c.  There is
>>>> no reason to make the platform_bus_init_notify+DynSysbusNotifier
>>>> interface public.  The code in sysbus.c can fill in the fields.
>> Sysbus != Platform bus. Sysbus is an in-QEMU representation of a
>> pseudo-bus that we put all devices onto that we consider unsorted.
>>
>> Platform bus is a machine representation of an actual bus that devices
>> are attached to. These devices usually are sysbus devices.
> 
> Is there any difference between the two?
> 
> Take a machine that has two chips, a SoC that does everything except
> USB, and a USB controller chip.
> 
> Strictly speaking the USB controller chip would be on a "platform bus",
> but we would likely put it on sysbus.
> 
> Why should it matter whether the devices are static or dynamic, for the
> sake of calling something the "system" or the "platform" bus?  I would
> say that QEMU calls "sysbus" the platform bus.
> 
> Some devices (e.g. the local APIC in x86, or the in-core timers and GIC
> in ARM) should probably not be in sysbus at all, and should attach
> directly to the CPU address space.  But that is a quirk in the modeling
> of those devices, it shouldn't mean that sysbus is not a "platform" bus.

On e500 for example, we have a predefined CCSR region. That is a machine
defined "platform bus". The offsets inside that region are strictly
defined by the spec.

Now take the serial ports. We have space for 2 serial ports inside of
that CCSR region. We can spawn these 2 ports in the machine file based
on -serial, but if you want to spawn them with -device, how do you tell
the machine whether they should go into the "big bucket platform bus" or
the "CCSR platform bus"?

In fact, thinking about this a bit more, maybe we should just have an
actual bus structure. Then we could have the legacy "big bucket" sysbus
bus that nobody may ever dynamically put devices into. For CCSR we could
create another bucket that the machine file can control where each
device goes and can also detect if a device doesn't fit. And then we
just declare the virt "platform bus" sysbus the default bus for cmdline
-device sysbusdevice and everything resolves automatically.


Alex

Il 10/09/2014 11:56, Alexander Graf ha scritto:
> 
> 
> On 10.09.14 11:43, Paolo Bonzini wrote:
>> Il 10/09/2014 11:31, Alexander Graf ha scritto:
>>>>> Yeah, please do the registration in sysbus.c, not in virt.c.  There is
>>>>> no reason to make the platform_bus_init_notify+DynSysbusNotifier
>>>>> interface public.  The code in sysbus.c can fill in the fields.
>>> Sysbus != Platform bus. Sysbus is an in-QEMU representation of a
>>> pseudo-bus that we put all devices onto that we consider unsorted.
>>>
>>> Platform bus is a machine representation of an actual bus that devices
>>> are attached to. These devices usually are sysbus devices.
>>
>> Is there any difference between the two?
>>
>> Take a machine that has two chips, a SoC that does everything except
>> USB, and a USB controller chip.
>>
>> Strictly speaking the USB controller chip would be on a "platform bus",
>> but we would likely put it on sysbus.
>>
>> Why should it matter whether the devices are static or dynamic, for the
>> sake of calling something the "system" or the "platform" bus?  I would
>> say that QEMU calls "sysbus" the platform bus.
>>
>> Some devices (e.g. the local APIC in x86, or the in-core timers and GIC
>> in ARM) should probably not be in sysbus at all, and should attach
>> directly to the CPU address space.  But that is a quirk in the modeling
>> of those devices, it shouldn't mean that sysbus is not a "platform" bus.
> 
> On e500 for example, we have a predefined CCSR region. That is a machine
> defined "platform bus". The offsets inside that region are strictly
> defined by the spec.
> 
> Now take the serial ports. We have space for 2 serial ports inside of
> that CCSR region. We can spawn these 2 ports in the machine file based
> on -serial, but if you want to spawn them with -device, how do you tell
> the machine whether they should go into the "big bucket platform bus" or
> the "CCSR platform bus"?

Two possibilities:

1) you would use two instances of sysbus (one default, one created by
the board) and specify ",bus=ccsr" on the command line when you want to
add the device to the CCSR region.

The two would work exactly the same way, only with different algorithms
for resource allocation.

2) similar to ISA, you would create a new ccsr-bus device and a new
ccsr-serial device, and use -device ccsr-serial,index=[0|1],chardev=foo
to specify which of the two serial ports this is for.  Most of the fdt
magic could be shared by the sysbus and CCSR cases.

I think I prefer (2)...

Paolo

> In fact, thinking about this a bit more, maybe we should just have an
> actual bus structure. Then we could have the legacy "big bucket" sysbus
> bus that nobody may ever dynamically put devices into. For CCSR we could
> create another bucket that the machine file can control where each
> device goes and can also detect if a device doesn't fit. And then we
> just declare the virt "platform bus" sysbus the default bus for cmdline
> -device sysbusdevice and everything resolves automatically.

Il 10/09/2014 11:56, Alexander Graf ha scritto:
> 
> 
> On 10.09.14 11:43, Paolo Bonzini wrote:
>> Il 10/09/2014 11:31, Alexander Graf ha scritto:
>>>>> Yeah, please do the registration in sysbus.c, not in virt.c.  There is
>>>>> no reason to make the platform_bus_init_notify+DynSysbusNotifier
>>>>> interface public.  The code in sysbus.c can fill in the fields.
>>> Sysbus != Platform bus. Sysbus is an in-QEMU representation of a
>>> pseudo-bus that we put all devices onto that we consider unsorted.
>>>
>>> Platform bus is a machine representation of an actual bus that devices
>>> are attached to. These devices usually are sysbus devices.
>>
>> Is there any difference between the two?
>>
>> Take a machine that has two chips, a SoC that does everything except
>> USB, and a USB controller chip.
>>
>> Strictly speaking the USB controller chip would be on a "platform bus",
>> but we would likely put it on sysbus.
>>
>> Why should it matter whether the devices are static or dynamic, for the
>> sake of calling something the "system" or the "platform" bus?  I would
>> say that QEMU calls "sysbus" the platform bus.
>>
>> Some devices (e.g. the local APIC in x86, or the in-core timers and GIC
>> in ARM) should probably not be in sysbus at all, and should attach
>> directly to the CPU address space.  But that is a quirk in the modeling
>> of those devices, it shouldn't mean that sysbus is not a "platform" bus.
> 
> On e500 for example, we have a predefined CCSR region. That is a machine
> defined "platform bus". The offsets inside that region are strictly
> defined by the spec.
> 
> Now take the serial ports. We have space for 2 serial ports inside of
> that CCSR region. We can spawn these 2 ports in the machine file based
> on -serial, but if you want to spawn them with -device, how do you tell
> the machine whether they should go into the "big bucket platform bus" or
> the "CCSR platform bus"?

Two possibilities:

1) you would use two instances of sysbus (one default, one created by
the board) and specify ",bus=ccsr" on the command line when you want to
add the device to the CCSR region.

The two would work exactly the same way, only with different algorithms
for resource allocation.

2) similar to ISA, you would create a new ccsr-bus device and a new
ccsr-serial device, and use -device ccsr-serial,index=[0|1],chardev=foo
to specify which of the two serial ports this is for.  Perhaps some of
the fdt magic could be shared by the sysbus and CCSR cases.

Paolo

> In fact, thinking about this a bit more, maybe we should just have an
> actual bus structure. Then we could have the legacy "big bucket" sysbus
> bus that nobody may ever dynamically put devices into. For CCSR we could
> create another bucket that the machine file can control where each
> device goes and can also detect if a device doesn't fit. And then we
> just declare the virt "platform bus" sysbus the default bus for cmdline
> -device sysbusdevice and everything resolves automatically.

On 10.09.14 12:05, Paolo Bonzini wrote:
> Il 10/09/2014 11:56, Alexander Graf ha scritto:
>>
>>
>> On 10.09.14 11:43, Paolo Bonzini wrote:
>>> Il 10/09/2014 11:31, Alexander Graf ha scritto:
>>>>>> Yeah, please do the registration in sysbus.c, not in virt.c.  There is
>>>>>> no reason to make the platform_bus_init_notify+DynSysbusNotifier
>>>>>> interface public.  The code in sysbus.c can fill in the fields.
>>>> Sysbus != Platform bus. Sysbus is an in-QEMU representation of a
>>>> pseudo-bus that we put all devices onto that we consider unsorted.
>>>>
>>>> Platform bus is a machine representation of an actual bus that devices
>>>> are attached to. These devices usually are sysbus devices.
>>>
>>> Is there any difference between the two?
>>>
>>> Take a machine that has two chips, a SoC that does everything except
>>> USB, and a USB controller chip.
>>>
>>> Strictly speaking the USB controller chip would be on a "platform bus",
>>> but we would likely put it on sysbus.
>>>
>>> Why should it matter whether the devices are static or dynamic, for the
>>> sake of calling something the "system" or the "platform" bus?  I would
>>> say that QEMU calls "sysbus" the platform bus.
>>>
>>> Some devices (e.g. the local APIC in x86, or the in-core timers and GIC
>>> in ARM) should probably not be in sysbus at all, and should attach
>>> directly to the CPU address space.  But that is a quirk in the modeling
>>> of those devices, it shouldn't mean that sysbus is not a "platform" bus.
>>
>> On e500 for example, we have a predefined CCSR region. That is a machine
>> defined "platform bus". The offsets inside that region are strictly
>> defined by the spec.
>>
>> Now take the serial ports. We have space for 2 serial ports inside of
>> that CCSR region. We can spawn these 2 ports in the machine file based
>> on -serial, but if you want to spawn them with -device, how do you tell
>> the machine whether they should go into the "big bucket platform bus" or
>> the "CCSR platform bus"?
> 
> Two possibilities:
> 
> 1) you would use two instances of sysbus (one default, one created by
> the board) and specify ",bus=ccsr" on the command line when you want to
> add the device to the CCSR region.
> 
> The two would work exactly the same way, only with different algorithms
> for resource allocation.
> 
> 2) similar to ISA, you would create a new ccsr-bus device and a new
> ccsr-serial device, and use -device ccsr-serial,index=[0|1],chardev=foo
> to specify which of the two serial ports this is for.  Most of the fdt
> magic could be shared by the sysbus and CCSR cases.
> 
> I think I prefer (2)...

Fair enough.

As far as moving "platform bus" logic into sysbus, I'd really like to
hold back and see what this whole thing ends up getting used for first.

So for now, I'd definitely prefer to keep "platform bus" logic and
"sysbus" logic separate. If we realize that every user only ever uses
the dynamic sysbus creation in conjunction with our "platform bus"
implementation, we can merge them.


Alex

Il 10/09/2014 12:09, Alexander Graf ha scritto:
> Fair enough.
> 
> As far as moving "platform bus" logic into sysbus, I'd really like to
> hold back and see what this whole thing ends up getting used for first.
> 
> So for now, I'd definitely prefer to keep "platform bus" logic and
> "sysbus" logic separate. If we realize that every user only ever uses
> the dynamic sysbus creation in conjunction with our "platform bus"
> implementation, we can merge them.

I agree.  As you pointed out, we have two usecases:

1) arbitrary dynamic sysbus devices, because you're playing with board
design or because you're working on a virtualized platform

2) pluggable components in a fixed board design (e.g. CCSR)

The only thing they share is FDT creation.  The other part, which is
assigning the interrupts and memory regions, is different: case (1) has
it driven by command line or simply bottom-to-top; case (2) has it
driven by an implementation of a spec.

It's not even clear to me that E500 CCSR devices should be sysbus, in
fact...

Paolo

On 10.09.14 12:21, Paolo Bonzini wrote:
> Il 10/09/2014 12:09, Alexander Graf ha scritto:
>> Fair enough.
>>
>> As far as moving "platform bus" logic into sysbus, I'd really like to
>> hold back and see what this whole thing ends up getting used for first.
>>
>> So for now, I'd definitely prefer to keep "platform bus" logic and
>> "sysbus" logic separate. If we realize that every user only ever uses
>> the dynamic sysbus creation in conjunction with our "platform bus"
>> implementation, we can merge them.
> 
> I agree.  As you pointed out, we have two usecases:
> 
> 1) arbitrary dynamic sysbus devices, because you're playing with board
> design or because you're working on a virtualized platform
> 
> 2) pluggable components in a fixed board design (e.g. CCSR)
> 
> The only thing they share is FDT creation.  The other part, which is
> assigning the interrupts and memory regions, is different: case (1) has
> it driven by command line or simply bottom-to-top; case (2) has it
> driven by an implementation of a spec.
> 
> It's not even clear to me that E500 CCSR devices should be sysbus, in
> fact...

The problem if you continue that thought process is that we'd end up
with 500 different buses and 500 different uart boilerplate devices just
to fit into the respective buses ;).

Otherwise I agree.


Alex

Il 10/09/2014 12:26, Alexander Graf ha scritto:
> > It's not even clear to me that E500 CCSR devices should be sysbus, in
> > fact...
>
> The problem if you continue that thought process is that we'd end up
> with 500 different buses and 500 different uart boilerplate devices just
> to fit into the respective buses ;).

True.  The alternative is to hardcode the knowledge of the spec in the
management layers (since you cannot do index=0|1, you have to do
something akin to iobase=0x3f8 for the x86 COM1 port).  I guess you will
still need two sysbuses so that you get the correct hierarchy in the
device tree, right?

And we're back to the beginning of the discussion: the distinction
between a "sysbus" and a "platform bus" disappears, and in fact it even
feels more accurate to just call these things "sysbuses"...

Paolo

On 09/10/2014 12:09 PM, Alexander Graf wrote:
> 
> 
> On 10.09.14 12:05, Paolo Bonzini wrote:
>> Il 10/09/2014 11:56, Alexander Graf ha scritto:
>>>
>>>
>>> On 10.09.14 11:43, Paolo Bonzini wrote:
>>>> Il 10/09/2014 11:31, Alexander Graf ha scritto:
>>>>>>> Yeah, please do the registration in sysbus.c, not in virt.c.  There is
>>>>>>> no reason to make the platform_bus_init_notify+DynSysbusNotifier
>>>>>>> interface public.  The code in sysbus.c can fill in the fields.
>>>>> Sysbus != Platform bus. Sysbus is an in-QEMU representation of a
>>>>> pseudo-bus that we put all devices onto that we consider unsorted.
>>>>>
>>>>> Platform bus is a machine representation of an actual bus that devices
>>>>> are attached to. These devices usually are sysbus devices.
>>>>
>>>> Is there any difference between the two?
>>>>
>>>> Take a machine that has two chips, a SoC that does everything except
>>>> USB, and a USB controller chip.
>>>>
>>>> Strictly speaking the USB controller chip would be on a "platform bus",
>>>> but we would likely put it on sysbus.
>>>>
>>>> Why should it matter whether the devices are static or dynamic, for the
>>>> sake of calling something the "system" or the "platform" bus?  I would
>>>> say that QEMU calls "sysbus" the platform bus.
>>>>
>>>> Some devices (e.g. the local APIC in x86, or the in-core timers and GIC
>>>> in ARM) should probably not be in sysbus at all, and should attach
>>>> directly to the CPU address space.  But that is a quirk in the modeling
>>>> of those devices, it shouldn't mean that sysbus is not a "platform" bus.
>>>
>>> On e500 for example, we have a predefined CCSR region. That is a machine
>>> defined "platform bus". The offsets inside that region are strictly
>>> defined by the spec.
>>>
>>> Now take the serial ports. We have space for 2 serial ports inside of
>>> that CCSR region. We can spawn these 2 ports in the machine file based
>>> on -serial, but if you want to spawn them with -device, how do you tell
>>> the machine whether they should go into the "big bucket platform bus" or
>>> the "CCSR platform bus"?
>>
>> Two possibilities:
>>
>> 1) you would use two instances of sysbus (one default, one created by
>> the board) and specify ",bus=ccsr" on the command line when you want to
>> add the device to the CCSR region.
>>
>> The two would work exactly the same way, only with different algorithms
>> for resource allocation.
>>
>> 2) similar to ISA, you would create a new ccsr-bus device and a new
>> ccsr-serial device, and use -device ccsr-serial,index=[0|1],chardev=foo
>> to specify which of the two serial ports this is for.  Most of the fdt
>> magic could be shared by the sysbus and CCSR cases.
>>
>> I think I prefer (2)...
> 
> Fair enough.
> 
> As far as moving "platform bus" logic into sysbus, I'd really like to
> hold back and see what this whole thing ends up getting used for first.
> 
> So for now, I'd definitely prefer to keep "platform bus" logic and
> "sysbus" logic separate. If we realize that every user only ever uses
> the dynamic sysbus creation in conjunction with our "platform bus"
> implementation, we can merge them.

Hi Paolo, Alex,

I understand I keep the code in a separate module from sysbus.c. Is that
the shared conclusion?

Thanks

Best Regards

Eric
> 
> 
> Alex
>

Il 10/09/2014 15:51, Eric Auger ha scritto:
> On 09/10/2014 12:09 PM, Alexander Graf wrote:
>>
>>
>> On 10.09.14 12:05, Paolo Bonzini wrote:
>>> Il 10/09/2014 11:56, Alexander Graf ha scritto:
>>>>
>>>>
>>>> On 10.09.14 11:43, Paolo Bonzini wrote:
>>>>> Il 10/09/2014 11:31, Alexander Graf ha scritto:
>>>>>>>> Yeah, please do the registration in sysbus.c, not in virt.c.  There is
>>>>>>>> no reason to make the platform_bus_init_notify+DynSysbusNotifier
>>>>>>>> interface public.  The code in sysbus.c can fill in the fields.
>>>>>> Sysbus != Platform bus. Sysbus is an in-QEMU representation of a
>>>>>> pseudo-bus that we put all devices onto that we consider unsorted.
>>>>>>
>>>>>> Platform bus is a machine representation of an actual bus that devices
>>>>>> are attached to. These devices usually are sysbus devices.
>>>>>
>>>>> Is there any difference between the two?
>>>>>
>>>>> Take a machine that has two chips, a SoC that does everything except
>>>>> USB, and a USB controller chip.
>>>>>
>>>>> Strictly speaking the USB controller chip would be on a "platform bus",
>>>>> but we would likely put it on sysbus.
>>>>>
>>>>> Why should it matter whether the devices are static or dynamic, for the
>>>>> sake of calling something the "system" or the "platform" bus?  I would
>>>>> say that QEMU calls "sysbus" the platform bus.
>>>>>
>>>>> Some devices (e.g. the local APIC in x86, or the in-core timers and GIC
>>>>> in ARM) should probably not be in sysbus at all, and should attach
>>>>> directly to the CPU address space.  But that is a quirk in the modeling
>>>>> of those devices, it shouldn't mean that sysbus is not a "platform" bus.
>>>>
>>>> On e500 for example, we have a predefined CCSR region. That is a machine
>>>> defined "platform bus". The offsets inside that region are strictly
>>>> defined by the spec.
>>>>
>>>> Now take the serial ports. We have space for 2 serial ports inside of
>>>> that CCSR region. We can spawn these 2 ports in the machine file based
>>>> on -serial, but if you want to spawn them with -device, how do you tell
>>>> the machine whether they should go into the "big bucket platform bus" or
>>>> the "CCSR platform bus"?
>>>
>>> Two possibilities:
>>>
>>> 1) you would use two instances of sysbus (one default, one created by
>>> the board) and specify ",bus=ccsr" on the command line when you want to
>>> add the device to the CCSR region.
>>>
>>> The two would work exactly the same way, only with different algorithms
>>> for resource allocation.
>>>
>>> 2) similar to ISA, you would create a new ccsr-bus device and a new
>>> ccsr-serial device, and use -device ccsr-serial,index=[0|1],chardev=foo
>>> to specify which of the two serial ports this is for.  Most of the fdt
>>> magic could be shared by the sysbus and CCSR cases.
>>>
>>> I think I prefer (2)...
>>
>> Fair enough.
>>
>> As far as moving "platform bus" logic into sysbus, I'd really like to
>> hold back and see what this whole thing ends up getting used for first.
>>
>> So for now, I'd definitely prefer to keep "platform bus" logic and
>> "sysbus" logic separate. If we realize that every user only ever uses
>> the dynamic sysbus creation in conjunction with our "platform bus"
>> implementation, we can merge them.
> 
> Hi Paolo, Alex,
> 
> I understand I keep the code in a separate module from sysbus.c. Is that
> the shared conclusion?

I don't think so, but maybe I misunderstood what Alex wrote.

Paolo

> Am 10.09.2014 um 16:18 schrieb Paolo Bonzini <pbonzini@redhat.com>:
> 
> Il 10/09/2014 15:51, Eric Auger ha scritto:
>>> On 09/10/2014 12:09 PM, Alexander Graf wrote:
>>> 
>>> 
>>>> On 10.09.14 12:05, Paolo Bonzini wrote:
>>>> Il 10/09/2014 11:56, Alexander Graf ha scritto:
>>>>> 
>>>>> 
>>>>>> On 10.09.14 11:43, Paolo Bonzini wrote:
>>>>>> Il 10/09/2014 11:31, Alexander Graf ha scritto:
>>>>>>>>> Yeah, please do the registration in sysbus.c, not in virt.c.  There is
>>>>>>>>> no reason to make the platform_bus_init_notify+DynSysbusNotifier
>>>>>>>>> interface public.  The code in sysbus.c can fill in the fields.
>>>>>>> Sysbus != Platform bus. Sysbus is an in-QEMU representation of a
>>>>>>> pseudo-bus that we put all devices onto that we consider unsorted.
>>>>>>> 
>>>>>>> Platform bus is a machine representation of an actual bus that devices
>>>>>>> are attached to. These devices usually are sysbus devices.
>>>>>> 
>>>>>> Is there any difference between the two?
>>>>>> 
>>>>>> Take a machine that has two chips, a SoC that does everything except
>>>>>> USB, and a USB controller chip.
>>>>>> 
>>>>>> Strictly speaking the USB controller chip would be on a "platform bus",
>>>>>> but we would likely put it on sysbus.
>>>>>> 
>>>>>> Why should it matter whether the devices are static or dynamic, for the
>>>>>> sake of calling something the "system" or the "platform" bus?  I would
>>>>>> say that QEMU calls "sysbus" the platform bus.
>>>>>> 
>>>>>> Some devices (e.g. the local APIC in x86, or the in-core timers and GIC
>>>>>> in ARM) should probably not be in sysbus at all, and should attach
>>>>>> directly to the CPU address space.  But that is a quirk in the modeling
>>>>>> of those devices, it shouldn't mean that sysbus is not a "platform" bus.
>>>>> 
>>>>> On e500 for example, we have a predefined CCSR region. That is a machine
>>>>> defined "platform bus". The offsets inside that region are strictly
>>>>> defined by the spec.
>>>>> 
>>>>> Now take the serial ports. We have space for 2 serial ports inside of
>>>>> that CCSR region. We can spawn these 2 ports in the machine file based
>>>>> on -serial, but if you want to spawn them with -device, how do you tell
>>>>> the machine whether they should go into the "big bucket platform bus" or
>>>>> the "CCSR platform bus"?
>>>> 
>>>> Two possibilities:
>>>> 
>>>> 1) you would use two instances of sysbus (one default, one created by
>>>> the board) and specify ",bus=ccsr" on the command line when you want to
>>>> add the device to the CCSR region.
>>>> 
>>>> The two would work exactly the same way, only with different algorithms
>>>> for resource allocation.
>>>> 
>>>> 2) similar to ISA, you would create a new ccsr-bus device and a new
>>>> ccsr-serial device, and use -device ccsr-serial,index=[0|1],chardev=foo
>>>> to specify which of the two serial ports this is for.  Most of the fdt
>>>> magic could be shared by the sysbus and CCSR cases.
>>>> 
>>>> I think I prefer (2)...
>>> 
>>> Fair enough.
>>> 
>>> As far as moving "platform bus" logic into sysbus, I'd really like to
>>> hold back and see what this whole thing ends up getting used for first.
>>> 
>>> So for now, I'd definitely prefer to keep "platform bus" logic and
>>> "sysbus" logic separate. If we realize that every user only ever uses
>>> the dynamic sysbus creation in conjunction with our "platform bus"
>>> implementation, we can merge them.
>> 
>> Hi Paolo, Alex,
>> 
>> I understand I keep the code in a separate module from sysbus.c. Is that
>> the shared conclusion?
> 
> I don't think so, but maybe I misunderstood what Alex wrote.

I still think it shouldn't be inside the sysbus files. You can use sysbus devices without a platform bus.

Alex

> 
> Paolo
>

Il 10/09/2014 16:38, Alexander Graf ha scritto:
>> > I don't think so, but maybe I misunderstood what Alex wrote.
> I still think it shouldn't be inside the sysbus files. You can use sysbus devices without a platform bus.

If you want to separate the files, that's fine.  It should still be in
hw/core.

Paolo

> Am 10.09.2014 um 16:39 schrieb Paolo Bonzini <pbonzini@redhat.com>:
> 
> Il 10/09/2014 16:38, Alexander Graf ha scritto:
>>>> I don't think so, but maybe I misunderstood what Alex wrote.
>> I still think it shouldn't be inside the sysbus files. You can use sysbus devices without a platform bus.
> 
> If you want to separate the files, that's fine.  It should still be in
> hw/core.

Agreed :)

Alex

> 
> Paolo