On Thu, Aug 18, 2022 at 09:10:25PM +0100, Robin Murphy wrote:
> On 2022-08-18 17:38, Jean-Philippe Brucker wrote:
> > Commit e8ae0e140c05 ("vfio: Require that devices support DMA cache
> > coherence") requires IOMMU drivers to advertise
> > IOMMU_CAP_CACHE_COHERENCY, in order to be used by VFIO. Since VFIO does
> > not provide to userspace the ability to maintain coherency through cache
> > invalidations, it requires hardware coherency. Advertise the capability
> > in order to restore VFIO support.
> >
> > The meaning of IOMMU_CAP_CACHE_COHERENCY also changed from "IOMMU can
> > enforce cache coherent DMA transactions" to "IOMMU_CACHE is supported".
> > While virtio-iommu cannot enforce coherency (of PCIe no-snoop
> > transactions), it does support IOMMU_CACHE.
> >
> > Non-coherent accesses are not currently a concern for virtio-iommu
> > because host OSes only assign coherent devices,
>
> Is that guaranteed though? I see nothing in VFIO checking *device*
> coherency, only that the *IOMMU* can impose it via this capability, which
> would form a very circular argument.
Yes the wording is wrong here, more like "host OSes only assign devices
whose accesses are coherent". And it's not guaranteed, just I'm still
looking for a realistic counter-example. I guess a good indicator would be
a VMM that presents a device without 'dma-coherent'.
> We can no longer say that in practice
> nobody has a VFIO-capable IOMMU in front of non-coherent PCI, now that
> Rockchip RK3588 boards are about to start shipping (at best we can only say
> that they still won't have the SMMUs in the DT until I've finished ripping
> up the bus ops).
Ah, I was hoping that vfio-pci should only be concerned about no-snoop. Do
you know if your series [2] ensures that the SMMU driver doesn't report
IOMMU_CAP_CACHE_COHERENCY for that system?
>
> > and the guest does not
> > enable PCIe no-snoop. Nevertheless, we can summarize here the possible
> > support for non-coherent DMA:
> >
> > (1) When accesses from a hardware endpoint are not coherent. The host
> > would describe such a device using firmware methods ('dma-coherent'
> > in device-tree, '_CCA' in ACPI), since they are also needed without
> > a vIOMMU. In this case mappings are created without IOMMU_CACHE.
> > virtio-iommu doesn't need any additional support. It sends the same
> > requests as for coherent devices.
> >
> > (2) When the physical IOMMU supports non-cacheable mappings. Supporting
> > those would require a new feature in virtio-iommu, new PROBE request
> > property and MAP flags. Device drivers would use a new API to
> > discover this since it depends on the architecture and the physical
> > IOMMU.
> >
> > (3) When the hardware supports PCIe no-snoop. Some architecture do not
> > support this either (whether no-snoop is supported by an Arm system
> > is not discoverable by software). If Linux did enable no-snoop in
> > endpoints on x86, then virtio-iommu would need additional feature,
> > PROBE property, ATTACH and/or MAP flags to support enforcing snoop.
>
> That's not an "if" - various Linux drivers *do* use no-snoop, which IIUC is
> the main reason for VFIO wanting to enforce this in the first place. For
> example, see the big fat comment in drm_arch_can_wc_memory() if you've
> forgotten the fun we had with AMD GPUs in the TX2 boxes back in the day ;)
Ah duh, I missed that PCI_EXP_DEVCTL_NOSNOOP_EN defaults to 1, of course
it does. So I think VFIO should clear it on Arm and make it read-only,
since the SMMU can't force-snoop like on x86. I'd be tempted to do that if
CONFIG_ARM{,64} is enabled, but checking a new IOMMU capability may be
cleaner.
Thanks,
Jean
>
> This is what I was getting at in reply to v1, it's really not a "this is
> fine as things stand" kind of patch, it's a "this is the best we can do to
> be less wrong for expected usage, but still definitely not right".
> Admittedly I downplayed that a little in [2] by deliberately avoiding all
> mention of no-snoop, but only because that's such a horrific unsolvable mess
> it's hardly worth the pain of bringing up...
>
> Cheers,
> Robin.
>
> > Fixes: e8ae0e140c05 ("vfio: Require that devices support DMA cache
> > coherence")
> > Signed-off-by: Jean-Philippe Brucker <[email protected]>
> > ---
> >
> > Since v1 [1], I added some details to the commit message. This fix is
> > still needed for v5.19 and v6.0.
> >
> > I can improve the check once Robin's change [2] is merged:
> > capable(IOMMU_CAP_CACHE_COHERENCY) could return dev->dma_coherent for
> > case (1) above.
> >
> > [1]
> > https://lore.kernel.org/linux-iommu/[email protected]/
> > [2]
> > https://lore.kernel.org/linux-iommu/d8bd8777d06929ad8f49df7fc80e1b9af32a41b5.1660574547.git.robin.mur...@arm.com/
> > ---
> > drivers/iommu/virtio-iommu.c | 11 +++++++++++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> > index 08eeafc9529f..80151176ba12 100644
> > --- a/drivers/iommu/virtio-iommu.c
> > +++ b/drivers/iommu/virtio-iommu.c
> > @@ -1006,7 +1006,18 @@ static int viommu_of_xlate(struct device *dev,
> > struct of_phandle_args *args)
> > return iommu_fwspec_add_ids(dev, args->args, 1);
> > }
> > +static bool viommu_capable(enum iommu_cap cap)
> > +{
> > + switch (cap) {
> > + case IOMMU_CAP_CACHE_COHERENCY:
> > + return true;
> > + default:
> > + return false;
> > + }
> > +}
> > +
> > static struct iommu_ops viommu_ops = {
> > + .capable = viommu_capable,
> > .domain_alloc = viommu_domain_alloc,
> > .probe_device = viommu_probe_device,
> > .probe_finalize = viommu_probe_finalize,
_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
