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

Tue Jan 24 08:06:45 PST 2017

On Tue, Jan 24, 2017 at 04:04:11PM +0000, Marc Zyngier wrote:
> 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.

I'll merge this in the next pull.

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