From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40846) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XJdn8-0001Ps-9z for qemu-devel@nongnu.org; Tue, 19 Aug 2014 03:25:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XJdn2-0006lK-1X for qemu-devel@nongnu.org; Tue, 19 Aug 2014 03:25:14 -0400 Received: from mail-we0-f177.google.com ([74.125.82.177]:36747) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XJdn1-0006l1-P1 for qemu-devel@nongnu.org; Tue, 19 Aug 2014 03:25:07 -0400 Received: by mail-we0-f177.google.com with SMTP id w62so5882313wes.8 for ; Tue, 19 Aug 2014 00:25:07 -0700 (PDT) Message-ID: <53F2FBC5.7000409@linaro.org> Date: Tue, 19 Aug 2014 09:24:53 +0200 From: Eric Auger MIME-Version: 1.0 References: <1407594349-9291-1-git-send-email-eric.auger@linaro.org> <1407594349-9291-11-git-send-email-eric.auger@linaro.org> <53F27627.4030401@amd.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v5 10/10] hw/arm/dyn_sysbus_devtree: enable simple VFIO dynamic instantiation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell , Joel Schopp Cc: Alexander Graf , Kim Phillips , eric.auger@st.com, Patch Tracking , Will Deacon , QEMU Developers , Alvise Rigo , Bharat Bhushan , Alex Williamson , Stuart Yoder , Antonios Motakis , "kvmarm@lists.cs.columbia.edu" , Christoffer Dall On 08/19/2014 12:11 AM, Peter Maydell wrote: > On 18 August 2014 22:54, Joel Schopp wrote: >> >> +static void vfio_fdt_add_device_node(SysBusDevice *sbdev, void *opaque) >> +{ >> + PlatformDevtreeData *data = opaque; >> + void *fdt = data->fdt; >> + const char *parent_node = data->node; >> + int compat_str_len; >> + char *nodename; >> + int i, ret; >> + uint32_t *irq_attr; >> + uint64_t *reg_attr; >> + uint64_t mmio_base; >> + uint64_t irq_number; >> + gchar mmio_base_prop[8]; >> + gchar irq_number_prop[8]; >> + VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev); >> + VFIODevice *vbasedev = &vdev->vbasedev; >> + Object *obj = OBJECT(sbdev); >> + >> + mmio_base = object_property_get_int(obj, "mmio[0]", NULL); >> + >> + nodename = g_strdup_printf("%s/%s@%" PRIx64, parent_node, >> + vbasedev->name, >> + mmio_base); >> + >> + qemu_fdt_add_subnode(fdt, nodename); >> + >> + compat_str_len = strlen(vdev->compat) + 1; > > At this point you've already substituted the NULs in, > so you can't call strlen(), I think. Hi Peter, yes you're right. Thanks > >> + qemu_fdt_setprop(fdt, nodename, "compatible", >> + vdev->compat, compat_str_len); >> + >> + reg_attr = g_new(uint64_t, vbasedev->num_regions*4); >> + >> + for (i = 0; i < vbasedev->num_regions; i++) { >> + snprintf(mmio_base_prop, sizeof(mmio_base_prop), "mmio[%d]", i); >> + mmio_base = object_property_get_int(obj, mmio_base_prop, NULL); >> + reg_attr[2*i] = 1; >> + reg_attr[2*i+1] = mmio_base; >> + reg_attr[2*i+2] = 1; >> + reg_attr[2*i+3] = memory_region_size(&vdev->regions[i]->mem); >> + } >> >> This should be 4 instead of 2. >> Also, to support 64 bit systems I think this should be 2 instead of 1. > > Actually it depends entirely on what the board has done to > create the device tree node that we're inserting this child > node into. For ARM boot.c sets both #address-cells and > #size-cells to 2 regardless of whether the system is 32 > or 64 bits, for simplicity. I imagine PPC does something > different. If we're editing a dtb that the user passed in (which > I think would be pretty lunatic so we shouldn't do this) > we'd have to actually walk the dtb to try to figure out what > the semantics of the reg property should be. Putting size=1 was the only solution I found to use an offset relative to the parent bus instead of an absolute base address. I would explain this because, in platform_bus_create_devtree, the function that creates the "platform bus" node, #address-cells and #size-cells currently are set to 1. I assume the motivation was that bus size was supposed to be smaller than 4GB. Then I guess the problem is shifted to the inclusion of the platform bus in any ARM platform. Thanks Eric > > thanks > -- PMM >