[Qemu-devel] [PATCH 0/2] arm/virt: Propagate pcie DMA coherency

[Qemu-devel] [PATCH 0/2] arm/virt: Propagate pcie DMA coherency

[Bogdan Purcareata]

A PCI device is marked as coherent according to the "dma-coherent"
attribute of itself or of the associated controller. This property will
determine how it will issue DMA transactions, and how it will map its DMA
memory. When passing the PCI device to the guest using VFIO PCI, this
isn't taken into account. QEMU emulates a single generic pcie controller,
regardless of how many PCI devices are passed to the guest and to which host
controllers they are attached to. Also, the device need not be described in
the device tree at all (e.g. e1000e).

In a scenario involving aarch64 and ARM SMMU v2, I noticed that without the
dma-coherent property of the guest pcie controller, the eth interface
descriptor rings read stale data without explicit flushes - hence causing
VFIO PCI to not work. Since the host pcie controllers and interconnect are
coherent and described in the host device tree accordingly, this needs to
be passed to the guest as well.

Add a routine and device tree helper to configure the guest pcie controller
based on the host device tree.

Bogdan Purcareata (2):
  device_tree: introduce qemu_fdt_node_path_prop
  arm/virt: Mark pcie controller node as dma-coherent

 device_tree.c                | 59 ++++++++++++++++++++++++++++++++++++++++++++
 hw/arm/virt.c                | 31 +++++++++++++++++++++++
 include/sysemu/device_tree.h | 20 +++++++++++++++
 3 files changed, 110 insertions(+)

-- 
1.9.1

[Qemu-devel] [PATCH 1/2] device_tree: introduce qemu_fdt_node_path_prop

[Bogdan Purcareata]

This new helper routine returns a NULL terminated array of
node paths matching a property and, optionally, a name.

Signed-off-by: Bogdan Purcareata <bogdan.purcareata@nxp.com>
---
 device_tree.c                | 59 ++++++++++++++++++++++++++++++++++++++++++++
 include/sysemu/device_tree.h | 20 +++++++++++++++
 2 files changed, 79 insertions(+)

diff --git a/device_tree.c b/device_tree.c
index ccba1fd..85c81cc 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -281,6 +281,65 @@ char **qemu_fdt_node_path(void *fdt, const char *name, char *compat,
     return path_array;
 }
 
+char **qemu_fdt_node_path_prop(void *fdt, const char *name,
+       const char *pname, const void *pval, int plen, Error **errp)
+{
+    int offset, len, ret;
+    const char *iter_name;
+    unsigned int path_len = 16, n = 0;
+    GSList *path_list = NULL, *iter;
+    char **path_array;
+
+    for (offset = fdt_node_offset_by_prop_value(fdt, 0, pname, pval, plen);
+    offset >= 0;
+    offset = fdt_node_offset_by_prop_value(fdt, offset, pname, pval, plen)) {
+
+        iter_name = fdt_get_name(fdt, offset, &len);
+
+        if (!iter_name) {
+            continue;
+        }
+
+        /* if the node doesn't match the name, move on */
+        if (name && strncmp(iter_name, name, strlen(name))) {
+            continue;
+        }
+
+        char *path;
+
+        path = g_malloc(path_len);
+        while ((ret = fdt_get_path(fdt, offset, path, path_len)) ==
+                    -FDT_ERR_NOSPACE) {
+            path_len += 16;
+            path = g_realloc(path, path_len);
+        }
+
+        path_list = g_slist_prepend(path_list, path);
+        n++;
+    }
+
+    if (offset < 0 && offset != -FDT_ERR_NOTFOUND) {
+        error_setg(errp, "%s: abort parsing dt for %s/%s: %s",
+                   __func__, name, pname, fdt_strerror(offset));
+        for (iter = path_list; iter; iter = iter->next) {
+            g_free(iter->data);
+        }
+        g_slist_free(path_list);
+        return NULL;
+    }
+
+    path_array = g_new(char *, n + 1);
+    path_array[n--] = NULL;
+
+    for (iter = path_list; iter; iter = iter->next) {
+        path_array[n--] = iter->data;
+    }
+
+    g_slist_free(path_list);
+
+    return path_array;
+}
+
 int qemu_fdt_setprop(void *fdt, const char *node_path,
                      const char *property, const void *val, int size)
 {
diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
index 705650a..03c8193 100644
--- a/include/sysemu/device_tree.h
+++ b/include/sysemu/device_tree.h
@@ -43,6 +43,26 @@ void *load_device_tree_from_sysfs(void);
 char **qemu_fdt_node_path(void *fdt, const char *name, char *compat,
                           Error **errp);
 
+/**
+ * qemu_fdt_node_path_property: return the paths of nodes matching a given
+ * property and, optionally, a name string
+ * @fdt: pointer to the dt blob
+ * @name: node name (can be NULL) - if NULL, will return all nodes mathing prop
+ * @propname: property name
+ * @propvalue: property value (can be NULL)
+ * @proplen: property length (can be 0)
+ * @errp: handle to an error object
+ *
+ * returns a newly allocated NULL-terminated array of node paths.
+ * Use g_strfreev() to free it. If one or more nodes were found, the
+ * array contains the path of each node and the last element equals to
+ * NULL. If there is no error but no matching node was found, the
+ * returned array contains a single element equal to NULL. If an error
+ * was encountered when parsing the blob, the function returns NULL
+ */
+char **qemu_fdt_node_path_prop(void *fdt, const char *name,
+    const char *propname, const void *propvalue, int proplen, Error **errp);
+
 int qemu_fdt_setprop(void *fdt, const char *node_path,
                      const char *property, const void *val, int size);
 int qemu_fdt_setprop_cell(void *fdt, const char *node_path,
-- 
1.9.1

[Qemu-devel] [PATCH 2/2] arm/virt: Mark pcie controller node as dma-coherent

[Bogdan Purcareata]

A PCI device is marked either as coherent or non-coherent based on the pcie
controller "dma-coherent" property. This is further used when configuring the
IOMMU ops for the device DMA resources (e.g. descriptor rings, for e1000e).

This dma-coherent property needs to be configured in the guest environment,
in case there's a directly assigned VFIO PCI device. Since the guest only
receives one emulated pcie controller bus - regardless of the host
configuration - add this property if there's at least one host pcie host
controller that is DMA coherent (this implies that the host interconnect
is coherent as well).

Signed-off-by: Bogdan Purcareata <bogdan.purcareata@nxp.com>
---
 hw/arm/virt.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 371e3a7..b640174 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -867,6 +867,32 @@ static void create_pcie_irq_map(const VirtBoardInfo *vbi, uint32_t gic_phandle,
                            0x7           /* PCI irq */);
 }
 
+static bool is_host_pcie_dma_coherent(void)
+{
+    void *host_fdt;
+    char **node_path;
+    bool ret = false;
+
+    host_fdt = load_device_tree_from_sysfs();
+
+    node_path = qemu_fdt_node_path_prop(host_fdt, "pcie", "dma-coherent",
+            NULL, 0, &error_fatal);
+
+    /* no pcie controllers found on host, therefore non dma-coherent */
+    if (!node_path || !node_path[0]) {
+        goto out;
+    }
+
+    /* for now, if at least one pcie node is dma-coherent,
+     * it is considered that the host is dma-coherent with respect to pcie */
+    ret = true;
+
+out:
+    g_strfreev(node_path);
+    g_free(host_fdt);
+    return ret;
+}
+
 static void create_pcie(const VirtBoardInfo *vbi, qemu_irq *pic,
                         bool use_highmem)
 {
@@ -981,6 +1007,11 @@ static void create_pcie(const VirtBoardInfo *vbi, qemu_irq *pic,
     }
 
     qemu_fdt_setprop_cell(vbi->fdt, nodename, "#interrupt-cells", 1);
+
+    if (is_host_pcie_dma_coherent()) {
+        qemu_fdt_setprop(vbi->fdt, nodename, "dma-coherent", NULL, 0);
+    }
+
     create_pcie_irq_map(vbi, vbi->gic_phandle, irq, nodename);
 
     g_free(nodename);
-- 
1.9.1

Re: [Qemu-devel] [PATCH 2/2] arm/virt: Mark pcie controller node as dma-coherent

[Peter Maydell]

On 2 June 2016 at 13:26, Bogdan Purcareata <bogdan.purcareata@nxp.com> wrote:
> A PCI device is marked either as coherent or non-coherent based on the pcie
> controller "dma-coherent" property. This is further used when configuring the
> IOMMU ops for the device DMA resources (e.g. descriptor rings, for e1000e).
>
> This dma-coherent property needs to be configured in the guest environment,
> in case there's a directly assigned VFIO PCI device. Since the guest only
> receives one emulated pcie controller bus - regardless of the host
> configuration - add this property if there's at least one host pcie host
> controller that is DMA coherent (this implies that the host interconnect
> is coherent as well).

This patch seems to change the property of the emulated PCIe controller
based on the host PCIe controller even if we're not doing any PCIe
passthrough at all. That seems definitely wrong to me.

(Should the purely-emulated case be marked DMA-coherent anyway?
I forget the fiddly details...)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/2] arm/virt: Mark pcie controller node as dma-coherent

[Alexander Graf]

On 02.06.16 14:32, Peter Maydell wrote:
> On 2 June 2016 at 13:26, Bogdan Purcareata <bogdan.purcareata@nxp.com> wrote:
>> A PCI device is marked either as coherent or non-coherent based on the pcie
>> controller "dma-coherent" property. This is further used when configuring the
>> IOMMU ops for the device DMA resources (e.g. descriptor rings, for e1000e).
>>
>> This dma-coherent property needs to be configured in the guest environment,
>> in case there's a directly assigned VFIO PCI device. Since the guest only
>> receives one emulated pcie controller bus - regardless of the host
>> configuration - add this property if there's at least one host pcie host
>> controller that is DMA coherent (this implies that the host interconnect
>> is coherent as well).
> 
> This patch seems to change the property of the emulated PCIe controller
> based on the host PCIe controller even if we're not doing any PCIe
> passthrough at all. That seems definitely wrong to me.
> 
> (Should the purely-emulated case be marked DMA-coherent anyway?
> I forget the fiddly details...)

I do too, let's involve a few people who know :). Not exposing it as
coherent is definitely wrong, but whether "dma-coherent" is the right
choice I don't know.


Alex

Re: [Qemu-devel] [PATCH 2/2] arm/virt: Mark pcie controller node as dma-coherent

[Mihai Claudiu Caraman]

> -----Original Message-----
> From: Qemu-devel [mailto:qemu-devel-bounces+mihai.caraman=freescale.com@nongnu.org] On Behalf Of Peter Maydell
> Sent: Thursday, June 02, 2016 3:33 PM
> To: Bogdan Purcareata <bogdan.purcareata@nxp.com>
> Cc: QEMU Developers <qemu-devel@nongnu.org>; Peter Crosthwaite <crosthwaite.peter@gmail.com>; Alexander Graf <agraf@suse.de>; qemu-arm <qemu-arm@nongnu.org>; Eric Auger <eric.auger@linaro.org>
> Subject: Re: [Qemu-devel] [PATCH 2/2] arm/virt: Mark pcie controller node as dma-coherent
> 
> On 2 June 2016 at 13:26, Bogdan Purcareata <bogdan.purcareata@nxp.com> wrote:
> > A PCI device is marked either as coherent or non-coherent based on the 
> > pcie controller "dma-coherent" property. This is further used when 
> > configuring the IOMMU ops for the device DMA resources (e.g. descriptor rings, for e1000e).
> >
> > This dma-coherent property needs to be configured in the guest 
> > environment, in case there's a directly assigned VFIO PCI device. 
> > Since the guest only receives one emulated pcie controller bus - 
> > regardless of the host configuration - add this property if there's at 
> > least one host pcie host controller that is DMA coherent (this implies 
> > that the host interconnect is coherent as well).
> 
> This patch seems to change the property of the emulated PCIe controller based on the host PCIe controller even if we're not doing any PCIe passthrough at all. That seems definitely wrong to me.
> 
> (Should the purely-emulated case be marked DMA-coherent anyway?
> I forget the fiddly details...)
> 
> thanks
> -- PMM

In particular for virtual and emulated devices the host CPU behaves as a DMA coherent 'device'. This should have been stated in patch description.

Regards,
Mike

Re: [Qemu-devel] [PATCH 2/2] arm/virt: Mark pcie controller node as dma-coherent

[Peter Maydell]

On 3 June 2016 at 15:22, Mihai Claudiu Caraman <mike.caraman@nxp.com> wrote:
> In particular for virtual and emulated devices the host CPU behaves
> as a DMA coherent 'device'. This should have been stated in patch
> description.

Wouldn't that imply that we should just always have the "dma-coherent"
property set, and we don't need to do any of the messing around looking
at the host sysfs ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/2] arm/virt: Mark pcie controller node as dma-coherent

[agraf]

On 2016-06-03 16:37, Peter Maydell wrote:
> On 3 June 2016 at 15:22, Mihai Claudiu Caraman <mike.caraman@nxp.com> 
> wrote:
>> In particular for virtual and emulated devices the host CPU behaves
>> as a DMA coherent 'device'. This should have been stated in patch
>> description.
> 
> Wouldn't that imply that we should just always have the "dma-coherent"
> property set, and we don't need to do any of the messing around looking
> at the host sysfs ?

What happens with VFIO targets that don't have coherent DMA on their PCI 
bus then? I think eventually we'll need to be able to spawn a second bus 
with explicit coherency setting.


Alex

Re: [Qemu-devel] [PATCH 2/2] arm/virt: Mark pcie controller node as dma-coherent

[Mihai Claudiu Caraman]

> -----Original Message-----
> From: Peter Maydell [mailto:peter.maydell@linaro.org] 
> Sent: Friday, June 03, 2016 5:38 PM
> To: Mihai Claudiu Caraman <mike.caraman@nxp.com>
> Cc: Bogdan Purcareata <bogdan.purcareata@nxp.com>; QEMU Developers <qemu-devel@nongnu.org>; Peter Crosthwaite <crosthwaite.peter@gmail.com>; Alexander Graf <agraf@suse.de>; qemu-arm <qemu-arm@nongnu.org>; Eric Auger <eric.auger@linaro.org>
> Subject: Re: [Qemu-devel] [PATCH 2/2] arm/virt: Mark pcie controller node as dma-coherent
> 
> On 3 June 2016 at 15:22, Mihai Claudiu Caraman <mike.caraman@nxp.com> wrote:
> > In particular for virtual and emulated devices the host CPU behaves as 
> > a DMA coherent 'device'. This should have been stated in patch 
> > description.
> 
> Wouldn't that imply that we should just always have the "dma-coherent"
> property set, and we don't need to do any of the messing around looking at the host sysfs ?
> 
> thanks
> -- PMM

We can always set "dma-coherent" for virtual and emulated devices but not for passthrough devices. So we can't have one PCIe controller for all devices marked as "dma-coherent".

Regards,
Mike

Re: [Qemu-devel] [PATCH 2/2] arm/virt: Mark pcie controller node as dma-coherent

[Alexander Graf]

On 06/03/2016 05:16 PM, Mihai Claudiu Caraman wrote:
>> -----Original Message-----
>> From: Peter Maydell [mailto:peter.maydell@linaro.org]
>> Sent: Friday, June 03, 2016 5:38 PM
>> To: Mihai Claudiu Caraman <mike.caraman@nxp.com>
>> Cc: Bogdan Purcareata <bogdan.purcareata@nxp.com>; QEMU Developers <qemu-devel@nongnu.org>; Peter Crosthwaite <crosthwaite.peter@gmail.com>; Alexander Graf <agraf@suse.de>; qemu-arm <qemu-arm@nongnu.org>; Eric Auger <eric.auger@linaro.org>
>> Subject: Re: [Qemu-devel] [PATCH 2/2] arm/virt: Mark pcie controller node as dma-coherent
>>
>> On 3 June 2016 at 15:22, Mihai Claudiu Caraman <mike.caraman@nxp.com> wrote:
>>> In particular for virtual and emulated devices the host CPU behaves as
>>> a DMA coherent 'device'. This should have been stated in patch
>>> description.
>> Wouldn't that imply that we should just always have the "dma-coherent"
>> property set, and we don't need to do any of the messing around looking at the host sysfs ?
>>
>> thanks
>> -- PMM
> We can always set "dma-coherent" for virtual and emulated devices but not for passthrough devices. So we can't have one PCIe controller for all devices marked as "dma-coherent".

The original patch is about the case where PCI is already cache coherent 
on the host.

I think at the end of the day this is simply outside of QEMU's scope to 
decide. What we can do is set dma-coherent per default (if Will and Ard 
agree) on the default PCIe bus and add code that allows to spawn a 
secondary PCIe bus which can then have different dma-coherent attributes 
and that you can then plug your non-coherent vfio devices into.


Alex

Re: [Qemu-devel] [PATCH 2/2] arm/virt: Mark pcie controller node as dma-coherent

[Mihai Claudiu Caraman]

-----Original Message-----
From: Alexander Graf [mailto:agraf@suse.de] 
Sent: Friday, June 03, 2016 6:26 PM
To: Mihai Claudiu Caraman <mike.caraman@nxp.com>; Peter Maydell <peter.maydell@linaro.org>
> Cc: Bogdan Purcareata <bogdan.purcareata@nxp.com>; QEMU Developers <qemu-devel@nongnu.org>; Peter Crosthwaite <crosthwaite.peter@gmail.com>; qemu-arm <qemu-arm@nongnu.org>; Eric Auger <eric.auger@linaro.org>
> Subject: Re: [Qemu-devel] [PATCH 2/2] arm/virt: Mark pcie controller node as dma-coherent
> 
> On 06/03/2016 05:16 PM, Mihai Claudiu Caraman wrote:
> >> -----Original Message-----
> >> From: Peter Maydell [mailto:peter.maydell@linaro.org]
> >> Sent: Friday, June 03, 2016 5:38 PM
> >> To: Mihai Claudiu Caraman <mike.caraman@nxp.com>
> >> Cc: Bogdan Purcareata <bogdan.purcareata@nxp.com>; QEMU Developers 
> >> <qemu-devel@nongnu.org>; Peter Crosthwaite 
> >> <crosthwaite.peter@gmail.com>; Alexander Graf <agraf@suse.de>; 
> >> qemu-arm <qemu-arm@nongnu.org>; Eric Auger <eric.auger@linaro.org>
> >> Subject: Re: [Qemu-devel] [PATCH 2/2] arm/virt: Mark pcie controller 
> >> node as dma-coherent
> >>
> >> On 3 June 2016 at 15:22, Mihai Claudiu Caraman <mike.caraman@nxp.com> wrote:
> >>> In particular for virtual and emulated devices the host CPU behaves 
> >>> as a DMA coherent 'device'. This should have been stated in patch 
> >>> description.
> >> Wouldn't that imply that we should just always have the "dma-coherent"
> >> property set, and we don't need to do any of the messing around looking at the host sysfs ?
> >>
> >> thanks
> >> -- PMM
> > We can always set "dma-coherent" for virtual and emulated devices but not for passthrough devices. So we can't have one PCIe controller for all devices marked as "dma-coherent".
> 
> The original patch is about the case where PCI is already cache coherent on the host.
>
> I think at the end of the day this is simply outside of QEMU's scope to decide. What we can do is set dma-coherent per default (if Will and Ard
> agree) on the default PCIe bus and add code that allows to spawn a secondary PCIe bus which can then have different dma-coherent attributes and that you can then plug your non-coherent vfio devices into. 

A dma-coherent default PCIe bus looks fine.

Thanks,
Mike

Re: [Qemu-devel] [PATCH 2/2] arm/virt: Mark pcie controller node as dma-coherent

[Auger Eric]

Hi,
Le 02/06/2016 à 14:45, Alexander Graf a écrit :
> 
> 
> On 02.06.16 14:32, Peter Maydell wrote:
>> On 2 June 2016 at 13:26, Bogdan Purcareata <bogdan.purcareata@nxp.com> wrote:
>>> A PCI device is marked either as coherent or non-coherent based on the pcie
>>> controller "dma-coherent" property. This is further used when configuring the
>>> IOMMU ops for the device DMA resources (e.g. descriptor rings, for e1000e).
>>>
>>> This dma-coherent property needs to be configured in the guest environment,
>>> in case there's a directly assigned VFIO PCI device. Since the guest only
>>> receives one emulated pcie controller bus - regardless of the host
>>> configuration - add this property if there's at least one host pcie host
>>> controller that is DMA coherent (this implies that the host interconnect
>>> is coherent as well).
>>
>> This patch seems to change the property of the emulated PCIe controller
>> based on the host PCIe controller even if we're not doing any PCIe
>> passthrough at all. That seems definitely wrong to me.
>>
>> (Should the purely-emulated case be marked DMA-coherent anyway?
>> I forget the fiddly details...)
> 
> I do too, let's involve a few people who know :). Not exposing it as
> coherent is definitely wrong, but whether "dma-coherent" is the right
> choice I don't know.

For information, on AMD overdrive the PCI host controller is marked as
dma-coherent too but I do not observe any issue with 82574L (e1000e)
and it worked fine as well on X540T1 Intel NIC. I just tested with
e1000e and dma-coherent guest PCI controller and works fine too. Don't
have x540t1 anymore so can't test with that PCIe CARD.

With Intel I35OT1 and guest non dma-coherent PCIe controller I cope with
some issues when assigning the PF but I don't know yet if there is any
relationship.

Best Regards

Eric
> 
> 
> Alex
> 

Re: [Qemu-devel] [PATCH 2/2] arm/virt: Mark pcie controller node as dma-coherent

[Ard Biesheuvel]

On 2 June 2016 at 14:45, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 02.06.16 14:32, Peter Maydell wrote:
>> On 2 June 2016 at 13:26, Bogdan Purcareata <bogdan.purcareata@nxp.com> wrote:
>>> A PCI device is marked either as coherent or non-coherent based on the pcie
>>> controller "dma-coherent" property. This is further used when configuring the
>>> IOMMU ops for the device DMA resources (e.g. descriptor rings, for e1000e).
>>>
>>> This dma-coherent property needs to be configured in the guest environment,
>>> in case there's a directly assigned VFIO PCI device. Since the guest only
>>> receives one emulated pcie controller bus - regardless of the host
>>> configuration - add this property if there's at least one host pcie host
>>> controller that is DMA coherent (this implies that the host interconnect
>>> is coherent as well).
>>
>> This patch seems to change the property of the emulated PCIe controller
>> based on the host PCIe controller even if we're not doing any PCIe
>> passthrough at all. That seems definitely wrong to me.
>>
>> (Should the purely-emulated case be marked DMA-coherent anyway?
>> I forget the fiddly details...)
>
> I do too, let's involve a few people who know :). Not exposing it as
> coherent is definitely wrong, but whether "dma-coherent" is the right
> choice I don't know.
>

As far as I understand it, the purely emulated case should be marked
DMA coherent, since otherwise, guest drivers may perform cache
maintenance that the host is not expecting. This is especially harmful
if the guest invalidates the caches after a device to memory transfer,
which may result in data being lost if the data was only present in
the caches to begin with (which is the case for devices that are
emulated by the host)

Re: [Qemu-devel] [PATCH 2/2] arm/virt: Mark pcie controller node as dma-coherent

[Peter Maydell]

On 16 June 2016 at 14:58, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 2 June 2016 at 14:45, Alexander Graf <agraf@suse.de> wrote:
>> On 02.06.16 14:32, Peter Maydell wrote:
>>> This patch seems to change the property of the emulated PCIe controller
>>> based on the host PCIe controller even if we're not doing any PCIe
>>> passthrough at all. That seems definitely wrong to me.
>>>
>>> (Should the purely-emulated case be marked DMA-coherent anyway?
>>> I forget the fiddly details...)
>>
>> I do too, let's involve a few people who know :). Not exposing it as
>> coherent is definitely wrong, but whether "dma-coherent" is the right
>> choice I don't know.

> As far as I understand it, the purely emulated case should be marked
> DMA coherent, since otherwise, guest drivers may perform cache
> maintenance that the host is not expecting. This is especially harmful
> if the guest invalidates the caches after a device to memory transfer,
> which may result in data being lost if the data was only present in
> the caches to begin with (which is the case for devices that are
> emulated by the host)

So the consensus seems to be that:
 * emulated PCI devices definitely need dma-coherent
 * passthrough devices where the host controller is dma-coherent
   also need dma-coherent
 * passthrough devices where the host controller is not dma-coherent
   don't want dma-coherent, but we have to set things per-PCI-controller

Would somebody like to write a patch which just unconditionally
sets the dma-coherent property on our PCI controller dt node?
That seems a clear improvement on what we have at the moment.
We can look at whether we want to support passthrough from a
non-dma-coherent host pci controller (via a 2nd guest pci controller?)
later...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/2] arm/virt: Mark pcie controller node as dma-coherent

[Bogdan Purcareata]

On 28.06.2016 17:16, Peter Maydell wrote:
> On 16 June 2016 at 14:58, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> On 2 June 2016 at 14:45, Alexander Graf <agraf@suse.de> wrote:
>>> On 02.06.16 14:32, Peter Maydell wrote:
>>>> This patch seems to change the property of the emulated PCIe controller
>>>> based on the host PCIe controller even if we're not doing any PCIe
>>>> passthrough at all. That seems definitely wrong to me.
>>>>
>>>> (Should the purely-emulated case be marked DMA-coherent anyway?
>>>> I forget the fiddly details...)
>>>
>>> I do too, let's involve a few people who know :). Not exposing it as
>>> coherent is definitely wrong, but whether "dma-coherent" is the right
>>> choice I don't know.
>
>> As far as I understand it, the purely emulated case should be marked
>> DMA coherent, since otherwise, guest drivers may perform cache
>> maintenance that the host is not expecting. This is especially harmful
>> if the guest invalidates the caches after a device to memory transfer,
>> which may result in data being lost if the data was only present in
>> the caches to begin with (which is the case for devices that are
>> emulated by the host)
>
> So the consensus seems to be that:
>  * emulated PCI devices definitely need dma-coherent
>  * passthrough devices where the host controller is dma-coherent
>    also need dma-coherent
>  * passthrough devices where the host controller is not dma-coherent
>    don't want dma-coherent, but we have to set things per-PCI-controller
>
> Would somebody like to write a patch which just unconditionally
> sets the dma-coherent property on our PCI controller dt node?

I will send this patch later today.

Best regards,
Bogdan P.

> That seems a clear improvement on what we have at the moment.
> We can look at whether we want to support passthrough from a
> non-dma-coherent host pci controller (via a 2nd guest pci controller?)
> later...
>
> thanks
> -- PMM
>