Re: [Qemu-devel] [PATCH v5 7/8] hw/arm/sysbus-fdt: enable amd-xgbe dynamic instantiation

[Peter Maydell]

On 18 January 2016 at 15:16, Eric Auger <address@hidden> wrote:
> This patch allows the instantiation of the vfio-amd-xgbe device
> from the QEMU command line (-device vfio-amd-xgbe,host="<device>").
>
> The guest is exposed with a device tree node that combines the description
> of both XGBE and PHY (representation supported from 4.2 onwards kernel):
> Documentation/devicetree/bindings/net/amd-xgbe.txt.
>
> There are 5 register regions, 6 interrupts including 4 optional
> edge-sensitive per-channel interrupts.
>
> Some property values are inherited from host device tree. Host device tree
> must feature a combined XGBE/PHY representation (>= 4.2 host kernel).
>
> 2 clock nodes (dma and ptp) also are created. It is checked those clocks
> are fixed on host side.
>
> AMD XGBE node creation function has a dependency on vfio Linux header and
> more generally node creation function for VFIO platform devices only make
> sense with CONFIG_LINUX so let's protect this code with #ifdef CONFIG_LINUX.
>
> Signed-off-by: Eric Auger <address@hidden>

>  hw/arm/sysbus-fdt.c | 195 
> ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 189 insertions(+), 6 deletions(-)

I'll let it pass for this patchset, but this file is starting to
get big, and probably needs some kind of split into common functions
in one file and then a separate file for each pass-through device,
if they're all going to require 200-odd lines to deal with.

> +/**
> + * sysfs_to_dt_name: convert the name found in sysfs into the node name
> + * for instance e0900000.xgmac is converted into address@hidden
> + * @sysfs_name: directory name in sysfs
> + *
> + * returns the device tree name upon success or NULL in case the sysfs name
> + * does not match the expected format
> + */
> +static char *sysfs_to_dt_name(const char *sysfs_name)
> +{
> +    gchar **substrings =  g_strsplit(sysfs_name, ".", 2);
> +    char *dt_name = NULL;
> +
> +    if (!substrings || !substrings[1] || !substrings[0]) {

We should check substrings[0] before substrings[1], as otherwise
if we're passed the empty string we'll index off the end of the
vector.

> +        goto out;
> +    }
> +    dt_name = g_strdup_printf("address@hidden", substrings[1], 
> substrings[0]);
> +out:
> +    g_strfreev(substrings);
> +    return dt_name;
> +}
> +
>  /* Device Specific Code */
>
>  /**
> @@ -243,9 +269,166 @@ fail_reg:
>      return ret;
>  }
>
> +
> +/* AMD xgbe properties whose values are copied/pasted from host */
> +static HostProperty amd_xgbe_copied_properties[] = {
> +    {"compatible", false},
> +    {"dma-coherent", true},
> +    {"amd,per-channel-interrupt", true},
> +    {"phy-mode", false},
> +    {"mac-address", true},
> +    {"amd,speed-set", false},
> +    {"amd,serdes-blwc", true},
> +    {"amd,serdes-cdr-rate", true},
> +    {"amd,serdes-pq-skew", true},
> +    {"amd,serdes-tx-amp", true},
> +    {"amd,serdes-dfe-tap-config", true},
> +    {"amd,serdes-dfe-tap-enable", true},
> +    {"clock-names", false},
> +};
> +
> +/**
> + * add_amd_xgbe_fdt_node
> + *
> + * Generates the combined xgbe/phy node following kernel >=4.2
> + * binding documentation:
> + * Documentation/devicetree/bindings/net/amd-xgbe.txt:
> + * Also 2 clock nodes are created (dma and ptp)
> + *
> + * self-asserts in case of error

"asserts".

> +    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(GIC_FDT_IRQ_TYPE_SPI);
> +        irq_attr[3 * i + 1] = cpu_to_be32(irq_number);
> +        /*
> +          * General device interrupt and PCS auto-negociation interrupts are

"negotiation"

> +          * level-sensitive while the 4 per-channel interrupts are edge
> +          * sensitive
> +          */
> +        QLIST_FOREACH(intp, &vdev->intp_list, next) {
> +            if (intp->pin == i) {
> +                break;
> +            }
> +        }
> +        if (intp->flags & VFIO_IRQ_INFO_AUTOMASKED) {
> +            irq_attr[3 * i + 2] = cpu_to_be32(GIC_FDT_IRQ_FLAGS_LEVEL_HI);
> +        } else {
> +            irq_attr[3 * i + 2] = cpu_to_be32(GIC_FDT_IRQ_FLAGS_EDGE_LO_HI);
> +        }
> +    }
> +    qemu_fdt_setprop(guest_fdt, nodename, "interrupts",
> +                     irq_attr, vbasedev->num_irqs * 3 * sizeof(uint32_t));
> +
> +    g_free(host_fdt);
> +    g_strfreev(node_path);
> +    g_free(irq_attr);
> +    g_free(reg_attr);
> +    g_free(nodename);
> +    return 0;
> +}
> +
> +#endif /* CONFIG_LINUX */
> +
>  /* list of supported dynamic sysbus devices */
>  static const NodeCreationPair add_fdt_node_functions[] = {
> +#ifdef CONFIG_LINUX
>      {TYPE_VFIO_CALXEDA_XGMAC, add_calxeda_midway_xgmac_fdt_node},
> +    {TYPE_VFIO_AMD_XGBE, add_amd_xgbe_fdt_node},
> +#endif
>      {"", NULL}, /* last element */
>  };

thanks
-- PMM