On 06/16/2015 10:29 AM, Peter Maydell wrote:
> On 15 June 2015 at 17:33, Eric Auger <eric.au...@linaro.org> wrote:
>> This patch allows the instantiation of the vfio-calxeda-xgmac device
>> from the QEMU command line (-device vfio-calxeda-xgmac,host="<device>").
>>
>> A specialized device tree node is created for the guest, containing
>> compat, dma-coherent, reg and interrupts properties.
>>
>> Signed-off-by: Eric Auger <eric.au...@linaro.org>
>>
>
> Minor nits:
>
>> +/* Device Specific Code */
>> +
>> +/**
>> + * add_calxeda_midway_xgmac_fdt_node
>> + *
>> + * Generates a simple node with following properties:
>> + * compatible string, regs, interrupts, dma-coherent
>> + */
>> +static int add_calxeda_midway_xgmac_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, i, ret = -1;
>> + char *nodename;
>> + uint32_t *irq_attr, *reg_attr;
>> + uint64_t mmio_base, irq_number;
>> + VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
>> + VFIODevice *vbasedev = &vdev->vbasedev;
>> +
>> + mmio_base = platform_bus_get_mmio_addr(pbus, sbdev, 0);
>> + 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);
>> +
>> + qemu_fdt_setprop(fdt, nodename, "dma-coherent", "", 0);
>> +
>> + reg_attr = g_new(uint32_t, vbasedev->num_regions*2);
>
> You should have spaces around the '*' operator here (and in a
> couple of other places below).
OK
>
>> + for (i = 0; i < vbasedev->num_regions; i++) {
>> + mmio_base = platform_bus_get_mmio_addr(pbus, sbdev, i);
>> + reg_attr[2*i] = cpu_to_be32(mmio_base);
>> + reg_attr[2*i+1] = cpu_to_be32(
>> + memory_region_size(&vdev->regions[i]->mem));
>> + }
>> + ret = qemu_fdt_setprop(fdt, nodename, "reg", reg_attr,
>> + vbasedev->num_regions*2*sizeof(uint32_t));
>> + if (ret) {
>> + error_report("could not set reg property of node %s", nodename);
>> + goto fail_reg;
>> + }
>> +
>> + 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);
>
> In virt.c the magic numbers in cells 0 and 2 have #defines
> (GIC_FDT_IRQ_TYPE_SPI and GIC_FDT_IRQ_FLAGS_LEVEL_HI).
>
> (1) maybe we should pull those out into a header so you can use them here
OK maybe I can put them in newly created include/hw/arm/virt.h
> (2) is this device really using a level-triggered interrupt?
yes it does
>
>> + }
>> + ret = qemu_fdt_setprop(fdt, nodename, "interrupts",
>> + irq_attr, vbasedev->num_irqs*3*sizeof(uint32_t));
>> + if (ret) {
>> + error_report("could not set interrupts property of node %s",
>> + nodename);
>> + }
>> + g_free(irq_attr);
>> +fail_reg:
>> + g_free(reg_attr);
>> + g_free(nodename);
>> + return ret;
>> +}
>> +
>> /* list of supported dynamic sysbus devices */
>> static const NodeCreationPair add_fdt_node_functions[] = {
>> + {TYPE_VFIO_CALXEDA_XGMAC, add_calxeda_midway_xgmac_fdt_node},
>> {"", NULL}, /* last element */
>> };
>
> This file could get big if we have a lot of platform passthrough
> devices, but let's hope we don't...
Yes let's see how this feature gets used ;-)
>
> Otherwise:
> Acked-by: Peter Maydell <peter.mayd...@linaro.org>
If you allow me I will resend the patch file separately with the above
modifications.
Thanks!
Best Regards
Eric
>
> -- PMM
>
