From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41974) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X2Ooi-0008Hv-Os for qemu-devel@nongnu.org; Wed, 02 Jul 2014 13:59:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X2OoY-0003gd-6N for qemu-devel@nongnu.org; Wed, 02 Jul 2014 13:59:36 -0400 Message-ID: <53B4487B.2070109@suse.de> Date: Wed, 02 Jul 2014 19:59:23 +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> <53B43D6C.90903@suse.de> <1404321984.21434.24.camel@snotra.buserror.net> <53B44198.5050307@suse.de> <1404323576.21434.41.camel@snotra.buserror.net> In-Reply-To: <1404323576.21434.41.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 19:52, Scott Wood wrote: > On Wed, 2014-07-02 at 19:30 +0200, Alexander Graf wrote: >> On 02.07.14 19:26, Scott Wood wrote: >>> On Wed, 2014-07-02 at 19:12 +0200, Alexander Graf wrote: >>>> On 02.07.14 00:50, Scott Wood wrote: >>>>> 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 :). >>> We theoretically support it elsewhere... Once parameterized, it >>> shouldn't be hard to base the address for this, CCSRBAR, and similar >>> things on whether MAS7 is supported. >> It gets parametrized in the machine file, CPU selection comes after >> machine selection. So parameterizing it doesn't really solve it. > Why can't e500plat_init() look at args->cpu_model? Or the > parameterization could take two sets of addresses, one for a 32-bit > layout and one for a 36-bit layout. It might make sense to make this a > user-settable parameter; some OSes might not be able to handle a 36-bit > layout (or might not be as efficient at handling it) even on e500v2. > Many of the e500v2 boards can be built for either a 32 or 36 bit address > layout in U-Boot. > >> However, again, I don't think we have to worry about it. > It's not a huge worry, but it'd be nice to not break it gratuitously. > If we do break it we should explicitly disallow e500v1 with e500plat. I'd prefer if we don't overparameterize - it'll just become a headache further down. Today we don't explicitly disallow anything anywhere - you could theoretically stick a G3 into e500plat. I don't see why we should start with heavy sanity checks now :). Plus, the machine works just fine today if you don't pass in -device eTSEC. It's not like we're moving all devices to the new "platform bus". > >>>>>> @@ -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. >>> OK, but why is that here but no irq_end, and no address range? How do >>> allocations from the irq range happen? >> There are 2 phases: >> >> 1) Device association with the machine >> 2) Device tree generation >> >> The allocation of IRQ ranges happens during the association phase. That >> phase also updates all the hints in the devices to reflect their current >> IRQ (and MMIO) mappings. The device tree generation phase only needs to >> read those bits then - and add the IRQ offset to get from the "platform >> bus IRQ range" to "MPIC IRQ range". > I think the answer to my original question is that irqs are allocated > based on zero because they go in an array, while memory regions are > allocated with their actual addresses because they don't. Memory regions are allocated based on zero as well, they get mapped into a subregion. From a device's point of view, the regions for MMIO and IRQs that it sees all start at 0 relative 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 :). >>> How does the plugging in work? >>> >>> It looks like all this does is increment id. >> I'm not sure I understand. The plugging in is different code :). This >> really only does increment an id. Maybe I'll just remove it if it >> confuses you? > My confusion is that it is called sysbus_device_create_devtree(), not > sysbus_device_alloc_id(). Am I missing some sort of virtual function > mechanism here that would allow this function to be replaced? I've removed the id bit - hope that makes it more obvious :). > > /me looks at patch 6/6 again > > Oh, you just add to this function in future patches. I was expecting > something fancier given the QOM stuff and my misunderstanding about what > file patch 6/6 was touching. :-) Heh, yeah, nothing impressively fancy :). Alex