| Message ID | 1414764350-5140-13-git-send-email-eric.auger@linaro.org |
|---|---|
| State | New |
| Headers | show |
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 <eric.auger@linaro.org> > > --- > > 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? > > @@ -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? > + > + 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? > + reg_attr[4*i+1] = mmio_base; > + reg_attr[4*i+2] = 1; and here? > + reg_attr[4*i+3] = memory_region_size(&vdev->regions[i]->mem); > + } > + > + ret = qemu_fdt_setprop_sized_cells_from_array(fdt, nodename, "reg", > + vbasedev->num_regions*2, reg_attr); > + if (ret < 0) { > + error_report("could not set reg property of node %s", nodename); > + goto fail; > + } > + > + 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); Why 0x4? How do you know whether an IRQ is edge or level triggered? I'm still not convinced we can make anything "generic" on the VFIO path. How about you call the function xgmac specific for now, but keep the code as dynamic as it is? Alex > + } > + > + ret = qemu_fdt_setprop(fdt, nodename, "interrupts", > + irq_attr, vbasedev->num_irqs*3*sizeof(uint32_t)); > + if (ret < 0) { > + error_report("could not set interrupts property of node %s", > + nodename); > + goto fail; > + } > + > + g_free(nodename); > + g_free(irq_attr); > + g_free(reg_attr); > + > + return 0; > + > +fail: > + > + return -1; > +} > + > +/** > * add_all_platform_bus_fdt_nodes - create all the platform bus nodes > * > * builds the parent platform bus node and all the nodes of dynamic >
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 <eric.auger@linaro.org> >> >> --- >> >> 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. > >> + reg_attr[4*i+1] = mmio_base; >> + reg_attr[4*i+2] = 1; > > and here? size-cells for this reg. same remark as above > >> + reg_attr[4*i+3] = memory_region_size(&vdev->regions[i]->mem); >> + } >> + >> + ret = qemu_fdt_setprop_sized_cells_from_array(fdt, nodename, "reg", >> + vbasedev->num_regions*2, reg_attr); >> + if (ret < 0) { >> + error_report("could not set reg property of node %s", nodename); >> + goto fail; >> + } >> + >> + 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); > > Why 0x4? How do you know whether an IRQ is edge or level triggered? this is indeed device specific. In the future I might be able to read the host dt using Antonios patch but I did not want to add new feature now and add extra dependencies as per the discussion we had with Alex W. > > I'm still not convinced we can make anything "generic" on the VFIO path. > How about you call the function xgmac specific for now, but keep the > code as dynamic as it is? yes I will. > > > Alex > >> + } >> + >> + ret = qemu_fdt_setprop(fdt, nodename, "interrupts", >> + irq_attr, vbasedev->num_irqs*3*sizeof(uint32_t)); >> + if (ret < 0) { >> + error_report("could not set interrupts property of node %s", >> + nodename); >> + goto fail; >> + } >> + >> + g_free(nodename); >> + g_free(irq_attr); >> + g_free(reg_attr); >> + >> + return 0; >> + >> +fail: >> + >> + return -1; >> +} >> + >> +/** >> * add_all_platform_bus_fdt_nodes - create all the platform bus nodes >> * >> * builds the parent platform bus node and all the nodes of dynamic >>
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 <eric.auger@linaro.org> >>> >>> --- >>> >>> 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 [ <addr> <size> ... ]? http://www.devicetree.org/Device_Tree_Usage#Memory_Mapped_Devices The number of cells is defined separately via #address-cells or #size-cells. > >> >>> + 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
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 <eric.auger@linaro.org> >>>> >>>> --- >>>> >>>> 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 [ <addr> <size> ... ]? > > 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 >
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 <eric.auger@linaro.org> >>>>> >>>>> --- >>>>> >>>>> 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 [ <addr> <size> ... ]? >> >> 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
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*/ }; @@ -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); + + 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; + reg_attr[4*i+1] = mmio_base; + reg_attr[4*i+2] = 1; + reg_attr[4*i+3] = memory_region_size(&vdev->regions[i]->mem); + } + + ret = qemu_fdt_setprop_sized_cells_from_array(fdt, nodename, "reg", + vbasedev->num_regions*2, reg_attr); + if (ret < 0) { + error_report("could not set reg property of node %s", nodename); + goto fail; + } + + 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); + } + + ret = qemu_fdt_setprop(fdt, nodename, "interrupts", + irq_attr, vbasedev->num_irqs*3*sizeof(uint32_t)); + if (ret < 0) { + error_report("could not set interrupts property of node %s", + nodename); + goto fail; + } + + g_free(nodename); + g_free(irq_attr); + g_free(reg_attr); + + return 0; + +fail: + + return -1; +} + +/** * add_all_platform_bus_fdt_nodes - create all the platform bus nodes * * builds the parent platform bus node and all the nodes of dynamic
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 <eric.auger@linaro.org> --- 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(+)