From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56034) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XmMGo-0001XT-Bl for qemu-devel@nongnu.org; Thu, 06 Nov 2014 07:34:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XmMGi-0007D6-Sn for qemu-devel@nongnu.org; Thu, 06 Nov 2014 07:34:34 -0500 Received: from cantor2.suse.de ([195.135.220.15]:44466 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XmMGi-0007D0-Hy for qemu-devel@nongnu.org; Thu, 06 Nov 2014 07:34:28 -0500 Message-ID: <545B6ACF.6070207@suse.de> Date: Thu, 06 Nov 2014 13:34:23 +0100 From: Alexander Graf 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> <545B37E0.3030800@linaro.org> In-Reply-To: <545B37E0.3030800@linaro.org> 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: Eric Auger , 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 06.11.14 09:57, Eric Auger wrote: > 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. Ah, what a horrible API :). Maybe we should start introducing functions that have awareness of what #address-cells and #size-cells are and just directly set "regs" to an array of uint64_ts. But this is out of scope for this patch set. Sorry for the fuss. Alex