[PATCH v2] iommu/virtio: Fix interaction with VFIO

[PATCH v2] iommu/virtio: Fix interaction with VFIO

[Jean-Philippe Brucker]

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, 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.

Fixes: e8ae0e140c05 ("vfio: Require that devices support DMA cache coherence")
Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---

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/20220714111059.708735-1-jean-philippe@linaro.org/
[2] https://lore.kernel.org/linux-iommu/d8bd8777d06929ad8f49df7fc80e1b9af32a41b5.1660574547.git.robin.murphy@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,
-- 
2.37.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] iommu/virtio: Fix interaction with VFIO

[Robin Murphy]

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. 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).

> 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 ;)

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 <jean-philippe@linaro.org>
> ---
> 
> 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/20220714111059.708735-1-jean-philippe@linaro.org/
> [2] https://lore.kernel.org/linux-iommu/d8bd8777d06929ad8f49df7fc80e1b9af32a41b5.1660574547.git.robin.murphy@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
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] iommu/virtio: Fix interaction with VFIO

[Jean-Philippe Brucker]

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 <jean-philippe@linaro.org>
> > ---
> > 
> > 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/20220714111059.708735-1-jean-philippe@linaro.org/
> > [2] https://lore.kernel.org/linux-iommu/d8bd8777d06929ad8f49df7fc80e1b9af32a41b5.1660574547.git.robin.murphy@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
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] iommu/virtio: Fix interaction with VFIO

[Robin Murphy]

On 2022-08-19 11:38, Jean-Philippe Brucker wrote:
> 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'.

vfio-amba with the pl330 on Juno, perhaps?

>> 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?

It should do, since the downstream DT says the SMMU is non-coherent.

>>> 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.

I think that's a good idea, but IIRC Jason mentioned in review of the 
VFIO series that it's not sufficient to provide the actual guarantee 
we're after, since there are out-of-spec devices that ignore the control 
and may send no-snoop packets anyway. However, as part of a best-effort 
approach for arm64 it still makes sense to help all the well-behaved 
drivers/devices do the right thing.

Cheers,
Robin.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization