From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60855) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X2O52-0005tv-U1 for qemu-devel@nongnu.org; Wed, 02 Jul 2014 13:12:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X2O4u-0005oD-FE for qemu-devel@nongnu.org; Wed, 02 Jul 2014 13:12:24 -0400 Message-ID: <53B43D6C.90903@suse.de> Date: Wed, 02 Jul 2014 19:12:12 +0200 From: Alexander Graf MIME-Version: 1.0 References: <1404251378-5242-1-git-send-email-agraf@suse.de> <1404251378-5242-6-git-send-email-agraf@suse.de> <1404255054.21434.7.camel@snotra.buserror.net> In-Reply-To: <1404255054.21434.7.camel@snotra.buserror.net> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 5/6] PPC: e500: Support dynamically spawned sysbus devices List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Scott Wood Cc: peter.maydell@linaro.org, peter.crosthwaite@xilinx.com, eric.auger@linaro.org, qemu-devel@nongnu.org, qemu-ppc@nongnu.org, sean.stalley@intel.com, pbonzini@redhat.com, afaerber@suse.de On 02.07.14 00:50, Scott Wood wrote: > On Tue, 2014-07-01 at 23:49 +0200, Alexander Graf wrote: >> For e500 our approach to supporting dynamically spawned sysbus devices is to >> create a simple bus from the guest's point of view within which we map those >> 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 >> --- >> hw/ppc/e500.c | 251 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> hw/ppc/e500.h | 1 + >> hw/ppc/e500plat.c | 1 + >> 3 files changed, 253 insertions(+) >> >> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c >> index bb2e75f..bf704b0 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 "qemu/error-report.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 > Should the IRQ and address range be parameterized? Even if the platform > bus is going to be restricted to only e500plat, it seems like it would > be good to keep all assumptions that are e500plat-specific inside the > e500plat file. Good idea. The only thing I'll leave here is the page_shift. The fact that we only allocate in 4k chunks is inherently implementation specific. > Plus, let's please not hardcode any more addresses that are going to be > a problem for giving guests a large amount of RAM (yes, CCSRBAR is also > blocking that, but that has a TODO to parameterize). How about > 0xf00000000ULL? Unless of course we're emulating an e500v1, which > doesn't support 36-bit physical addressing. Parameterization would help > there as well. I don't think we have to worry about e500v1. I'll change it :). > >> +#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 > What is the "hole"? If that's meant to be the size to go along with > E500_PLATFORM_BASE, that seems like odd terminology. True - renamed to "size". > >> + >> /* TODO: parameterize */ >> #define MPC8544_CCSRBAR_BASE 0xE0000000ULL >> #define MPC8544_CCSRBAR_SIZE 0x00100000ULL >> @@ -122,6 +131,77 @@ 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; >> + int id; >> +} PlatformDevtreeData; > What is id? How does irq_start work? "id" is just a linear counter over all devices in the platform bus so that if you need to have a unique identifier, you can have one. "irq_start" is the offset of the first mpic irq that's connected to the platform bus. > >> +static int sysbus_device_create_devtree(Object *obj, void *opaque) >> +{ >> + PlatformDevtreeData *data = opaque; >> + Object *dev; >> + SysBusDevice *sbdev; >> + bool matched = false; >> + >> + 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_create_devtree, data); >> + } >> + >> + if (matched) { >> + data->id++; >> + } else { >> + error_report("Device %s is not supported by this machine yet.", >> + qdev_fw_name(DEVICE(dev))); >> + exit(1); >> + } >> + >> + return 0; >> +} > It's not clear to me how this function is creating a device tree node. It's not yet - it's only the stub that allows to plug in specific device code that then generates device tree nodes :). > >> + >> +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; >> + Object *container; >> + >> + /* 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"); > Where did this device_type come from? > > device_type is deprecated and new uses should not be introduced. Fair enough, will remove it :) > >> 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; > It would be clearer to refer to this as "platform bus", "platform > devices", or similar rather than just "platform" -- the latter is > confusing relative to existing use of the word "platform", such as > e500plat.c. Renamed to "platform_bus" :). In fact, I'll go through the file with a small sweeper and try to make sure we're reasonably consistent in our naming. Alex