Linux Virtualization - Jan 2017 - [PATCH v2 2/2] vring: Force use of DMA API for ARM-based systems

On Mon, Jan 16, 2017 at 02:34:08PM +0000, Will Deacon
wrote:
> On Mon, Jan 16, 2017 at 04:27:28PM +0200, Michael S. Tsirkin wrote:
> > On Mon, Jan 16, 2017 at 02:21:03PM +0000, Will Deacon wrote:
> > > On Mon, Jan 16, 2017 at 04:18:03PM +0200, Michael S. Tsirkin
wrote:
> > > > On Mon, Jan 16, 2017 at 10:40:28AM +0000, Will Deacon wrote:
> > > > > On Fri, Jan 13, 2017 at 08:23:35PM +0200, Michael S.
Tsirkin wrote:
> > > > > > On Fri, Jan 13, 2017 at 05:21:54PM +0000, Will
Deacon wrote:
> > > > > > > On Fri, Jan 13, 2017 at 06:46:32PM +0200,
Michael S. Tsirkin wrote:
> > > > > > > > On Fri, Jan 13, 2017 at 09:25:22AM
+0000, Will Deacon wrote:
> > > > > > > > > On Fri, Jan 13, 2017 at 12:12:56AM
+0200, Michael S. Tsirkin wrote:
> > > > > > > > > > I'd rather people
didn't use SMMU with legacy devices.
> > > > > > > > > 
> > > > > > > > > I'm afraid we've been doing
that for two years and the model already
> > > > > > > > > exists in a mature state, being
actively used for development and
> > > > > > > > > validation by ARM and our partners.
One of the big things its used for
> > > > > > > > > is to develop SMMU and GIC (our
interrupt controller) code with PCI, so
> > > > > > > > > dropping the SMMU from the picture
isn't an option.
> > > > > > > > 
> > > > > > > > Oh so this fixes a regression?  This is
something I didn't realize.
> > > > > > > 
> > > > > > > Yes, thanks. The regression came about
because we implemented SMMU-backed
> > > > > > > DMA ops and only then was it apparent that
the virtio stuff was bypassing
> > > > > > > even with translation enabled (because it
wasn't using the DMA API).
> > > > > > 
> > > > > > Could you point out a commit ID?
> > > > > 
> > > > > There has been a fair amount of work in this area
recently, but you're
> > > > > probably after something like 876945dbf649
("arm64: Hook up IOMMU dma_ops")
> > > > > as the culprit, which is the point at which we started
to swizzle DMA
> > > > > ops for devices upstream of an SMMU automatically.
> > > > > 
> > > > > > > > A "Fixes:" tag can't hurt
here.  I then wonder
> > > > > > > > might DMA ops ever use a DMA address
which isn't a physical address
> > > > > > > > from QEMU point of view? If that
happens, this hack breaks
> > > > > > > > because in legacy mode QEMU still uses
the GPA.
> > > > > > > 
> > > > > > > If QEMU doesn't advertise an SMMU, then
it will work fine with the GPA,
> > > > > > > because we won't swizzle the DMA ops for
the master device. If QEMU does
> > > > > > > advertise an SMMU, then we'll allocate
DMA addresses to fit within the
> > > > > > > the intersection of the SMMU aperture and
device's DMA mask.
> > > > > > 
> > > > > > 
> > > > > > Right but doesn't just poking from qemu into
phys addresses work
> > > > > > anymore? It used to ...
> > > > > 
> > > > > Provided that there's no SMMU, then it will
continue to work. and my
> > > > > understanding (from talking to Peter Maydell) is that
qemu doesn't model
> > > > > an SMMU for ARM-based machines.
> > > > > 
> > > > 
> > > > So how come people report failures due to presence of SMMU?
> > > > Using some other hypervisor?
> > > 
> > > The failures are reported on the ARM fastmodel (a complete system
> > > emulation that runs on an x86 box), where an SMMU *is* present
> > > downstream of the virtio-pci masters. There's no qemu
involved there.
> > > 
> > I see. And this hypervisor actually coded up looking up
> > translations in the SMMU unconditionally for legacy devices,
> > and this worked as long as guest didn't touch the SMMU?
> 
> Well, the fastmodel isn't a hypervisor really. It's a full system
emulation,
> so it's better to think of it like a piece of hardware. For example,
you
> could run KVM on the fastmodel. But yes, when Linux didn't swizzle the
> DMA ops to point at the SMMU, then everything defaults to bypass (because
> that's the default behaviour of the SMMU driver -- this is configurable
> on the command line) which is why things used to work.
> 
> Will
I would be a bit happier if Linux checked virtio iommu quirk and skipped
the DMA ops thing then. It's a bit ugly but at least it's consistently
ugly.  To get clean emulation you would then use a modern device.

-- 
MST

On Thu, Jan 19, 2017 at 11:51:06PM +0200, Michael S. Tsirkin
wrote:
> On Mon, Jan 16, 2017 at 02:34:08PM +0000, Will Deacon wrote:
> > On Mon, Jan 16, 2017 at 04:27:28PM +0200, Michael S. Tsirkin wrote:
> > > On Mon, Jan 16, 2017 at 02:21:03PM +0000, Will Deacon wrote:
> > > > On Mon, Jan 16, 2017 at 04:18:03PM +0200, Michael S. Tsirkin
wrote:
> > > > > On Mon, Jan 16, 2017 at 10:40:28AM +0000, Will Deacon
wrote:
> > > > > > On Fri, Jan 13, 2017 at 08:23:35PM +0200, Michael
S. Tsirkin wrote:
> > > > > > > On Fri, Jan 13, 2017 at 05:21:54PM +0000,
Will Deacon wrote:
> > > > > > > > On Fri, Jan 13, 2017 at 06:46:32PM
+0200, Michael S. Tsirkin wrote:
> > > > > > > > > On Fri, Jan 13, 2017 at 09:25:22AM
+0000, Will Deacon wrote:
> > > > > > > > > > On Fri, Jan 13, 2017 at
12:12:56AM +0200, Michael S. Tsirkin wrote:
> > > > > > > > > > > I'd rather people
didn't use SMMU with legacy devices.
> > > > > > > > > > 
> > > > > > > > > > I'm afraid we've been
doing that for two years and the model already
> > > > > > > > > > exists in a mature state,
being actively used for development and
> > > > > > > > > > validation by ARM and our
partners. One of the big things its used for
> > > > > > > > > > is to develop SMMU and GIC
(our interrupt controller) code with PCI, so
> > > > > > > > > > dropping the SMMU from the
picture isn't an option.
> > > > > > > > > 
> > > > > > > > > Oh so this fixes a regression? 
This is something I didn't realize.
> > > > > > > > 
> > > > > > > > Yes, thanks. The regression came about
because we implemented SMMU-backed
> > > > > > > > DMA ops and only then was it apparent
that the virtio stuff was bypassing
> > > > > > > > even with translation enabled (because
it wasn't using the DMA API).
> > > > > > > 
> > > > > > > Could you point out a commit ID?
> > > > > > 
> > > > > > There has been a fair amount of work in this area
recently, but you're
> > > > > > probably after something like 876945dbf649
("arm64: Hook up IOMMU dma_ops")
> > > > > > as the culprit, which is the point at which we
started to swizzle DMA
> > > > > > ops for devices upstream of an SMMU automatically.
> > > > > > 
> > > > > > > > > A "Fixes:" tag can't
hurt here.  I then wonder
> > > > > > > > > might DMA ops ever use a DMA
address which isn't a physical address
> > > > > > > > > from QEMU point of view? If that
happens, this hack breaks
> > > > > > > > > because in legacy mode QEMU still
uses the GPA.
> > > > > > > > 
> > > > > > > > If QEMU doesn't advertise an SMMU,
then it will work fine with the GPA,
> > > > > > > > because we won't swizzle the DMA ops
for the master device. If QEMU does
> > > > > > > > advertise an SMMU, then we'll
allocate DMA addresses to fit within the
> > > > > > > > the intersection of the SMMU aperture
and device's DMA mask.
> > > > > > > 
> > > > > > > 
> > > > > > > Right but doesn't just poking from qemu
into phys addresses work
> > > > > > > anymore? It used to ...
> > > > > > 
> > > > > > Provided that there's no SMMU, then it will
continue to work. and my
> > > > > > understanding (from talking to Peter Maydell) is
that qemu doesn't model
> > > > > > an SMMU for ARM-based machines.
> > > > > > 
> > > > > 
> > > > > So how come people report failures due to presence of
SMMU?
> > > > > Using some other hypervisor?
> > > > 
> > > > The failures are reported on the ARM fastmodel (a complete
system
> > > > emulation that runs on an x86 box), where an SMMU *is*
present
> > > > downstream of the virtio-pci masters. There's no qemu
involved there.
> > > > 
> > > I see. And this hypervisor actually coded up looking up
> > > translations in the SMMU unconditionally for legacy devices,
> > > and this worked as long as guest didn't touch the SMMU?
> > 
> > Well, the fastmodel isn't a hypervisor really. It's a full
system emulation,
> > so it's better to think of it like a piece of hardware. For
example, you
> > could run KVM on the fastmodel. But yes, when Linux didn't swizzle
the
> > DMA ops to point at the SMMU, then everything defaults to bypass
(because
> > that's the default behaviour of the SMMU driver -- this is
configurable
> > on the command line) which is why things used to work.
> > 
> I would be a bit happier if Linux checked virtio iommu quirk and skipped
> the DMA ops thing then. It's a bit ugly but at least it's
consistently
> ugly.  To get clean emulation you would then use a modern device.
Sorry, but I'm not sure I follow your suggestion here. If you're talking
about the DMA ops themselves, they are assigned in arch_setup_dma_ops long
before the virtio layer gets involved, so we really can't figure out
whether the device has a virtio iommu quirk at that point.

I've reworked the patch (see below) so that we unconditionally set the
DMA ops for arm-based legacy devices, but I can't really tell what
you're
after here.

Will

--->8
>From 213bad7fdb8e4f45a7724be169cda292bbb50d2b Mon Sep 17 00:00:00 2001
From: Will Deacon <will.deacon at arm.com>
Date: Tue, 20 Dec 2016 12:12:49 +0000
Subject: [PATCH] vring: Force use of DMA API for ARM-based systems with legacy
 devices

Booting Linux on an ARM fastmodel containing an SMMU emulation results
in an unexpected I/O page fault from the legacy virtio-blk PCI device:

[    1.211721] arm-smmu-v3 2b400000.smmu: event 0x10 received:
[    1.211800] arm-smmu-v3 2b400000.smmu:	0x00000000fffff010
[    1.211880] arm-smmu-v3 2b400000.smmu:	0x0000020800000000
[    1.211959] arm-smmu-v3 2b400000.smmu:	0x00000008fa081002
[    1.212075] arm-smmu-v3 2b400000.smmu:	0x0000000000000000
[    1.212155] arm-smmu-v3 2b400000.smmu: event 0x10 received:
[    1.212234] arm-smmu-v3 2b400000.smmu:	0x00000000fffff010
[    1.212314] arm-smmu-v3 2b400000.smmu:	0x0000020800000000
[    1.212394] arm-smmu-v3 2b400000.smmu:	0x00000008fa081000
[    1.212471] arm-smmu-v3 2b400000.smmu:	0x0000000000000000

<system hangs failing to read partition table>

This is because the legacy virtio-blk device is behind an SMMU, so we
have consequently swizzled its DMA ops and configured the SMMU to
translate accesses. This then requires the vring code to use the DMA API
to establish translations, otherwise all transactions will result in
fatal faults and termination.

Given that ARM-based systems only see an SMMU if one is really present
(the topology is all described by firmware tables such as device-tree or
IORT), then we can safely use the DMA API for all legacy virtio devices.
Modern devices can advertise the prescense of an IOMMU using the
VIRTIO_F_IOMMU_PLATFORM feature flag.

Cc: Andy Lutomirski <luto at kernel.org>
Cc: Michael S. Tsirkin <mst at redhat.com>
Cc: <stable at vger.kernel.org>
Fixes: 876945dbf649 ("arm64: Hook up IOMMU dma_ops")
Signed-off-by: Will Deacon <will.deacon at arm.com>
---
 drivers/virtio/virtio_ring.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 409aeaa49246..7e38ed79c3fc 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -159,6 +159,13 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
 	if (xen_domain())
 		return true;
 
+	/*
+	 * On ARM-based machines, the DMA ops will do the right thing,
+	 * so always use them with legacy devices.
+	 */
+	if (IS_ENABLED(CONFIG_ARM) || IS_ENABLED(CONFIG_ARM64))
+		return !virtio_has_feature(vdev, VIRTIO_F_VERSION_1);
+
 	return false;
 }
 
-- 
2.1.4

On 20/01/17 10:33, Will Deacon wrote:
> On Thu, Jan 19, 2017 at 11:51:06PM +0200, Michael S. Tsirkin wrote:
>> On Mon, Jan 16, 2017 at 02:34:08PM +0000, Will Deacon wrote:
>>> On Mon, Jan 16, 2017 at 04:27:28PM +0200, Michael S. Tsirkin wrote:
>>>> On Mon, Jan 16, 2017 at 02:21:03PM +0000, Will Deacon wrote:
>>>>> On Mon, Jan 16, 2017 at 04:18:03PM +0200, Michael S.
Tsirkin wrote:
>>>>>> On Mon, Jan 16, 2017 at 10:40:28AM +0000, Will Deacon
wrote:
>>>>>>> On Fri, Jan 13, 2017 at 08:23:35PM +0200, Michael
S. Tsirkin wrote:
>>>>>>>> On Fri, Jan 13, 2017 at 05:21:54PM +0000, Will
Deacon wrote:
>>>>>>>>> On Fri, Jan 13, 2017 at 06:46:32PM +0200,
Michael S. Tsirkin wrote:
>>>>>>>>>> On Fri, Jan 13, 2017 at 09:25:22AM
+0000, Will Deacon wrote:
>>>>>>>>>>> On Fri, Jan 13, 2017 at 12:12:56AM
+0200, Michael S. Tsirkin wrote:
>>>>>>>>>>>> I'd rather people
didn't use SMMU with legacy devices.
>>>>>>>>>>>
>>>>>>>>>>> I'm afraid we've been doing
that for two years and the model already
>>>>>>>>>>> exists in a mature state, being
actively used for development and
>>>>>>>>>>> validation by ARM and our partners.
One of the big things its used for
>>>>>>>>>>> is to develop SMMU and GIC (our
interrupt controller) code with PCI, so
>>>>>>>>>>> dropping the SMMU from the picture
isn't an option.
>>>>>>>>>>
>>>>>>>>>> Oh so this fixes a regression?  This is
something I didn't realize.
>>>>>>>>>
>>>>>>>>> Yes, thanks. The regression came about
because we implemented SMMU-backed
>>>>>>>>> DMA ops and only then was it apparent that
the virtio stuff was bypassing
>>>>>>>>> even with translation enabled (because it
wasn't using the DMA API).
>>>>>>>>
>>>>>>>> Could you point out a commit ID?
>>>>>>>
>>>>>>> There has been a fair amount of work in this area
recently, but you're
>>>>>>> probably after something like 876945dbf649
("arm64: Hook up IOMMU dma_ops")
>>>>>>> as the culprit, which is the point at which we
started to swizzle DMA
>>>>>>> ops for devices upstream of an SMMU automatically.
>>>>>>>
>>>>>>>>>> A "Fixes:" tag can't hurt
here.  I then wonder
>>>>>>>>>> might DMA ops ever use a DMA address
which isn't a physical address
>>>>>>>>>> from QEMU point of view? If that
happens, this hack breaks
>>>>>>>>>> because in legacy mode QEMU still uses
the GPA.
>>>>>>>>>
>>>>>>>>> If QEMU doesn't advertise an SMMU, then
it will work fine with the GPA,
>>>>>>>>> because we won't swizzle the DMA ops
for the master device. If QEMU does
>>>>>>>>> advertise an SMMU, then we'll allocate
DMA addresses to fit within the
>>>>>>>>> the intersection of the SMMU aperture and
device's DMA mask.
>>>>>>>>
>>>>>>>>
>>>>>>>> Right but doesn't just poking from qemu
into phys addresses work
>>>>>>>> anymore? It used to ...
>>>>>>>
>>>>>>> Provided that there's no SMMU, then it will
continue to work. and my
>>>>>>> understanding (from talking to Peter Maydell) is
that qemu doesn't model
>>>>>>> an SMMU for ARM-based machines.
>>>>>>>
>>>>>>
>>>>>> So how come people report failures due to presence of
SMMU?
>>>>>> Using some other hypervisor?
>>>>>
>>>>> The failures are reported on the ARM fastmodel (a complete
system
>>>>> emulation that runs on an x86 box), where an SMMU *is*
present
>>>>> downstream of the virtio-pci masters. There's no qemu
involved there.
>>>>>
>>>> I see. And this hypervisor actually coded up looking up
>>>> translations in the SMMU unconditionally for legacy devices,
>>>> and this worked as long as guest didn't touch the SMMU?
>>>
>>> Well, the fastmodel isn't a hypervisor really. It's a full
system emulation,
>>> so it's better to think of it like a piece of hardware. For
example, you
>>> could run KVM on the fastmodel. But yes, when Linux didn't
swizzle the
>>> DMA ops to point at the SMMU, then everything defaults to bypass
(because
>>> that's the default behaviour of the SMMU driver -- this is
configurable
>>> on the command line) which is why things used to work.
>>>
>> I would be a bit happier if Linux checked virtio iommu quirk and
skipped
>> the DMA ops thing then. It's a bit ugly but at least it's
consistently
>> ugly.  To get clean emulation you would then use a modern device.
> 
> Sorry, but I'm not sure I follow your suggestion here. If you're
talking
> about the DMA ops themselves, they are assigned in arch_setup_dma_ops long
> before the virtio layer gets involved, so we really can't figure out
> whether the device has a virtio iommu quirk at that point.
> 
> I've reworked the patch (see below) so that we unconditionally set the
> DMA ops for arm-based legacy devices, but I can't really tell what
you're
> after here.
> 
> Will
> 
> --->8
> 
> From 213bad7fdb8e4f45a7724be169cda292bbb50d2b Mon Sep 17 00:00:00 2001
> From: Will Deacon <will.deacon at arm.com>
> Date: Tue, 20 Dec 2016 12:12:49 +0000
> Subject: [PATCH] vring: Force use of DMA API for ARM-based systems with
legacy
>  devices
> 
> Booting Linux on an ARM fastmodel containing an SMMU emulation results
> in an unexpected I/O page fault from the legacy virtio-blk PCI device:
> 
> [    1.211721] arm-smmu-v3 2b400000.smmu: event 0x10 received:
> [    1.211800] arm-smmu-v3 2b400000.smmu:	0x00000000fffff010
> [    1.211880] arm-smmu-v3 2b400000.smmu:	0x0000020800000000
> [    1.211959] arm-smmu-v3 2b400000.smmu:	0x00000008fa081002
> [    1.212075] arm-smmu-v3 2b400000.smmu:	0x0000000000000000
> [    1.212155] arm-smmu-v3 2b400000.smmu: event 0x10 received:
> [    1.212234] arm-smmu-v3 2b400000.smmu:	0x00000000fffff010
> [    1.212314] arm-smmu-v3 2b400000.smmu:	0x0000020800000000
> [    1.212394] arm-smmu-v3 2b400000.smmu:	0x00000008fa081000
> [    1.212471] arm-smmu-v3 2b400000.smmu:	0x0000000000000000
> 
> <system hangs failing to read partition table>
> 
> This is because the legacy virtio-blk device is behind an SMMU, so we
> have consequently swizzled its DMA ops and configured the SMMU to
> translate accesses. This then requires the vring code to use the DMA API
> to establish translations, otherwise all transactions will result in
> fatal faults and termination.
> 
> Given that ARM-based systems only see an SMMU if one is really present
> (the topology is all described by firmware tables such as device-tree or
> IORT), then we can safely use the DMA API for all legacy virtio devices.
> Modern devices can advertise the prescense of an IOMMU using the
> VIRTIO_F_IOMMU_PLATFORM feature flag.
> 
> Cc: Andy Lutomirski <luto at kernel.org>
> Cc: Michael S. Tsirkin <mst at redhat.com>
> Cc: <stable at vger.kernel.org>
> Fixes: 876945dbf649 ("arm64: Hook up IOMMU dma_ops")
> Signed-off-by: Will Deacon <will.deacon at arm.com>
> ---
>  drivers/virtio/virtio_ring.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 409aeaa49246..7e38ed79c3fc 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -159,6 +159,13 @@ static bool vring_use_dma_api(struct virtio_device
*vdev)
>  	if (xen_domain())
>  		return true;
>  
> +	/*
> +	 * On ARM-based machines, the DMA ops will do the right thing,
> +	 * so always use them with legacy devices.
> +	 */
> +	if (IS_ENABLED(CONFIG_ARM) || IS_ENABLED(CONFIG_ARM64))
> +		return !virtio_has_feature(vdev, VIRTIO_F_VERSION_1);
> +
>  	return false;
>  }
>  
> 
Acked-by: Marc Zyngier <marc.zyngier at arm.com>

Any chance this fix (or anything with similar effects) gets applied
sometime soon? I cannot use the model without using a similar workaround:

http://git.kernel.org/cgit/linux/kernel/git/maz/arm-platforms.git/commit/?h=kvm-arm64/gicv4-wip&id=622ff1190890c0ae60d57e76a7c2f3e6fb27e25d

and I suspect that other users of the same system are carrying their own
version of the fix. Something in mainline would be infinitely better.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...