[Qemu-devel] [PATCH v3 0/4] hw/arm/sysbus-fdt: Generic DT Pass-Through

[Qemu-devel] [PATCH v3 0/4] hw/arm/sysbus-fdt: Generic DT Pass-Through

[Geert Uytterhoeven]

	Hi all,

This patch series allows to export generic devices in DT using
vfio-platform, providing direct access from a QEMU+KVM guest to the
exported devices.

  - Patches 1-2 (submitted before by Eric Auger) make the vfio-platform
    device non-abstract, incl. matching using a compatible string.
  - Patch 3 allows dynamic sysbus devices again, without needing to
    create device-specific vfio types for each and every new device.
  - Patch 4 adds code to instantiate device nodes for generic DT
    devices, copying a set of generic properties from the host.

This avoids having to write device-specific instantation methods for
each and every "simple" device using only a set of generic properties.
Devices that need more specialized handling can still provide their own
instantation method.

Changes compared to v2 (not submitted to the mailing list):
  - Use the compatible values from sysfs instead of user-supplied
    manufacturer and model options,
  - Replace "hw/arm/sysbus-fdt: Enable rcar-gen3-gpio dynamic
    instatiation" by generic "hw/arm/sysbus-fdt: Add support for
    instantiating generic devices",
  - Reword patch descriptions,
  - Drop RFC state,
  - Drop "vfio: No-IOMMU mode support".

Changes compared to v1 ("R-Car Gen3 GPIO Pass-Through Prototype (QEMU)",
https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg02716.html):
  - Restrict dynamic sysbus devices to TYPE_VFIO_PLATFORM, as suggested
    by Eric Auger.

For proper operation, this depends on "hw/arm/sysbus-fdt: Fix assertion
in copy_properties_from_host()"
(https://lists.gnu.org/archive/html/qemu-devel/2018-07/msg04922.html).
Thanks!

For testing, this patch series and all prerequisites are available in
the topic/rcar3-virt-gpio-passthrough-v3 branch of my git repository at
https://github.com/geertu/qemu.git.

This has been tested on a Renesas Salvator-XS board with R-Car H3 ES2.0
with SATA:

    -device vfio-platform,host=ee300000.sata

and GPIO (needs VFIO No-IOMMU support):

    -device vfio-platform,host=e6055400.gpio

Thanks!

Auger Eric (2):
  vfio/platform: Make the vfio-platform device non-abstract
  hw/arm/sysbus-fdt: Allow device matching with DT compatible value

Geert Uytterhoeven (2):
  hw/arm/virt: Allow dynamic sysbus devices again
  hw/arm/sysbus-fdt: Add support for instantiating generic devices

 hw/arm/sysbus-fdt.c             | 163 +++++++++++++++++++++++++++++---
 hw/arm/virt.c                   |   1 +
 hw/vfio/amd-xgbe.c              |   1 +
 hw/vfio/calxeda-xgmac.c         |   1 +
 hw/vfio/platform.c              |  22 ++++-
 include/hw/vfio/vfio-platform.h |   3 +-
 6 files changed, 175 insertions(+), 16 deletions(-)

-- 
2.17.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[Qemu-devel] [PATCH v3 1/4] vfio/platform: Make the vfio-platform device non-abstract

[Geert Uytterhoeven]

From: Auger Eric <eric.auger@redhat.com>

Up to now the vfio-platform device has been abstract and could not be
instantiated.  The integration of a new vfio platform device required
creating a dummy derived device which only set the compatible string.

Following the few vfio-platform device integrations we have seen the
actual requested adaptation happens on device tree node creation
(sysbus-fdt).

Hence remove the abstract setting, and read the list of compatible
values from sysfs if not set by a derived device.

Update the amd-xgbe and calxeda-xgmac drivers to fill in the number of
compatible values, as there can now be more than one.

Note that sysbus-fdt does not support the instantiation of the
vfio-platform device yet.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
[geert: Rebase, set user_creatable=true, use compatible values in sysfs
	instead of user-supplied manufacturer/model options, reword]
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v3:
  - Read all compatible values from sysfs instead of using user-supplied
    manufacturer and model options,
  - Reword patch description,
  - Drop RFC state,

v2:
  - No changes,

v1:
  - Rebase, Set user_creatable=true,

v0:
  - Original version from Eric.
---
 hw/vfio/amd-xgbe.c              |  1 +
 hw/vfio/calxeda-xgmac.c         |  1 +
 hw/vfio/platform.c              | 22 +++++++++++++++++++++-
 include/hw/vfio/vfio-platform.h |  3 ++-
 4 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/hw/vfio/amd-xgbe.c b/hw/vfio/amd-xgbe.c
index 0c4ec4ba25170366..ee64a3b4a2e45bf5 100644
--- a/hw/vfio/amd-xgbe.c
+++ b/hw/vfio/amd-xgbe.c
@@ -20,6 +20,7 @@ static void amd_xgbe_realize(DeviceState *dev, Error **errp)
     VFIOAmdXgbeDeviceClass *k = VFIO_AMD_XGBE_DEVICE_GET_CLASS(dev);
 
     vdev->compat = g_strdup("amd,xgbe-seattle-v1a");
+    vdev->num_compat = 1;
 
     k->parent_realize(dev, errp);
 }
diff --git a/hw/vfio/calxeda-xgmac.c b/hw/vfio/calxeda-xgmac.c
index 24cee6d06512c1b6..e7767c4b021be566 100644
--- a/hw/vfio/calxeda-xgmac.c
+++ b/hw/vfio/calxeda-xgmac.c
@@ -20,6 +20,7 @@ static void calxeda_xgmac_realize(DeviceState *dev, Error **errp)
     VFIOCalxedaXgmacDeviceClass *k = VFIO_CALXEDA_XGMAC_DEVICE_GET_CLASS(dev);
 
     vdev->compat = g_strdup("calxeda,hb-xgmac");
+    vdev->num_compat = 1;
 
     k->parent_realize(dev, errp);
 }
diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index 57c4a0ee2b58da70..e264555cd5dafb16 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -655,6 +655,25 @@ static void vfio_platform_realize(DeviceState *dev, Error **errp)
         goto out;
     }
 
+    if (!vdev->compat) {
+        gchar *contents;
+        gsize length;
+        char *tmp;
+
+        tmp = g_strdup_printf("%s/of_node/compatible", vbasedev->sysfsdev);
+        if (!g_file_get_contents(tmp, &contents, &length, NULL)) {
+            error_report("failed to load \"%s\"", tmp);
+            exit(1);
+        }
+        g_free(tmp);
+        vdev->compat = contents;
+        for (vdev->num_compat = 0; length; vdev->num_compat++) {
+            size_t skip = strlen(contents) + 1;
+            contents += skip;
+            length -= skip;
+        }
+    }
+
     for (i = 0; i < vbasedev->num_regions; i++) {
         if (vfio_region_mmap(vdev->regions[i])) {
             error_report("%s mmap unsupported. Performance may be slow",
@@ -700,6 +719,8 @@ static void vfio_platform_class_init(ObjectClass *klass, void *data)
     dc->desc = "VFIO-based platform device assignment";
     sbc->connect_irq_notifier = vfio_start_irqfd_injection;
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+    /* Supported by TYPE_VIRT_MACHINE */
+    dc->user_creatable = true;
 }
 
 static const TypeInfo vfio_platform_dev_info = {
@@ -708,7 +729,6 @@ static const TypeInfo vfio_platform_dev_info = {
     .instance_size = sizeof(VFIOPlatformDevice),
     .class_init = vfio_platform_class_init,
     .class_size = sizeof(VFIOPlatformDeviceClass),
-    .abstract   = true,
 };
 
 static void register_vfio_platform_dev_type(void)
diff --git a/include/hw/vfio/vfio-platform.h b/include/hw/vfio/vfio-platform.h
index 9baaa2db09b84f3e..0ee10b1d71ed2503 100644
--- a/include/hw/vfio/vfio-platform.h
+++ b/include/hw/vfio/vfio-platform.h
@@ -54,7 +54,8 @@ typedef struct VFIOPlatformDevice {
     QLIST_HEAD(, VFIOINTp) intp_list; /* list of IRQs */
     /* queue of pending IRQs */
     QSIMPLEQ_HEAD(pending_intp_queue, VFIOINTp) pending_intp_queue;
-    char *compat; /* compatibility string */
+    char *compat; /* DT compatible values, separated by NUL */
+    unsigned int num_compat; /* number of compatible values */
     uint32_t mmap_timeout; /* delay to re-enable mmaps after interrupt */
     QEMUTimer *mmap_timer; /* allows fast-path resume after IRQ hit */
     QemuMutex intp_mutex; /* protect the intp_list IRQ state */
-- 
2.17.1

[Qemu-devel] [PATCH v3 2/4] hw/arm/sysbus-fdt: Allow device matching with DT compatible value

[Geert Uytterhoeven]

From: Auger Eric <eric.auger@redhat.com>

Up to now we have relied on the device type to identify a device tree
node creation function.  Since we would like the vfio-platform device to
be instantiatable with different compatible strings we introduce the
capability to specialize the node creation depending on actual
compatible value.

NodeCreationPair is renamed into BindingEntry. The struct is enhanced
with compat and match_fn() fields.  We introduce a new matching function
adapted to the vfio-platform generic device.

>From now on, the AMD XGBE can be instantiated with either manner, i.e.:

    -device vfio-amd-xgbe,host=e0900000.xgmac

or using the new option line:

    -device vfio-platform,host=e0900000.xgmac

Signed-off-by: Eric Auger <eric.auger@redhat.com>
[geert: Match using compatible values in sysfs instead of user-supplied
	manufacturer/model options, reword]
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Not tested with amd-xgbe due to lack of hardware.

v3:
  - Match using the compatible values from sysfs instead of using
    user-supplied manufacturer and model options,
  - Reword patch description,
  - Drop RFC state,

v2:
  - No changes,

v1:
  - No changes,

v0:
  - Original version from Eric.
---
 hw/arm/sysbus-fdt.c | 61 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 47 insertions(+), 14 deletions(-)

diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index 43d6a7bb48ddc351..0e24c803a1c2c734 100644
--- a/hw/arm/sysbus-fdt.c
+++ b/hw/arm/sysbus-fdt.c
@@ -50,11 +50,13 @@ typedef struct PlatformBusFDTData {
     PlatformBusDevice *pbus;
 } PlatformBusFDTData;
 
-/* struct that associates a device type name and a node creation function */
-typedef struct NodeCreationPair {
+/* struct that allows to match a device and create its FDT node */
+typedef struct BindingEntry {
     const char *typename;
-    int (*add_fdt_node_fn)(SysBusDevice *sbdev, void *opaque);
-} NodeCreationPair;
+    const char *compat;
+    int  (*add_fn)(SysBusDevice *sbdev, void *opaque);
+    bool (*match_fn)(SysBusDevice *sbdev, const struct BindingEntry *combo);
+} BindingEntry;
 
 /* helpers */
 
@@ -413,6 +415,27 @@ static int add_amd_xgbe_fdt_node(SysBusDevice *sbdev, void *opaque)
     return 0;
 }
 
+/* DT compatible matching */
+static bool vfio_platform_match(SysBusDevice *sbdev,
+                                const BindingEntry *entry)
+{
+    VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
+    const char *compat;
+    unsigned int n;
+
+    for (n = vdev->num_compat, compat = vdev->compat; n > 0;
+         n--, compat += strlen(compat) + 1) {
+        if (!strcmp(entry->compat, compat)) {
+            return true;
+        }
+    }
+
+    return false;
+}
+
+#define VFIO_PLATFORM_BINDING(compat, add_fn) \
+    {TYPE_VFIO_PLATFORM, (compat), (add_fn), vfio_platform_match}
+
 #endif /* CONFIG_LINUX */
 
 static int no_fdt_node(SysBusDevice *sbdev, void *opaque)
@@ -420,14 +443,23 @@ static int no_fdt_node(SysBusDevice *sbdev, void *opaque)
     return 0;
 }
 
-/* list of supported dynamic sysbus devices */
-static const NodeCreationPair add_fdt_node_functions[] = {
+/* Device type based matching */
+static bool type_match(SysBusDevice *sbdev, const BindingEntry *entry)
+{
+    return !strcmp(object_get_typename(OBJECT(sbdev)), entry->typename);
+}
+
+#define TYPE_BINDING(type, add_fn) {(type), NULL, (add_fn), type_match}
+
+/* list of supported dynamic sysbus bindings */
+static const BindingEntry bindings[] = {
 #ifdef CONFIG_LINUX
-    {TYPE_VFIO_CALXEDA_XGMAC, add_calxeda_midway_xgmac_fdt_node},
-    {TYPE_VFIO_AMD_XGBE, add_amd_xgbe_fdt_node},
+    TYPE_BINDING(TYPE_VFIO_CALXEDA_XGMAC, add_calxeda_midway_xgmac_fdt_node),
+    TYPE_BINDING(TYPE_VFIO_AMD_XGBE, add_amd_xgbe_fdt_node),
+    VFIO_PLATFORM_BINDING("amd,xgbe-seattle-v1a", add_amd_xgbe_fdt_node),
 #endif
-    {TYPE_RAMFB_DEVICE, no_fdt_node},
-    {"", NULL}, /* last element */
+    TYPE_BINDING(TYPE_RAMFB_DEVICE, no_fdt_node),
+    TYPE_BINDING("", NULL), /* last element */
 };
 
 /* Generic Code */
@@ -446,10 +478,11 @@ static void add_fdt_node(SysBusDevice *sbdev, void *opaque)
 {
     int i, ret;
 
-    for (i = 0; i < ARRAY_SIZE(add_fdt_node_functions); i++) {
-        if (!strcmp(object_get_typename(OBJECT(sbdev)),
-                    add_fdt_node_functions[i].typename)) {
-            ret = add_fdt_node_functions[i].add_fdt_node_fn(sbdev, opaque);
+    for (i = 0; i < ARRAY_SIZE(bindings); i++) {
+        const BindingEntry *iter = &bindings[i];
+
+        if (iter->match_fn(sbdev, iter)) {
+            ret = iter->add_fn(sbdev, opaque);
             assert(!ret);
             return;
         }
-- 
2.17.1

[Qemu-devel] [PATCH v3 3/4] hw/arm/virt: Allow dynamic sysbus devices again

[Geert Uytterhoeven]

Allow the instantation of generic dynamic sysbus devices again, without
the need to create a new device-specific vfio type.

This is more or less a partial revert of commit 6f2062b9758ebc64
("hw/arm/virt: Allow only supported dynamic sysbus devices").

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v3:
  - Drop RFC state,

v2:
  - Restrict from TYPE_SYS_BUS_DEVICE to TYPE_VFIO_PLATFORM, as
    suggested by Eric Auger.
---
 hw/arm/virt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 281ddcdf6e26236d..24c1d8e28c7c4285 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1724,6 +1724,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
     machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_CALXEDA_XGMAC);
     machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_AMD_XGBE);
     machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE);
+    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_PLATFORM);
     mc->block_default_type = IF_VIRTIO;
     mc->no_cdrom = 1;
     mc->pci_allow_0_address = true;
-- 
2.17.1

[Qemu-devel] [PATCH v3 4/4] hw/arm/sysbus-fdt: Add support for instantiating generic devices

[Geert Uytterhoeven]

Add a fallback for instantiating generic devices without a type-specific
or compatible-specific instantation method.  This will be used when no
other match is found.

The generic instantation method creates a device node with "reg" and
(optional) "interrupts" properties, and copies the "compatible"
property and other optional properties from the host.
For now the following optional properties are copied, if found:
  - "#gpio-cells",
  - "gpio-controller",
  - "#interrupt-cells",
  - "interrupt-controller",
  - "interrupt-names".

More can be added when needed.

This avoids having to write device-specific instantation methods for
each and every "simple" device using only a set of generic properties.
Devices that need more specialized handling can still provide their
own instantation method.

This has been tested on a Renesas Salvator-XS board with R-Car H3 ES2.0
with SATA:

    -device vfio-platform,host=ee300000.sata

and GPIO (needs VFIO No-IOMMU support):

    -device vfio-platform,host=e6055400.gpio

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v3:
  - New.
---
 hw/arm/sysbus-fdt.c | 102 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 102 insertions(+)

diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index 0e24c803a1c2c734..8040af00ccf131d7 100644
--- a/hw/arm/sysbus-fdt.c
+++ b/hw/arm/sysbus-fdt.c
@@ -415,6 +415,99 @@ static int add_amd_xgbe_fdt_node(SysBusDevice *sbdev, void *opaque)
     return 0;
 }
 
+static HostProperty generic_copied_properties[] = {
+    {"compatible", false},
+    {"#gpio-cells", true},
+    {"gpio-controller", true},
+    {"#interrupt-cells", true},
+    {"interrupt-controller", true},
+    {"interrupt-names", true},
+};
+
+/**
+ * add_generic_fdt_node
+ *
+ * Generates a generic DT node by copying properties from the host
+ */
+static int add_generic_fdt_node(SysBusDevice *sbdev, void *opaque)
+{
+    PlatformBusFDTData *data = opaque;
+    VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
+    const char *parent_node = data->pbus_node_name;
+    void *guest_fdt = data->fdt, *host_fdt;
+    VFIODevice *vbasedev = &vdev->vbasedev;
+    char **node_path, *node_name, *dt_name;
+    PlatformBusDevice *pbus = data->pbus;
+    int i, irq_number;
+    uint32_t *reg_attr, *irq_attr;
+    uint64_t mmio_base;
+
+    host_fdt = load_device_tree_from_sysfs();
+
+    dt_name = sysfs_to_dt_name(vbasedev->name);
+    if (!dt_name) {
+        error_report("%s incorrect sysfs device name %s", __func__,
+                     vbasedev->name);
+        exit(1);
+    }
+    node_path = qemu_fdt_node_path(host_fdt, dt_name, vdev->compat,
+                                   &error_fatal);
+    if (!node_path || !node_path[0]) {
+        error_report("%s unable to retrieve node path for %s/%s", __func__,
+                     dt_name, vdev->compat);
+        exit(1);
+    }
+
+    if (node_path[1]) {
+        error_report("%s more than one node matching %s/%s!", __func__, dt_name,
+                     vdev->compat);
+        exit(1);
+    }
+
+    mmio_base = platform_bus_get_mmio_addr(pbus, sbdev, 0);
+    node_name = g_strdup_printf("%s/%s@%" PRIx64, parent_node,
+                               vbasedev->name, mmio_base);
+    qemu_fdt_add_subnode(guest_fdt, node_name);
+
+    /* Copy generic properties */
+    copy_properties_from_host(generic_copied_properties,
+                              ARRAY_SIZE(generic_copied_properties), host_fdt,
+                              guest_fdt, node_path[0], node_name);
+
+    /* Copy reg (remapped) */
+    reg_attr = g_new(uint32_t, vbasedev->num_regions * 2);
+    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));
+    }
+    qemu_fdt_setprop(guest_fdt, node_name, "reg", reg_attr,
+                     vbasedev->num_regions * 2 * sizeof(uint32_t));
+
+    /* Copy interrupts (remapped) */
+    if (vbasedev->num_irqs) {
+        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(GIC_FDT_IRQ_TYPE_SPI);
+            irq_attr[3 * i + 1] = cpu_to_be32(irq_number);
+            irq_attr[3 * i + 2] = cpu_to_be32(GIC_FDT_IRQ_FLAGS_LEVEL_HI);
+        }
+        qemu_fdt_setprop(guest_fdt, node_name, "interrupts",
+                         irq_attr, vbasedev->num_irqs * 3 * sizeof(uint32_t));
+        g_free(irq_attr);
+    }
+
+    g_free(reg_attr);
+    g_free(node_name);
+    g_strfreev(node_path);
+    g_free(dt_name);
+    g_free(host_fdt);
+    return 0;
+}
+
 /* DT compatible matching */
 static bool vfio_platform_match(SysBusDevice *sbdev,
                                 const BindingEntry *entry)
@@ -423,6 +516,11 @@ static bool vfio_platform_match(SysBusDevice *sbdev,
     const char *compat;
     unsigned int n;
 
+    if (!entry->compat) {
+        /* Generic DT fallback */
+        return true;
+    }
+
     for (n = vdev->num_compat, compat = vdev->compat; n > 0;
          n--, compat += strlen(compat) + 1) {
         if (!strcmp(entry->compat, compat)) {
@@ -459,6 +557,10 @@ static const BindingEntry bindings[] = {
     VFIO_PLATFORM_BINDING("amd,xgbe-seattle-v1a", add_amd_xgbe_fdt_node),
 #endif
     TYPE_BINDING(TYPE_RAMFB_DEVICE, no_fdt_node),
+#ifdef CONFIG_LINUX
+    /* Generic DT fallback must be last */
+    VFIO_PLATFORM_BINDING(NULL, add_generic_fdt_node),
+#endif
     TYPE_BINDING("", NULL), /* last element */
 };
 
-- 
2.17.1

Re: [Qemu-devel] [PATCH v3 1/4] vfio/platform: Make the vfio-platform device non-abstract

[Auger Eric]

Hi Geert,

On 07/25/2018 04:34 PM, Geert Uytterhoeven wrote:
> From: Auger Eric <eric.auger@redhat.com>
> 
> Up to now the vfio-platform device has been abstract and could not be
> instantiated.  The integration of a new vfio platform device required
> creating a dummy derived device which only set the compatible string.
> 
> Following the few vfio-platform device integrations we have seen the
> actual requested adaptation happens on device tree node creation
> (sysbus-fdt).
> 
> Hence remove the abstract setting, and read the list of compatible
> values from sysfs if not set by a derived device.
> 
> Update the amd-xgbe and calxeda-xgmac drivers to fill in the number of
> compatible values, as there can now be more than one.
> 
> Note that sysbus-fdt does not support the instantiation of the
> vfio-platform device yet.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> [geert: Rebase, set user_creatable=true, use compatible values in sysfs
> 	instead of user-supplied manufacturer/model options, reword]
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v3:
>   - Read all compatible values from sysfs instead of using user-supplied
>     manufacturer and model options,
>   - Reword patch description,
>   - Drop RFC state,
> 
> v2:
>   - No changes,
> 
> v1:
>   - Rebase, Set user_creatable=true,
> 
> v0:
>   - Original version from Eric.
> ---
>  hw/vfio/amd-xgbe.c              |  1 +
>  hw/vfio/calxeda-xgmac.c         |  1 +
>  hw/vfio/platform.c              | 22 +++++++++++++++++++++-
>  include/hw/vfio/vfio-platform.h |  3 ++-
>  4 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/vfio/amd-xgbe.c b/hw/vfio/amd-xgbe.c
> index 0c4ec4ba25170366..ee64a3b4a2e45bf5 100644
> --- a/hw/vfio/amd-xgbe.c
> +++ b/hw/vfio/amd-xgbe.c
> @@ -20,6 +20,7 @@ static void amd_xgbe_realize(DeviceState *dev, Error **errp)
>      VFIOAmdXgbeDeviceClass *k = VFIO_AMD_XGBE_DEVICE_GET_CLASS(dev);
>  
>      vdev->compat = g_strdup("amd,xgbe-seattle-v1a");
> +    vdev->num_compat = 1;
>  
>      k->parent_realize(dev, errp);
>  }
> diff --git a/hw/vfio/calxeda-xgmac.c b/hw/vfio/calxeda-xgmac.c
> index 24cee6d06512c1b6..e7767c4b021be566 100644
> --- a/hw/vfio/calxeda-xgmac.c
> +++ b/hw/vfio/calxeda-xgmac.c
> @@ -20,6 +20,7 @@ static void calxeda_xgmac_realize(DeviceState *dev, Error **errp)
>      VFIOCalxedaXgmacDeviceClass *k = VFIO_CALXEDA_XGMAC_DEVICE_GET_CLASS(dev);
>  
>      vdev->compat = g_strdup("calxeda,hb-xgmac");
> +    vdev->num_compat = 1;
>  
>      k->parent_realize(dev, errp);
>  }
> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
> index 57c4a0ee2b58da70..e264555cd5dafb16 100644
> --- a/hw/vfio/platform.c
> +++ b/hw/vfio/platform.c
> @@ -655,6 +655,25 @@ static void vfio_platform_realize(DeviceState *dev, Error **errp)
>          goto out;
>      }
>  
> +    if (!vdev->compat) {
> +        gchar *contents;
> +        gsize length;
> +        char *tmp;
> +
> +        tmp = g_strdup_printf("%s/of_node/compatible", vbasedev->sysfsdev);
> +        if (!g_file_get_contents(tmp, &contents, &length, NULL)) {
> +            error_report("failed to load \"%s\"", tmp);
> +            exit(1);
You should set errp instead so that the error gets properly propagated.
> +        }
> +        g_free(tmp);
> +        vdev->compat = contents;
> +        for (vdev->num_compat = 0; length; vdev->num_compat++) {
> +            size_t skip = strlen(contents) + 1;
> +            contents += skip;
> +            length -= skip;
> +        }
> +    }
> +
>      for (i = 0; i < vbasedev->num_regions; i++) {
>          if (vfio_region_mmap(vdev->regions[i])) {
>              error_report("%s mmap unsupported. Performance may be slow",
> @@ -700,6 +719,8 @@ static void vfio_platform_class_init(ObjectClass *klass, void *data)
>      dc->desc = "VFIO-based platform device assignment";
>      sbc->connect_irq_notifier = vfio_start_irqfd_injection;
>      set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> +    /* Supported by TYPE_VIRT_MACHINE */
> +    dc->user_creatable = true;
>  }
>  
>  static const TypeInfo vfio_platform_dev_info = {
> @@ -708,7 +729,6 @@ static const TypeInfo vfio_platform_dev_info = {
>      .instance_size = sizeof(VFIOPlatformDevice),
>      .class_init = vfio_platform_class_init,
>      .class_size = sizeof(VFIOPlatformDeviceClass),
> -    .abstract   = true,
>  };
>  
>  static void register_vfio_platform_dev_type(void)
> diff --git a/include/hw/vfio/vfio-platform.h b/include/hw/vfio/vfio-platform.h
> index 9baaa2db09b84f3e..0ee10b1d71ed2503 100644
> --- a/include/hw/vfio/vfio-platform.h
> +++ b/include/hw/vfio/vfio-platform.h
> @@ -54,7 +54,8 @@ typedef struct VFIOPlatformDevice {
>      QLIST_HEAD(, VFIOINTp) intp_list; /* list of IRQs */
>      /* queue of pending IRQs */
>      QSIMPLEQ_HEAD(pending_intp_queue, VFIOINTp) pending_intp_queue;
> -    char *compat; /* compatibility string */
> +    char *compat; /* DT compatible values, separated by NUL */
by NULL characters?
> +    unsigned int num_compat; /* number of compatible values */
>      uint32_t mmap_timeout; /* delay to re-enable mmaps after interrupt */
>      QEMUTimer *mmap_timer; /* allows fast-path resume after IRQ hit */
>      QemuMutex intp_mutex; /* protect the intp_list IRQ state */
Thanks

Eric
> 

Re: [Qemu-devel] [PATCH v3 2/4] hw/arm/sysbus-fdt: Allow device matching with DT compatible value

[Auger Eric]

Hi Geert,

On 07/25/2018 04:34 PM, Geert Uytterhoeven wrote:
> From: Auger Eric <eric.auger@redhat.com>
> 
> Up to now we have relied on the device type to identify a device tree
> node creation function.  Since we would like the vfio-platform device to
> be instantiatable with different compatible strings we introduce the
> capability to specialize the node creation depending on actual
> compatible value.
> 
> NodeCreationPair is renamed into BindingEntry. The struct is enhanced
> with compat and match_fn() fields.  We introduce a new matching function
> adapted to the vfio-platform generic device.
> 
> From now on, the AMD XGBE can be instantiated with either manner, i.e.:
> 
>     -device vfio-amd-xgbe,host=e0900000.xgmac
> 
> or using the new option line:
> 
>     -device vfio-platform,host=e0900000.xgmac
strictly speaking I think you need patch 3 to do that?
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> [geert: Match using compatible values in sysfs instead of user-supplied
> 	manufacturer/model options, reword]
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Not tested with amd-xgbe due to lack of hardware.
I tested it. Feel free to add:
Tested-by: Eric Auger <eric.auger@redhat.com>

Otherwise looks fine to me

Thanks

Eric
> 
> v3:
>   - Match using the compatible values from sysfs instead of using
>     user-supplied manufacturer and model options,
>   - Reword patch description,
>   - Drop RFC state,
> 
> v2:
>   - No changes,
> 
> v1:
>   - No changes,
> 
> v0:
>   - Original version from Eric.
> ---
>  hw/arm/sysbus-fdt.c | 61 ++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 47 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> index 43d6a7bb48ddc351..0e24c803a1c2c734 100644
> --- a/hw/arm/sysbus-fdt.c
> +++ b/hw/arm/sysbus-fdt.c
> @@ -50,11 +50,13 @@ typedef struct PlatformBusFDTData {
>      PlatformBusDevice *pbus;
>  } PlatformBusFDTData;
>  
> -/* struct that associates a device type name and a node creation function */
> -typedef struct NodeCreationPair {
> +/* struct that allows to match a device and create its FDT node */
> +typedef struct BindingEntry {
>      const char *typename;
> -    int (*add_fdt_node_fn)(SysBusDevice *sbdev, void *opaque);
> -} NodeCreationPair;
> +    const char *compat;
> +    int  (*add_fn)(SysBusDevice *sbdev, void *opaque);
> +    bool (*match_fn)(SysBusDevice *sbdev, const struct BindingEntry *combo);
> +} BindingEntry;
>  
>  /* helpers */
>  
> @@ -413,6 +415,27 @@ static int add_amd_xgbe_fdt_node(SysBusDevice *sbdev, void *opaque)
>      return 0;
>  }
>  
> +/* DT compatible matching */
> +static bool vfio_platform_match(SysBusDevice *sbdev,
> +                                const BindingEntry *entry)
> +{
> +    VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
> +    const char *compat;
> +    unsigned int n;
> +
> +    for (n = vdev->num_compat, compat = vdev->compat; n > 0;
> +         n--, compat += strlen(compat) + 1) {
> +        if (!strcmp(entry->compat, compat)) {
> +            return true;
> +        }
> +    }
> +
> +    return false;
> +}
> +
> +#define VFIO_PLATFORM_BINDING(compat, add_fn) \
> +    {TYPE_VFIO_PLATFORM, (compat), (add_fn), vfio_platform_match}
> +
>  #endif /* CONFIG_LINUX */
>  
>  static int no_fdt_node(SysBusDevice *sbdev, void *opaque)
> @@ -420,14 +443,23 @@ static int no_fdt_node(SysBusDevice *sbdev, void *opaque)
>      return 0;
>  }
>  
> -/* list of supported dynamic sysbus devices */
> -static const NodeCreationPair add_fdt_node_functions[] = {
> +/* Device type based matching */
> +static bool type_match(SysBusDevice *sbdev, const BindingEntry *entry)
> +{
> +    return !strcmp(object_get_typename(OBJECT(sbdev)), entry->typename);
> +}
> +
> +#define TYPE_BINDING(type, add_fn) {(type), NULL, (add_fn), type_match}
> +
> +/* list of supported dynamic sysbus bindings */
> +static const BindingEntry bindings[] = {
>  #ifdef CONFIG_LINUX
> -    {TYPE_VFIO_CALXEDA_XGMAC, add_calxeda_midway_xgmac_fdt_node},
> -    {TYPE_VFIO_AMD_XGBE, add_amd_xgbe_fdt_node},
> +    TYPE_BINDING(TYPE_VFIO_CALXEDA_XGMAC, add_calxeda_midway_xgmac_fdt_node),
> +    TYPE_BINDING(TYPE_VFIO_AMD_XGBE, add_amd_xgbe_fdt_node),
> +    VFIO_PLATFORM_BINDING("amd,xgbe-seattle-v1a", add_amd_xgbe_fdt_node),
>  #endif
> -    {TYPE_RAMFB_DEVICE, no_fdt_node},
> -    {"", NULL}, /* last element */
> +    TYPE_BINDING(TYPE_RAMFB_DEVICE, no_fdt_node),
> +    TYPE_BINDING("", NULL), /* last element */
>  };
>  
>  /* Generic Code */
> @@ -446,10 +478,11 @@ static void add_fdt_node(SysBusDevice *sbdev, void *opaque)
>  {
>      int i, ret;
>  
> -    for (i = 0; i < ARRAY_SIZE(add_fdt_node_functions); i++) {
> -        if (!strcmp(object_get_typename(OBJECT(sbdev)),
> -                    add_fdt_node_functions[i].typename)) {
> -            ret = add_fdt_node_functions[i].add_fdt_node_fn(sbdev, opaque);
> +    for (i = 0; i < ARRAY_SIZE(bindings); i++) {
> +        const BindingEntry *iter = &bindings[i];
> +
> +        if (iter->match_fn(sbdev, iter)) {
> +            ret = iter->add_fn(sbdev, opaque);
>              assert(!ret);
>              return;
>          }
> 

Re: [Qemu-devel] [PATCH v3 3/4] hw/arm/virt: Allow dynamic sysbus devices again

[Auger Eric]

Hi Geert,

On 07/25/2018 04:34 PM, Geert Uytterhoeven wrote:
> Allow the instantation of generic dynamic sysbus devices again, without
> the need to create a new device-specific vfio type.
The patch title and commit message need to be updated as we allow the
dynamic instantiation of VFIO PLATFORM devices.

Thanks

Eric
> 
> This is more or less a partial revert of commit 6f2062b9758ebc64
> ("hw/arm/virt: Allow only supported dynamic sysbus devices").
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v3:
>   - Drop RFC state,
> 
> v2:
>   - Restrict from TYPE_SYS_BUS_DEVICE to TYPE_VFIO_PLATFORM, as
>     suggested by Eric Auger.
> ---
>  hw/arm/virt.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 281ddcdf6e26236d..24c1d8e28c7c4285 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1724,6 +1724,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
>      machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_CALXEDA_XGMAC);
>      machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_AMD_XGBE);
>      machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE);
> +    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_PLATFORM);
>      mc->block_default_type = IF_VIRTIO;
>      mc->no_cdrom = 1;
>      mc->pci_allow_0_address = true;
> 

Re: [Qemu-devel] [PATCH v3 4/4] hw/arm/sysbus-fdt: Add support for instantiating generic devices

[Auger Eric]

Hi Geert,

On 07/25/2018 04:34 PM, Geert Uytterhoeven wrote:
> Add a fallback for instantiating generic devices without a type-specific
> or compatible-specific instantation method.  This will be used when no
> other match is found.
> 
> The generic instantation method creates a device node with "reg" and
> (optional) "interrupts" properties, and copies the "compatible"
> property and other optional properties from the host.
> For now the following optional properties are copied, if found:
>   - "#gpio-cells",
>   - "gpio-controller",
>   - "#interrupt-cells",
>   - "interrupt-controller",
>   - "interrupt-names".
> 
> More can be added when needed.
> 
> This avoids having to write device-specific instantation methods for
instantiation
> each and every "simple" device using only a set of generic properties.
> Devices that need more specialized handling can still provide their
> own instantation method.
> 
> This has been tested on a Renesas Salvator-XS board with R-Car H3 ES2.0
> with SATA:
> 
>     -device vfio-platform,host=ee300000.sata
> 
> and GPIO (needs VFIO No-IOMMU support):
> 
>     -device vfio-platform,host=e6055400.gpio
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v3:
>   - New.
> ---
>  hw/arm/sysbus-fdt.c | 102 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 102 insertions(+)
> 
> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> index 0e24c803a1c2c734..8040af00ccf131d7 100644
> --- a/hw/arm/sysbus-fdt.c
> +++ b/hw/arm/sysbus-fdt.c
> @@ -415,6 +415,99 @@ static int add_amd_xgbe_fdt_node(SysBusDevice *sbdev, void *opaque)
>      return 0;
>  }
>  
> +static HostProperty generic_copied_properties[] = {
> +    {"compatible", false},
> +    {"#gpio-cells", true},
> +    {"gpio-controller", true},
> +    {"#interrupt-cells", true},
> +    {"interrupt-controller", true},
> +    {"interrupt-names", true},
I think we would need to enumerate the other source properties which
were not copied to the guest fdt and either warn the userspace those
were omitted or fail. We may end up generating an incomplete guest dt
node that may not work properly.
> +};
> +
> +/**
> + * add_generic_fdt_node
> + *
> + * Generates a generic DT node by copying properties from the host
> + */
> +static int add_generic_fdt_node(SysBusDevice *sbdev, void *opaque)
> +{
> +    PlatformBusFDTData *data = opaque;
> +    VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
> +    const char *parent_node = data->pbus_node_name;
> +    void *guest_fdt = data->fdt, *host_fdt;
> +    VFIODevice *vbasedev = &vdev->vbasedev;
> +    char **node_path, *node_name, *dt_name;
> +    PlatformBusDevice *pbus = data->pbus;
> +    int i, irq_number;
> +    uint32_t *reg_attr, *irq_attr;
> +    uint64_t mmio_base;
> +
> +    host_fdt = load_device_tree_from_sysfs();
> +
> +    dt_name = sysfs_to_dt_name(vbasedev->name);
> +    if (!dt_name) {
> +        error_report("%s incorrect sysfs device name %s", __func__,
> +                     vbasedev->name);
> +        exit(1);
> +    }
> +    node_path = qemu_fdt_node_path(host_fdt, dt_name, vdev->compat,
> +                                   &error_fatal);
> +    if (!node_path || !node_path[0]) {
> +        error_report("%s unable to retrieve node path for %s/%s", __func__,
> +                     dt_name, vdev->compat);
> +        exit(1);
> +    }
> +
> +    if (node_path[1]) {
> +        error_report("%s more than one node matching %s/%s!", __func__, dt_name,
> +                     vdev->compat);
> +        exit(1);
> +    }
> +
> +    mmio_base = platform_bus_get_mmio_addr(pbus, sbdev, 0);
> +    node_name = g_strdup_printf("%s/%s@%" PRIx64, parent_node,
> +                               vbasedev->name, mmio_base);
> +    qemu_fdt_add_subnode(guest_fdt, node_name);
> +
> +    /* Copy generic properties */
> +    copy_properties_from_host(generic_copied_properties,
> +                              ARRAY_SIZE(generic_copied_properties), host_fdt,
> +                              guest_fdt, node_path[0], node_name);
> +
> +    /* Copy reg (remapped) */
> +    reg_attr = g_new(uint32_t, vbasedev->num_regions * 2);
> +    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));
> +    }
> +    qemu_fdt_setprop(guest_fdt, node_name, "reg", reg_attr,
> +                     vbasedev->num_regions * 2 * sizeof(uint32_t));
> +
> +    /* Copy interrupts (remapped) */
> +    if (vbasedev->num_irqs) {
> +        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(GIC_FDT_IRQ_TYPE_SPI);
> +            irq_attr[3 * i + 1] = cpu_to_be32(irq_number);
> +            irq_attr[3 * i + 2] = cpu_to_be32(GIC_FDT_IRQ_FLAGS_LEVEL_HI);
I think this part is not generic enough. How can you assume the IRQs are
level or edge? Can't you copy the source interrupt properties and just
overwrite the IRQ number?

Thanks

Eric
> +        }
> +        qemu_fdt_setprop(guest_fdt, node_name, "interrupts",
> +                         irq_attr, vbasedev->num_irqs * 3 * sizeof(uint32_t));
> +        g_free(irq_attr);
> +    }
> +
> +    g_free(reg_attr);
> +    g_free(node_name);
> +    g_strfreev(node_path);
> +    g_free(dt_name);
> +    g_free(host_fdt);
> +    return 0;
> +}
> +
>  /* DT compatible matching */
>  static bool vfio_platform_match(SysBusDevice *sbdev,
>                                  const BindingEntry *entry)
> @@ -423,6 +516,11 @@ static bool vfio_platform_match(SysBusDevice *sbdev,
>      const char *compat;
>      unsigned int n;
>  
> +    if (!entry->compat) {
> +        /* Generic DT fallback */
> +        return true;
> +    }
> +
>      for (n = vdev->num_compat, compat = vdev->compat; n > 0;
>           n--, compat += strlen(compat) + 1) {
>          if (!strcmp(entry->compat, compat)) {
> @@ -459,6 +557,10 @@ static const BindingEntry bindings[] = {
>      VFIO_PLATFORM_BINDING("amd,xgbe-seattle-v1a", add_amd_xgbe_fdt_node),
>  #endif
>      TYPE_BINDING(TYPE_RAMFB_DEVICE, no_fdt_node),
> +#ifdef CONFIG_LINUX
> +    /* Generic DT fallback must be last */
> +    VFIO_PLATFORM_BINDING(NULL, add_generic_fdt_node),
> +#endif
>      TYPE_BINDING("", NULL), /* last element */
>  };
>  
> 

Re: [Qemu-devel] [PATCH v3 1/4] vfio/platform: Make the vfio-platform device non-abstract

[Geert Uytterhoeven]

Hi Eric,

On Tue, Aug 7, 2018 at 4:18 PM Auger Eric <eric.auger@redhat.com> wrote:
> On 07/25/2018 04:34 PM, Geert Uytterhoeven wrote:
> > From: Auger Eric <eric.auger@redhat.com>
> >
> > Up to now the vfio-platform device has been abstract and could not be
> > instantiated.  The integration of a new vfio platform device required
> > creating a dummy derived device which only set the compatible string.
> >
> > Following the few vfio-platform device integrations we have seen the
> > actual requested adaptation happens on device tree node creation
> > (sysbus-fdt).
> >
> > Hence remove the abstract setting, and read the list of compatible
> > values from sysfs if not set by a derived device.
> >
> > Update the amd-xgbe and calxeda-xgmac drivers to fill in the number of
> > compatible values, as there can now be more than one.
> >
> > Note that sysbus-fdt does not support the instantiation of the
> > vfio-platform device yet.
> >
> > Signed-off-by: Eric Auger <eric.auger@redhat.com>
> > [geert: Rebase, set user_creatable=true, use compatible values in sysfs
> >       instead of user-supplied manufacturer/model options, reword]
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > v3:
> >   - Read all compatible values from sysfs instead of using user-supplied
> >     manufacturer and model options,
> >   - Reword patch description,
> >   - Drop RFC state,
> >
> > v2:
> >   - No changes,
> >
> > v1:
> >   - Rebase, Set user_creatable=true,
> >
> > v0:
> >   - Original version from Eric.

> > --- a/hw/vfio/platform.c
> > +++ b/hw/vfio/platform.c
> > @@ -655,6 +655,25 @@ static void vfio_platform_realize(DeviceState *dev, Error **errp)
> >          goto out;
> >      }
> >
> > +    if (!vdev->compat) {
> > +        gchar *contents;
> > +        gsize length;
> > +        char *tmp;
> > +
> > +        tmp = g_strdup_printf("%s/of_node/compatible", vbasedev->sysfsdev);
> > +        if (!g_file_get_contents(tmp, &contents, &length, NULL)) {
> > +            error_report("failed to load \"%s\"", tmp);
> > +            exit(1);
> You should set errp instead so that the error gets properly propagated.

Thanks, will do.

> > --- a/include/hw/vfio/vfio-platform.h
> > +++ b/include/hw/vfio/vfio-platform.h
> > @@ -54,7 +54,8 @@ typedef struct VFIOPlatformDevice {
> >      QLIST_HEAD(, VFIOINTp) intp_list; /* list of IRQs */
> >      /* queue of pending IRQs */
> >      QSIMPLEQ_HEAD(pending_intp_queue, VFIOINTp) pending_intp_queue;
> > -    char *compat; /* compatibility string */
> > +    char *compat; /* DT compatible values, separated by NUL */
> by NULL characters?

"NUL" is the character ('\0'), "NULL" is the pointer.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [Qemu-devel] [PATCH v3 1/4] vfio/platform: Make the vfio-platform device non-abstract

[Auger Eric]

Hi Geert,

On 08/07/2018 05:00 PM, Geert Uytterhoeven wrote:
> Hi Eric,
> 
> On Tue, Aug 7, 2018 at 4:18 PM Auger Eric <eric.auger@redhat.com> wrote:
>> On 07/25/2018 04:34 PM, Geert Uytterhoeven wrote:
>>> From: Auger Eric <eric.auger@redhat.com>
>>>
>>> Up to now the vfio-platform device has been abstract and could not be
>>> instantiated.  The integration of a new vfio platform device required
>>> creating a dummy derived device which only set the compatible string.
>>>
>>> Following the few vfio-platform device integrations we have seen the
>>> actual requested adaptation happens on device tree node creation
>>> (sysbus-fdt).
>>>
>>> Hence remove the abstract setting, and read the list of compatible
>>> values from sysfs if not set by a derived device.
>>>
>>> Update the amd-xgbe and calxeda-xgmac drivers to fill in the number of
>>> compatible values, as there can now be more than one.
>>>
>>> Note that sysbus-fdt does not support the instantiation of the
>>> vfio-platform device yet.
>>>
>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>> [geert: Rebase, set user_creatable=true, use compatible values in sysfs
>>>       instead of user-supplied manufacturer/model options, reword]
>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>> ---
>>> v3:
>>>   - Read all compatible values from sysfs instead of using user-supplied
>>>     manufacturer and model options,
>>>   - Reword patch description,
>>>   - Drop RFC state,
>>>
>>> v2:
>>>   - No changes,
>>>
>>> v1:
>>>   - Rebase, Set user_creatable=true,
>>>
>>> v0:
>>>   - Original version from Eric.
> 
>>> --- a/hw/vfio/platform.c
>>> +++ b/hw/vfio/platform.c
>>> @@ -655,6 +655,25 @@ static void vfio_platform_realize(DeviceState *dev, Error **errp)
>>>          goto out;
>>>      }
>>>
>>> +    if (!vdev->compat) {
>>> +        gchar *contents;
>>> +        gsize length;
>>> +        char *tmp;
>>> +
>>> +        tmp = g_strdup_printf("%s/of_node/compatible", vbasedev->sysfsdev);
>>> +        if (!g_file_get_contents(tmp, &contents, &length, NULL)) {
>>> +            error_report("failed to load \"%s\"", tmp);
>>> +            exit(1);
>> You should set errp instead so that the error gets properly propagated.
> 
> Thanks, will do.
> 
>>> --- a/include/hw/vfio/vfio-platform.h
>>> +++ b/include/hw/vfio/vfio-platform.h
>>> @@ -54,7 +54,8 @@ typedef struct VFIOPlatformDevice {
>>>      QLIST_HEAD(, VFIOINTp) intp_list; /* list of IRQs */
>>>      /* queue of pending IRQs */
>>>      QSIMPLEQ_HEAD(pending_intp_queue, VFIOINTp) pending_intp_queue;
>>> -    char *compat; /* compatibility string */
>>> +    char *compat; /* DT compatible values, separated by NUL */
>> by NULL characters?
> 
> "NUL" is the character ('\0'), "NULL" is the pointer.
Ah OK ;-)

Thanks

Eric
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

Re: [Qemu-devel] [PATCH v3 4/4] hw/arm/sysbus-fdt: Add support for instantiating generic devices

[Geert Uytterhoeven]

Hi Eric,

On Tue, Aug 7, 2018 at 4:19 PM Auger Eric <eric.auger@redhat.com> wrote:
> On 07/25/2018 04:34 PM, Geert Uytterhoeven wrote:
> > Add a fallback for instantiating generic devices without a type-specific
> > or compatible-specific instantation method.  This will be used when no
> > other match is found.
> >
> > The generic instantation method creates a device node with "reg" and
> > (optional) "interrupts" properties, and copies the "compatible"
> > property and other optional properties from the host.
> > For now the following optional properties are copied, if found:
> >   - "#gpio-cells",
> >   - "gpio-controller",
> >   - "#interrupt-cells",
> >   - "interrupt-controller",
> >   - "interrupt-names".
> >
> > More can be added when needed.
> >
> > This avoids having to write device-specific instantation methods for
> instantiation

Thanks, will fix.

> > each and every "simple" device using only a set of generic properties.
> > Devices that need more specialized handling can still provide their
> > own instantation method.

Arghl, one more.

> > --- a/hw/arm/sysbus-fdt.c
> > +++ b/hw/arm/sysbus-fdt.c
> > @@ -415,6 +415,99 @@ static int add_amd_xgbe_fdt_node(SysBusDevice *sbdev, void *opaque)
> >      return 0;
> >  }
> >
> > +static HostProperty generic_copied_properties[] = {
> > +    {"compatible", false},
> > +    {"#gpio-cells", true},
> > +    {"gpio-controller", true},
> > +    {"#interrupt-cells", true},
> > +    {"interrupt-controller", true},
> > +    {"interrupt-names", true},
>
> I think we would need to enumerate the other source properties which
> were not copied to the guest fdt and either warn the userspace those
> were omitted or fail. We may end up generating an incomplete guest dt
> node that may not work properly.

The list above is quite generic, so it is expected that some of the optional
properties (marked "true") cannot be copied. Hence warning about them
will be noisy, and confuse users.  Failing is definitely the wrong thing
to do.

If the host lacks a property that is mandatory for a specific device, the
device won't have worked on the host before neither, right?

The major issue remains that an incomplete guest DT node may be generated
if the list above lacks certain needed properties, or if subnodes are
needed.
I expect the guest OS driver would complain about the missing parts, though.
In that case, either the list should be extended, or a device-specific
instantiation method should be written.

> > +/**
> > + * add_generic_fdt_node
> > + *
> > + * Generates a generic DT node by copying properties from the host
> > + */
> > +static int add_generic_fdt_node(SysBusDevice *sbdev, void *opaque)
> > +{

> > +    /* Copy interrupts (remapped) */
> > +    if (vbasedev->num_irqs) {
> > +        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(GIC_FDT_IRQ_TYPE_SPI);
> > +            irq_attr[3 * i + 1] = cpu_to_be32(irq_number);
> > +            irq_attr[3 * i + 2] = cpu_to_be32(GIC_FDT_IRQ_FLAGS_LEVEL_HI);
>
> I think this part is not generic enough. How can you assume the IRQs are
> level or edge? Can't you copy the source interrupt properties and just
> overwrite the IRQ number?

I have to check that. I wouldn't be surprised if the virtualized GIC supports
only a subset of all possible flags and/or remaps interrupt types too.

I see the AMD XGBE driver looks at VFIO_IRQ_INFO_AUTOMASKED to choose
between GIC_FDT_IRQ_FLAGS_LEVEL_HI and GIC_FDT_IRQ_FLAGS_EDGE_LO_HI.
The VFIO_IRQ_INFO_* don't seem to provide more details.
There are also no users of GIC_FDT_IRQ_FLAGS_LEVEL_LO and
GIC_FDT_IRQ_FLAGS_EDGE_HI_LO in the Qemu sources...

So probably following the AMD XGBE example is the way forward?

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [Qemu-devel] [PATCH v3 4/4] hw/arm/sysbus-fdt: Add support for instantiating generic devices

[Auger Eric]

Hi Geert,

On 08/07/2018 05:32 PM, Geert Uytterhoeven wrote:
> Hi Eric,
> 
> On Tue, Aug 7, 2018 at 4:19 PM Auger Eric <eric.auger@redhat.com> wrote:
>> On 07/25/2018 04:34 PM, Geert Uytterhoeven wrote:
>>> Add a fallback for instantiating generic devices without a type-specific
>>> or compatible-specific instantation method.  This will be used when no
>>> other match is found.
>>>
>>> The generic instantation method creates a device node with "reg" and
>>> (optional) "interrupts" properties, and copies the "compatible"
>>> property and other optional properties from the host.
>>> For now the following optional properties are copied, if found:
>>>   - "#gpio-cells",
>>>   - "gpio-controller",
>>>   - "#interrupt-cells",
>>>   - "interrupt-controller",
>>>   - "interrupt-names".
>>>
>>> More can be added when needed.
>>>
>>> This avoids having to write device-specific instantation methods for
>> instantiation
> 
> Thanks, will fix.
> 
>>> each and every "simple" device using only a set of generic properties.
>>> Devices that need more specialized handling can still provide their
>>> own instantation method.
> 
> Arghl, one more.
> 
>>> --- a/hw/arm/sysbus-fdt.c
>>> +++ b/hw/arm/sysbus-fdt.c
>>> @@ -415,6 +415,99 @@ static int add_amd_xgbe_fdt_node(SysBusDevice *sbdev, void *opaque)
>>>      return 0;
>>>  }
>>>
>>> +static HostProperty generic_copied_properties[] = {
>>> +    {"compatible", false},
>>> +    {"#gpio-cells", true},
>>> +    {"gpio-controller", true},
>>> +    {"#interrupt-cells", true},
>>> +    {"interrupt-controller", true},
>>> +    {"interrupt-names", true},
>>
>> I think we would need to enumerate the other source properties which
>> were not copied to the guest fdt and either warn the userspace those
>> were omitted or fail. We may end up generating an incomplete guest dt
>> node that may not work properly.
> 
> The list above is quite generic, so it is expected that some of the optional
> properties (marked "true") cannot be copied. Hence warning about them
> will be noisy, and confuse users.  Failing is definitely the wrong thing
> to do.
I was not talking about those listed here and optional. Those ones are
taken care of. I was rather talking about potential other ones, present
on host side and not copied on guest side. For instance, let's say your
host dt node has a clocks property. You will attempt to create a guest
dt node without this property and obviously the device will not work
properly on guest side. The end user attempting this assignment does not
get any warning on guest launch. Maybe the driver on guest side will be
cool enough to issue a warning but we cannot really rely on this
assumption. So from a maintenance point of view, it looks not
manageable. I think we should checl all props found in the host dt node
are considered and copied into the guest node. Otherwise this means the
host dt node does not belong to the category of a "simple" one and thus
cannot use this creation function.
> 
> If the host lacks a property that is mandatory for a specific device, the
> device won't have worked on the host before neither, right?
> 
> The major issue remains that an incomplete guest DT node may be generated
> if the list above lacks certain needed properties, or if subnodes are
> needed.
> I expect the guest OS driver would complain about the missing parts, though.
> In that case, either the list should be extended, or a device-specific
> instantiation method should be written.
> 
>>> +/**
>>> + * add_generic_fdt_node
>>> + *
>>> + * Generates a generic DT node by copying properties from the host
>>> + */
>>> +static int add_generic_fdt_node(SysBusDevice *sbdev, void *opaque)
>>> +{
> 
>>> +    /* Copy interrupts (remapped) */
>>> +    if (vbasedev->num_irqs) {
>>> +        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(GIC_FDT_IRQ_TYPE_SPI);
>>> +            irq_attr[3 * i + 1] = cpu_to_be32(irq_number);
>>> +            irq_attr[3 * i + 2] = cpu_to_be32(GIC_FDT_IRQ_FLAGS_LEVEL_HI);
>>
>> I think this part is not generic enough. How can you assume the IRQs are
>> level or edge? Can't you copy the source interrupt properties and just
>> overwrite the IRQ number?
> 
> I have to check that. I wouldn't be surprised if the virtualized GIC supports
> only a subset of all possible flags and/or remaps interrupt types too.
> 
> I see the AMD XGBE driver looks at VFIO_IRQ_INFO_AUTOMASKED to choose
> between GIC_FDT_IRQ_FLAGS_LEVEL_HI and GIC_FDT_IRQ_FLAGS_EDGE_LO_HI.
> The VFIO_IRQ_INFO_* don't seem to provide more details.
Yep it tells you whether this is edge or level. That's why I initially
suggested to keep the copied values for the type.

Thanks

Eric
> There are also no users of GIC_FDT_IRQ_FLAGS_LEVEL_LO and
> GIC_FDT_IRQ_FLAGS_EDGE_HI_LO in the Qemu sources...
> 
> So probably following the AMD XGBE example is the way forward?
> 
> Thanks!
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

Re: [Qemu-devel] [PATCH v3 4/4] hw/arm/sysbus-fdt: Add support for instantiating generic devices

[Geert Uytterhoeven]

Hi Eric,

On Tue, Aug 7, 2018 at 7:21 PM Auger Eric <eric.auger@redhat.com> wrote:
> On 08/07/2018 05:32 PM, Geert Uytterhoeven wrote:
> > On Tue, Aug 7, 2018 at 4:19 PM Auger Eric <eric.auger@redhat.com> wrote:
> >> On 07/25/2018 04:34 PM, Geert Uytterhoeven wrote:
> >>> Add a fallback for instantiating generic devices without a type-specific
> >>> or compatible-specific instantation method.  This will be used when no
> >>> other match is found.
> >>>
> >>> The generic instantation method creates a device node with "reg" and
> >>> (optional) "interrupts" properties, and copies the "compatible"
> >>> property and other optional properties from the host.

> >>> --- a/hw/arm/sysbus-fdt.c
> >>> +++ b/hw/arm/sysbus-fdt.c
> >>> @@ -415,6 +415,99 @@ static int add_amd_xgbe_fdt_node(SysBusDevice *sbdev, void *opaque)
> >>>      return 0;
> >>>  }
> >>>
> >>> +static HostProperty generic_copied_properties[] = {
> >>> +    {"compatible", false},
> >>> +    {"#gpio-cells", true},
> >>> +    {"gpio-controller", true},
> >>> +    {"#interrupt-cells", true},
> >>> +    {"interrupt-controller", true},
> >>> +    {"interrupt-names", true},
> >>
> >> I think we would need to enumerate the other source properties which
> >> were not copied to the guest fdt and either warn the userspace those
> >> were omitted or fail. We may end up generating an incomplete guest dt
> >> node that may not work properly.
> >
> > The list above is quite generic, so it is expected that some of the optional
> > properties (marked "true") cannot be copied. Hence warning about them
> > will be noisy, and confuse users.  Failing is definitely the wrong thing
> > to do.
>
> I was not talking about those listed here and optional. Those ones are
> taken care of. I was rather talking about potential other ones, present
> on host side and not copied on guest side. For instance, let's say your
> host dt node has a clocks property. You will attempt to create a guest
> dt node without this property and obviously the device will not work
> properly on guest side. The end user attempting this assignment does not
> get any warning on guest launch. Maybe the driver on guest side will be
> cool enough to issue a warning but we cannot really rely on this
> assumption. So from a maintenance point of view, it looks not
> manageable. I think we should checl all props found in the host dt node
> are considered and copied into the guest node. Otherwise this means the
> host dt node does not belong to the category of a "simple" one and thus
> cannot use this creation function.

It depends. And that makes it difficult to come up with a sensible
detection system for notifying the user, while avoiding too many false
positives and negatives.

Properties like "clocks" typically use phandles, which means the node
they're pointing to should be copied, too, possibly involving rewriting
like with interrupts. Hence I think this should be left to a
device-specific instantiation method.

Furthermore, depending on the SoC, some DT properties should be ignored,
and must not be copied.
Examples are:
  1. "power-domains", and optionally "clocks", when the device is part of a
     power and/or clock domain.
     Power management must be handled by the host, cfr. commit
     415eb9fc0e23071f ("vfio: platform: Fix using devices in PM Domains",
     https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=415eb9fc0e23071f).
     That is another reason why we are replacing explicit clock handling
     for power management by Runtime PM in the individual drivers, cfr.
     e.g. commit 1ecd34ddf63ef1d4 ("ata: sata_rcar: Add rudimentary Runtime
     PM support") in linux-next
     (https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=1ecd34ddf63ef1d4).
     (the first reason is abstracting power management, which may differ
      among SoCs using the same IP core).
  2. "resets", pointing to a reset controller, as reset must be handled by
     the host's vfio driver to restore the device to a pristine state
     before/after virtualization.
     See also "[PATCH v3 2/2] vfio: platform: Add generic DT reset support"
     (https://www.spinics.net/lists/devicetree/msg223516.html).

> > If the host lacks a property that is mandatory for a specific device, the
> > device won't have worked on the host before neither, right?
> >
> > The major issue remains that an incomplete guest DT node may be generated
> > if the list above lacks certain needed properties, or if subnodes are
> > needed.
> > I expect the guest OS driver would complain about the missing parts, though.
> > In that case, either the list should be extended, or a device-specific
> > instantiation method should be written.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [Qemu-devel] [PATCH v3 4/4] hw/arm/sysbus-fdt: Add support for instantiating generic devices

[Auger Eric]

Hi Geert,

On 08/08/2018 02:59 PM, Geert Uytterhoeven wrote:
> Hi Eric,
> 
> On Tue, Aug 7, 2018 at 7:21 PM Auger Eric <eric.auger@redhat.com> wrote:
>> On 08/07/2018 05:32 PM, Geert Uytterhoeven wrote:
>>> On Tue, Aug 7, 2018 at 4:19 PM Auger Eric <eric.auger@redhat.com> wrote:
>>>> On 07/25/2018 04:34 PM, Geert Uytterhoeven wrote:
>>>>> Add a fallback for instantiating generic devices without a type-specific
>>>>> or compatible-specific instantation method.  This will be used when no
>>>>> other match is found.
>>>>>
>>>>> The generic instantation method creates a device node with "reg" and
>>>>> (optional) "interrupts" properties, and copies the "compatible"
>>>>> property and other optional properties from the host.
> 
>>>>> --- a/hw/arm/sysbus-fdt.c
>>>>> +++ b/hw/arm/sysbus-fdt.c
>>>>> @@ -415,6 +415,99 @@ static int add_amd_xgbe_fdt_node(SysBusDevice *sbdev, void *opaque)
>>>>>      return 0;
>>>>>  }
>>>>>
>>>>> +static HostProperty generic_copied_properties[] = {
>>>>> +    {"compatible", false},
>>>>> +    {"#gpio-cells", true},
>>>>> +    {"gpio-controller", true},
>>>>> +    {"#interrupt-cells", true},
>>>>> +    {"interrupt-controller", true},
>>>>> +    {"interrupt-names", true},
>>>>
>>>> I think we would need to enumerate the other source properties which
>>>> were not copied to the guest fdt and either warn the userspace those
>>>> were omitted or fail. We may end up generating an incomplete guest dt
>>>> node that may not work properly.
>>>
>>> The list above is quite generic, so it is expected that some of the optional
>>> properties (marked "true") cannot be copied. Hence warning about them
>>> will be noisy, and confuse users.  Failing is definitely the wrong thing
>>> to do.
>>
>> I was not talking about those listed here and optional. Those ones are
>> taken care of. I was rather talking about potential other ones, present
>> on host side and not copied on guest side. For instance, let's say your
>> host dt node has a clocks property. You will attempt to create a guest
>> dt node without this property and obviously the device will not work
>> properly on guest side. The end user attempting this assignment does not
>> get any warning on guest launch. Maybe the driver on guest side will be
>> cool enough to issue a warning but we cannot really rely on this
>> assumption. So from a maintenance point of view, it looks not
>> manageable. I think we should checl all props found in the host dt node
>> are considered and copied into the guest node. Otherwise this means the
>> host dt node does not belong to the category of a "simple" one and thus
>> cannot use this creation function.
> 
> It depends. And that makes it difficult to come up with a sensible
> detection system for notifying the user, while avoiding too many false
> positives and negatives.
> 
> Properties like "clocks" typically use phandles, which means the node
> they're pointing to should be copied, too, possibly involving rewriting
> like with interrupts. Hence I think this should be left to a
> device-specific instantiation method.
Fully agreed. But with patch 4, an end-user can try to assign this
device without writing a specific dt node creation function. The device
on guest side will not run properly and he may report a bug. Then what
do we do?

Better emitting a false positive than nothing?
> 
> Furthermore, depending on the SoC, some DT properties should be ignored,
> and must not be copied.
> Examples are:
>   1. "power-domains", and optionally "clocks", when the device is part of a
>      power and/or clock domain.
>      Power management must be handled by the host, cfr. commit
>      415eb9fc0e23071f ("vfio: platform: Fix using devices in PM Domains",
>      https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=415eb9fc0e23071f).
>      That is another reason why we are replacing explicit clock handling
>      for power management by Runtime PM in the individual drivers, cfr.
>      e.g. commit 1ecd34ddf63ef1d4 ("ata: sata_rcar: Add rudimentary Runtime
>      PM support") in linux-next
>      (https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=1ecd34ddf63ef1d4).
>      (the first reason is abstracting power management, which may differ
>       among SoCs using the same IP core).
>   2. "resets", pointing to a reset controller, as reset must be handled by
>      the host's vfio driver to restore the device to a pristine state
>      before/after virtualization.
>      See also "[PATCH v3 2/2] vfio: platform: Add generic DT reset support"
>      (https://www.spinics.net/lists/devicetree/msg223516.html).

Yes. maybe this should be handled by using a white list of properties
that are safe not to be copied? This is a vast topic.

Wouldn't it make sense to try getting 1-3 upstream separately as this
last patch seems more controversial and independent?

Thanks

Eric
> 
>>> If the host lacks a property that is mandatory for a specific device, the
>>> device won't have worked on the host before neither, right?
>>>
>>> The major issue remains that an incomplete guest DT node may be generated
>>> if the list above lacks certain needed properties, or if subnodes are
>>> needed.
>>> I expect the guest OS driver would complain about the missing parts, though.
>>> In that case, either the list should be extended, or a device-specific
>>> instantiation method should be written.
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

Re: [Qemu-devel] [PATCH v3 4/4] hw/arm/sysbus-fdt: Add support for instantiating generic devices

[Geert Uytterhoeven]

Hi Eric,

On Wed, Aug 8, 2018 at 3:16 PM Auger Eric <eric.auger@redhat.com> wrote:
> On 08/08/2018 02:59 PM, Geert Uytterhoeven wrote:
> > On Tue, Aug 7, 2018 at 7:21 PM Auger Eric <eric.auger@redhat.com> wrote:
> >> On 08/07/2018 05:32 PM, Geert Uytterhoeven wrote:
> >>> On Tue, Aug 7, 2018 at 4:19 PM Auger Eric <eric.auger@redhat.com> wrote:
> >>>> On 07/25/2018 04:34 PM, Geert Uytterhoeven wrote:
> >>>>> Add a fallback for instantiating generic devices without a type-specific
> >>>>> or compatible-specific instantation method.  This will be used when no
> >>>>> other match is found.
> >>>>>
> >>>>> The generic instantation method creates a device node with "reg" and
> >>>>> (optional) "interrupts" properties, and copies the "compatible"
> >>>>> property and other optional properties from the host.

[...]

> Wouldn't it make sense to try getting 1-3 upstream separately as this
> last patch seems more controversial and independent?

I agree. I will rework and resubmit after my holidays.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds