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

[Alexander Graf]

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.

> 
>>
>>> +        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