Re: [Qemu-devel] [PATCH v7 12/16] hw/arm/sysbus-fdt: enable vfio-calxeda-xgmac dynamic instantiation

[Eric Auger]

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 <address@hidden>
>>>>
>>>> ---
>>>>
>>>> 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/address@hidden" 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>;

    ...

    address@hidden {
        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
>