From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36479) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X0ZMJ-0001zb-AY for qemu-devel@nongnu.org; Fri, 27 Jun 2014 12:50:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X0ZMC-0004xX-G2 for qemu-devel@nongnu.org; Fri, 27 Jun 2014 12:50:43 -0400 Received: from mail-we0-f171.google.com ([74.125.82.171]:56100) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X0ZMC-0004wb-68 for qemu-devel@nongnu.org; Fri, 27 Jun 2014 12:50:36 -0400 Received: by mail-we0-f171.google.com with SMTP id q58so5577600wes.30 for ; Fri, 27 Jun 2014 09:50:35 -0700 (PDT) Message-ID: <53ADA0C7.4050608@linaro.org> Date: Fri, 27 Jun 2014 18:50:15 +0200 From: Eric Auger MIME-Version: 1.0 References: <1401884936-12907-1-git-send-email-agraf@suse.de> <1401884936-12907-5-git-send-email-agraf@suse.de> <53AD3983.8040300@linaro.org> <53AD55F3.5040705@suse.de> In-Reply-To: <53AD55F3.5040705@suse.de> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 4/5] PPC: e500: Support platform devices List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf , qemu-ppc@nongnu.org Cc: qemu-devel@nongnu.org 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 >>> --- >>> 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 >