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 <address@hidden>
>>> ---
>>>   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
>