[Qemu-devel] [PATCH] hw/arm/virt: mark the PCIe host controller as DMA coherent in the DT

[Qemu-devel] [PATCH] hw/arm/virt: mark the PCIe host controller as DMA coherent in the DT

[Ard Biesheuvel]

Since QEMU performs cacheable accesses to guest memory when doing DMA
as part of the implementation of emulated PCI devices, guest drivers
should use cacheable accesses as well when running under KVM. Since this
essentially means that emulated PCI devices are DMA coherent, set the
'dma-coherent' DT property on the PCIe host controller DT node.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 hw/arm/virt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index c5c125e9204a..6e098afd1fe5 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1021,6 +1021,7 @@ static void create_pcie(const VirtBoardInfo *vbi, qemu_irq *pic,
     qemu_fdt_setprop_cell(vbi->fdt, nodename, "#size-cells", 2);
     qemu_fdt_setprop_cells(vbi->fdt, nodename, "bus-range", 0,
                            nr_pcie_buses - 1);
+    qemu_fdt_setprop(vbi->fdt, nodename, "dma-coherent", NULL, 0);
 
     if (vbi->v2m_phandle) {
         qemu_fdt_setprop_cells(vbi->fdt, nodename, "msi-parent",
-- 
2.7.4

Re: [Qemu-devel] [PATCH] hw/arm/virt: mark the PCIe host controller as DMA coherent in the DT

[Peter Maydell]

On 28 June 2016 at 18:14, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> Since QEMU performs cacheable accesses to guest memory when doing DMA
> as part of the implementation of emulated PCI devices, guest drivers
> should use cacheable accesses as well when running under KVM. Since this
> essentially means that emulated PCI devices are DMA coherent, set the
> 'dma-coherent' DT property on the PCIe host controller DT node.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  hw/arm/virt.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index c5c125e9204a..6e098afd1fe5 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1021,6 +1021,7 @@ static void create_pcie(const VirtBoardInfo *vbi, qemu_irq *pic,
>      qemu_fdt_setprop_cell(vbi->fdt, nodename, "#size-cells", 2);
>      qemu_fdt_setprop_cells(vbi->fdt, nodename, "bus-range", 0,
>                             nr_pcie_buses - 1);
> +    qemu_fdt_setprop(vbi->fdt, nodename, "dma-coherent", NULL, 0);
>
>      if (vbi->v2m_phandle) {
>          qemu_fdt_setprop_cells(vbi->fdt, nodename, "msi-parent",
> --
> 2.7.4

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PATCH] hw/arm/virt: mark the PCIe host controller as DMA coherent in the DT

[Andrew Jones]

On Thu, Jun 30, 2016 at 03:09:53PM +0100, Peter Maydell wrote:
> On 28 June 2016 at 18:14, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > Since QEMU performs cacheable accesses to guest memory when doing DMA
> > as part of the implementation of emulated PCI devices, guest drivers
> > should use cacheable accesses as well when running under KVM. Since this
> > essentially means that emulated PCI devices are DMA coherent, set the
> > 'dma-coherent' DT property on the PCIe host controller DT node.
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  hw/arm/virt.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index c5c125e9204a..6e098afd1fe5 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -1021,6 +1021,7 @@ static void create_pcie(const VirtBoardInfo *vbi, qemu_irq *pic,
> >      qemu_fdt_setprop_cell(vbi->fdt, nodename, "#size-cells", 2);
> >      qemu_fdt_setprop_cells(vbi->fdt, nodename, "bus-range", 0,
> >                             nr_pcie_buses - 1);
> > +    qemu_fdt_setprop(vbi->fdt, nodename, "dma-coherent", NULL, 0);
> >
> >      if (vbi->v2m_phandle) {
> >          qemu_fdt_setprop_cells(vbi->fdt, nodename, "msi-parent",
> > --
> > 2.7.4
> 
> Applied, thanks.
> 
> -- PMM
>

I might have mentioned in the commit message that the ACPI generation
already does this, as _CCA is set to 1, added with commit bc64b96c
(assuming I'm right, and a value of 1 there is the ACPI equivalent of
 this patch)

bc64b96c's commit message is also lacking, in the fact it doesn't
state why the value of 1 is chosen, only that the attribute is
compulsory, which I presume could have been added with the value 0
to satisfy that.

Anyway, I just wanted to point out that I *think* we're fine for
ACPI. Perhaps I'm the only one who didn't know that already though...

Thanks,
drew

Re: [Qemu-devel] [PATCH] hw/arm/virt: mark the PCIe host controller as DMA coherent in the DT

[Ard Biesheuvel]

On 1 July 2016 at 13:40, Andrew Jones <drjones@redhat.com> wrote:
> On Thu, Jun 30, 2016 at 03:09:53PM +0100, Peter Maydell wrote:
>> On 28 June 2016 at 18:14, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> > Since QEMU performs cacheable accesses to guest memory when doing DMA
>> > as part of the implementation of emulated PCI devices, guest drivers
>> > should use cacheable accesses as well when running under KVM. Since this
>> > essentially means that emulated PCI devices are DMA coherent, set the
>> > 'dma-coherent' DT property on the PCIe host controller DT node.
>> >
>> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> > ---
>> >  hw/arm/virt.c | 1 +
>> >  1 file changed, 1 insertion(+)
>> >
>> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> > index c5c125e9204a..6e098afd1fe5 100644
>> > --- a/hw/arm/virt.c
>> > +++ b/hw/arm/virt.c
>> > @@ -1021,6 +1021,7 @@ static void create_pcie(const VirtBoardInfo *vbi, qemu_irq *pic,
>> >      qemu_fdt_setprop_cell(vbi->fdt, nodename, "#size-cells", 2);
>> >      qemu_fdt_setprop_cells(vbi->fdt, nodename, "bus-range", 0,
>> >                             nr_pcie_buses - 1);
>> > +    qemu_fdt_setprop(vbi->fdt, nodename, "dma-coherent", NULL, 0);
>> >
>> >      if (vbi->v2m_phandle) {
>> >          qemu_fdt_setprop_cells(vbi->fdt, nodename, "msi-parent",
>> > --
>> > 2.7.4
>>
>> Applied, thanks.
>>
>> -- PMM
>>
>
> I might have mentioned in the commit message that the ACPI generation
> already does this, as _CCA is set to 1, added with commit bc64b96c
> (assuming I'm right, and a value of 1 there is the ACPI equivalent of
>  this patch)
>
> bc64b96c's commit message is also lacking, in the fact it doesn't
> state why the value of 1 is chosen, only that the attribute is
> compulsory, which I presume could have been added with the value 0
> to satisfy that.
>
> Anyway, I just wanted to point out that I *think* we're fine for
> ACPI. Perhaps I'm the only one who didn't know that already though...
>

I did a quick test to compare:

(a) mach-virt in DT mode without this patch
(b) mach-virt in DT mode with this patch
(c) mach-virt in ACPI mode (with bc64b96c applied)

with '-device nec-usb-xhci -device usb-kbd' added to the QEMU command line

(b) and (c) detect the keyboard, whereas (a) does not. So I guess that
solves the USB mystery (but perhaps Alex could confirm?)

Re: [Qemu-devel] [PATCH] hw/arm/virt: mark the PCIe host controller as DMA coherent in the DT

[Peter Maydell]

On 1 July 2016 at 12:40, Andrew Jones <drjones@redhat.com> wrote:
> I might have mentioned in the commit message that the ACPI generation
> already does this, as _CCA is set to 1, added with commit bc64b96c
> (assuming I'm right, and a value of 1 there is the ACPI equivalent of
>  this patch)
>
> bc64b96c's commit message is also lacking, in the fact it doesn't
> state why the value of 1 is chosen, only that the attribute is
> compulsory, which I presume could have been added with the value 0
> to satisfy that.
>
> Anyway, I just wanted to point out that I *think* we're fine for
> ACPI. Perhaps I'm the only one who didn't know that already though...

Thanks for the update: I've edited the commit message to add:
# This brings the DT description into line with the ACPI description,
# which already marks the PCI bridge as cache coherent (see commit
# bc64b96c984abf).

thanks
-- PMM