From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39491) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XmItR-0007G1-PE for qemu-devel@nongnu.org; Thu, 06 Nov 2014 03:58:19 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XmItM-0007Nd-Cb for qemu-devel@nongnu.org; Thu, 06 Nov 2014 03:58:13 -0500 Received: from mail-wi0-f171.google.com ([209.85.212.171]:34775) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XmItM-0007NK-3o for qemu-devel@nongnu.org; Thu, 06 Nov 2014 03:58:08 -0500 Received: by mail-wi0-f171.google.com with SMTP id r20so775934wiv.10 for ; Thu, 06 Nov 2014 00:58:06 -0800 (PST) Message-ID: <545B37E0.3030800@linaro.org> Date: Thu, 06 Nov 2014 09:57:04 +0100 From: Eric Auger MIME-Version: 1.0 References: <1414764350-5140-1-git-send-email-eric.auger@linaro.org> <1414764350-5140-13-git-send-email-eric.auger@linaro.org> <545A0306.3090601@suse.de> <545A18BF.3050001@linaro.org> <545AA34B.4050303@suse.de> In-Reply-To: <545AA34B.4050303@suse.de> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v7 12/16] hw/arm/sysbus-fdt: enable vfio-calxeda-xgmac dynamic instantiation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf , eric.auger@st.com, christoffer.dall@linaro.org, qemu-devel@nongnu.org, pbonzini@redhat.com, kim.phillips@freescale.com, a.rigo@virtualopensystems.com, manish.jaggi@caviumnetworks.com, joel.schopp@amd.com Cc: peter.maydell@linaro.org, patches@linaro.org, will.deacon@arm.com, stuart.yoder@freescale.com, Bharat.Bhushan@freescale.com, alex.williamson@redhat.com, a.motakis@virtualopensystems.com, kvmarm@lists.cs.columbia.edu On 11/05/2014 11:23 PM, Alexander Graf wrote: > > > On 05.11.14 13:31, Eric Auger wrote: >> On 11/05/2014 11:59 AM, Alexander Graf wrote: >>> >>> >>> On 31.10.14 15:05, Eric Auger wrote: >>>> vfio-calxeda-xgmac now can be instantiated using the -device option. >>>> The node creation function generates a very basic dt node composed >>>> of the compat, reg and interrupts properties >>>> >>>> Signed-off-by: Eric Auger >>>> >>>> --- >>>> >>>> v6 -> v7: >>>> - compat string re-formatting removed since compat string is not exposed >>>> anymore as a user option >>>> - VFIO IRQ kick-off removed from sysbus-fdt and moved to VFIO platform >>>> device >>>> --- >>>> hw/arm/sysbus-fdt.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 88 insertions(+) >>>> >>>> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c >>>> index d5476f1..f8b310b 100644 >>>> --- a/hw/arm/sysbus-fdt.c >>>> +++ b/hw/arm/sysbus-fdt.c >>>> @@ -27,6 +27,8 @@ >>>> #include "hw/platform-bus.h" >>>> #include "sysemu/sysemu.h" >>>> #include "hw/platform-bus.h" >>>> +#include "hw/vfio/vfio-platform.h" >>>> +#include "hw/vfio/vfio-calxeda-xgmac.h" >>>> >>>> /* >>>> * internal struct that contains the information to create dynamic >>>> @@ -54,8 +56,11 @@ typedef struct NodeCreationPair { >>>> int (*add_fdt_node_fn)(SysBusDevice *sbdev, void *opaque); >>>> } NodeCreationPair; >>>> >>>> +static int add_basic_vfio_fdt_node(SysBusDevice *sbdev, void *opaque); >>>> + >>>> /* list of supported dynamic sysbus devices */ >>>> NodeCreationPair add_fdt_node_functions[] = { >>>> + {TYPE_VFIO_CALXEDA_XGMAC, add_basic_vfio_fdt_node}, >>>> {"", NULL}, /*last element*/ >>>> }; >>> >>> Can you maybe place the list somewhere smartly to make sure we don't >>> need forward declarations? Either put it in between the "generic" and >>> "device specific" code or at the end of the file with a single forward >>> declaration for the array? >> >> sure >>> >>>> >>>> @@ -86,6 +91,89 @@ static int add_fdt_node(SysBusDevice *sbdev, void *opaque) >>>> } >>>> >>>> /** >>>> + * add_basic_vfio_fdt_node - generates the most basic node for a VFIO node >>>> + * >>>> + * set properties are: >>>> + * - compatible string >>>> + * - regs >>>> + * - interrupts >>>> + */ >>>> +static int add_basic_vfio_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; >>>> + char *nodename; >>>> + int i, ret; >>>> + uint32_t *irq_attr; >>>> + uint64_t *reg_attr; >>>> + uint64_t mmio_base; >>>> + uint64_t irq_number; >>>> + 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; >>>> + qemu_fdt_setprop(fdt, nodename, "compatible", >>>> + vdev->compat, compat_str_len); >>> >>> What if there are multiple compatibles? >> My purpose here was absolutely not to come back again on a proposal >> where we could have a generic node creation. I understand that it is not >> realistic. I rather tried to put some common property creation in this >> function but you're right even the interrupt prop depend on the device. >> >> About your question, I think the specialized VFIO device would set its >> compat string including the various substrings. This was done in the >> past for PL330 which required arm,pl330;arm,primecell. >> >>> >>>> + >>>> + reg_attr = g_new(uint64_t, vbasedev->num_regions*4); >>>> + >>>> + for (i = 0; i < vbasedev->num_regions; i++) { >>>> + mmio_base = platform_bus_get_mmio_addr(pbus, sbdev, i); >>>> + reg_attr[4*i] = 1; >>> >>> What is the 1 here? >> address-cells? since the bus is < 4GB, 1 32b reg is required to specify >> the base address. But since you put #size-cells already in the parent >> node maybe I can remove it. > > I'm confused. Shouldn't the reg look like [ ... ]? > > http://www.devicetree.org/Device_Tree_Usage#Memory_Mapped_Devices > > The number of cells is defined separately via #address-cells or #size-cells. Hi Alex, sorry my answer was misleading and I was mixing qemu_fdt_setprop_sized_cells_from_array usage and produced dts syntax. "1" values effectively correspond to the number of cells respectively used for addr value and size value. Args of qemu_fdt_setprop_sized_cells_from_array are pairs (size, value), see below as a reminder. The fact platform bus node has attributes #size-cells = <0x1>, and #address-cells = <0x1> forces me to use 1. As a result the guest dt will look as / { #address-cells = <1>; #size-cells = <1>; ... serial@101f0000 { compatible = "arm,pl011"; reg = <0x101f0000 0x1000 >; ../.. I hope this clarifies. Best Regards Eric * qemu_fdt_setprop_sized_cells_from_array: * @fdt: device tree blob * @node_path: node to set property on * @property: property to set * @numvalues: number of values * @values: array of number-of-cells, value pairs * * Set the specified property on the specified node in the device tree * to be an array of cells. The values of the cells are specified via * the values list, which alternates between "number of cells used by * this value" and "value". * number-of-cells must be either 1 or 2 (other values will result in * an error being returned). If a value is too large to fit in the * number of cells specified for it, an error is returned. > >> >>> >>>> + reg_attr[4*i+1] = mmio_base; >>>> + reg_attr[4*i+2] = 1; >>> >>> and here? >> size-cells for this reg. same remark as above > > > Alex >