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@xxxxxxxxxx>
>>>>
>>>> ---
>>>>
>>>> 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
>
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
