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

[Alexander Graf <agraf@suse.de>]

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