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

[Alexander Graf <agraf@suse.de>]

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