From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35399) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z4mU6-0000cp-7J for qemu-devel@nongnu.org; Tue, 16 Jun 2015 04:44:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z4mU1-0004F9-OK for qemu-devel@nongnu.org; Tue, 16 Jun 2015 04:44:42 -0400 Received: from mail-wg0-f54.google.com ([74.125.82.54]:34514) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z4mU1-0004F1-G3 for qemu-devel@nongnu.org; Tue, 16 Jun 2015 04:44:37 -0400 Received: by wgv5 with SMTP id 5so6927857wgv.1 for ; Tue, 16 Jun 2015 01:44:36 -0700 (PDT) Message-ID: <557FE1E3.9060706@linaro.org> Date: Tue, 16 Jun 2015 10:44:19 +0200 From: Eric Auger MIME-Version: 1.0 References: <1434386038-9246-1-git-send-email-eric.auger@linaro.org> <1434386038-9246-2-git-send-email-eric.auger@linaro.org> In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RESEND PATCH v16 1/6] hw/arm/sysbus-fdt: enable vfio-calxeda-xgmac dynamic instantiation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: b.reynal@virtualopensystems.com, Peter Crosthwaite , eric.auger@st.com, vikrams@codeaurora.org, Patch Tracking , QEMU Developers , Alex Williamson , Paolo Bonzini , Christoffer Dall On 06/16/2015 10:29 AM, Peter Maydell wrote: > On 15 June 2015 at 17:33, Eric Auger wrote: >> This patch allows the instantiation of the vfio-calxeda-xgmac device >> from the QEMU command line (-device vfio-calxeda-xgmac,host=""). >> >> A specialized device tree node is created for the guest, containing >> compat, dma-coherent, reg and interrupts properties. >> >> Signed-off-by: Eric Auger >> > > Minor nits: > >> +/* Device Specific Code */ >> + >> +/** >> + * add_calxeda_midway_xgmac_fdt_node >> + * >> + * Generates a simple node with following properties: >> + * compatible string, regs, interrupts, dma-coherent >> + */ >> +static int add_calxeda_midway_xgmac_fdt_node(SysBusDevice *sbdev, void *opaque) >> +{ >> + PlatformBusFDTData *data = opaque; >> + PlatformBusDevice *pbus = data->pbus; >> + void *fdt = data->fdt; >> + const char *parent_node = data->pbus_node_name; >> + int compat_str_len, i, ret = -1; >> + char *nodename; >> + uint32_t *irq_attr, *reg_attr; >> + uint64_t mmio_base, irq_number; >> + VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev); >> + VFIODevice *vbasedev = &vdev->vbasedev; >> + >> + mmio_base = platform_bus_get_mmio_addr(pbus, sbdev, 0); >> + 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; >> + qemu_fdt_setprop(fdt, nodename, "compatible", >> + vdev->compat, compat_str_len); >> + >> + qemu_fdt_setprop(fdt, nodename, "dma-coherent", "", 0); >> + >> + reg_attr = g_new(uint32_t, vbasedev->num_regions*2); > > You should have spaces around the '*' operator here (and in a > couple of other places below). OK > >> + for (i = 0; i < vbasedev->num_regions; i++) { >> + mmio_base = platform_bus_get_mmio_addr(pbus, sbdev, i); >> + reg_attr[2*i] = cpu_to_be32(mmio_base); >> + reg_attr[2*i+1] = cpu_to_be32( >> + memory_region_size(&vdev->regions[i]->mem)); >> + } >> + ret = qemu_fdt_setprop(fdt, nodename, "reg", reg_attr, >> + vbasedev->num_regions*2*sizeof(uint32_t)); >> + if (ret) { >> + error_report("could not set reg property of node %s", nodename); >> + goto fail_reg; >> + } >> + >> + irq_attr = g_new(uint32_t, vbasedev->num_irqs*3); >> + for (i = 0; i < vbasedev->num_irqs; i++) { >> + irq_number = platform_bus_get_irqn(pbus, sbdev , i) >> + + data->irq_start; >> + irq_attr[3*i] = cpu_to_be32(0); >> + irq_attr[3*i+1] = cpu_to_be32(irq_number); >> + irq_attr[3*i+2] = cpu_to_be32(0x4); > > In virt.c the magic numbers in cells 0 and 2 have #defines > (GIC_FDT_IRQ_TYPE_SPI and GIC_FDT_IRQ_FLAGS_LEVEL_HI). > > (1) maybe we should pull those out into a header so you can use them here OK maybe I can put them in newly created include/hw/arm/virt.h > (2) is this device really using a level-triggered interrupt? yes it does > >> + } >> + ret = qemu_fdt_setprop(fdt, nodename, "interrupts", >> + irq_attr, vbasedev->num_irqs*3*sizeof(uint32_t)); >> + if (ret) { >> + error_report("could not set interrupts property of node %s", >> + nodename); >> + } >> + g_free(irq_attr); >> +fail_reg: >> + g_free(reg_attr); >> + g_free(nodename); >> + return ret; >> +} >> + >> /* list of supported dynamic sysbus devices */ >> static const NodeCreationPair add_fdt_node_functions[] = { >> + {TYPE_VFIO_CALXEDA_XGMAC, add_calxeda_midway_xgmac_fdt_node}, >> {"", NULL}, /* last element */ >> }; > > This file could get big if we have a lot of platform passthrough > devices, but let's hope we don't... Yes let's see how this feature gets used ;-) > > Otherwise: > Acked-by: Peter Maydell If you allow me I will resend the patch file separately with the above modifications. Thanks! Best Regards Eric > > -- PMM >