Re: [PATCH] iommu/virtio: Advertise IOMMU_CAP_CACHE_COHERENCY
On Thu, Jul 14, 2022 at 02:39:32PM +0100, Robin Murphy wrote: > > In the meantime we do need to restore VFIO support under virtio-iommu, > > since userspace still expects that to work, and the existing use-cases are > > coherent devices. > > Yeah, I'm not necessarily against adding this as a horrible bodge for now - > the reality is that people using VFIO must be doing it on coherent systems > or it wouldn't be working properly anyway - as long as we all agree that > that's what it is. > > Next cycle I'll be sending the follow-up patches to bring > device_iommu_capable() to its final form (hoping the outstanding VDPA patch > lands in the meantime), at which point we get to sort-of-fix the SMMU > drivers[1], and can do something similar here too. I guess the main question > for virtio-iommu is whether it needs to be described/negotiated in the > protocol itself, or can be reliably described by other standard firmware > properties (with maybe just a spec not to clarify that coherency must be > consistent). What consumers of IOMMU_CAP_CACHE_COHERENCY now want to know, is whether coherency is managed in HW for one particular endpoint, or if they need to issue cache maintenance. The latter cannot be handled by VFIO since cache maintenance is generally privileged. So I had to list several possibilities regarding non-coherent accesses. I don't think we need a spec change. A. Accesses through physical IOMMU are never coherent - In this case, translated accesses from the physical device can't access memory coherently. The host would describe it using existing FW methods (dma-coherent in DT, _CCA in ACPI) since it's also needed without a vIOMMU. No change needed for virtio-iommu, I think, it can support non-coherent devices. It can also support mixing coherent and non-coherent devices in the same domain, because domains just allow to multiplex map requests at the moment. Since we allow sending the same map request onto two different domains, one with coherent devices and one with non-coherent ones, then we can also allow using a single domain for that. If the host cannot handle this, it is allowed to reject attach requests for incompatible devices. In Linux I think compatible() should include dev->dma_coherent after your change, or the callers should check dev->dma_coherent themselves (vfio-platform in particular) B. Non-cacheable mappings - Here, accesses are normally coherent but the pIOMMU mappings may be configured to be non-coherent (non-cacheable access type on Arm). If there is an actual need for this, we could add a feature bit, a probe request property and a map flag. In Linux we may want to disallow !IOMMU_CACHE if the device is coherent, since we don't support this case. C. PCIe devices performing no-snoop accesses Accesses are normally coherent but the device may set a transaction bit requesting the transaction to be non-coherent. A guest can't enable and use no-snoop in a PCIe device without knowing whether the system supports it. It's not discoverable on Arm, so a guest can't use it. On x86 I think it's always supported but the pIOMMU may enforce snoop (and the guest may be unable to perform cache maintenance? I didn't follow the whole wbinvd discussions for lack of time). The problem is the same without a vIOMMU, so I'd defer that to some firmware method describing no-snoop. D. Non-coherent virtio-iommu Non-coherent virtqueues. It's not forbidden by the spec, and a transport driver could support it, but it's a transport problem and virtio-iommu doesn't need to know about it. Did I forget anything? Otherwise I don't think we need any spec change at the moment. But when adding support for page tables, we'll have to consider each of these cases since the guest will be able to set memory attributes and will care about page walks coherency. That will be bundled in a probe request along with the other page table information. Thanks, Jean ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] iommu/virtio: Advertise IOMMU_CAP_CACHE_COHERENCY
On 2022-07-14 14:00, Jean-Philippe Brucker wrote: On Thu, Jul 14, 2022 at 01:01:37PM +0100, Robin Murphy wrote: On 2022-07-14 12:11, Jean-Philippe Brucker wrote: Fix virtio-iommu interaction with VFIO, as VFIO now requires IOMMU_CAP_CACHE_COHERENCY. virtio-iommu does not support non-cacheable mappings, and always expects to be called with IOMMU_CACHE. Can we know this is actually true though? What if the virtio-iommu implementation is backed by something other than VFIO, and the underlying hardware isn't coherent? AFAICS the spec doesn't disallow that. Right, I should add a note about that. If someone does actually want to support non-coherent device, I assume we'll add a per-device property, a 'non-cacheable' mapping flag, and IOMMU_CAP_CACHE_COHERENCY will hold. I'm also planning to add a check on (IOMMU_CACHE && !IOMMU_NOEXEC) in viommu_map(), but not as a fix. But what about all the I/O-coherent PL330s? :P (IIRC you can actually make a Juno do that with S2CR.MTCFG hacks...) In the meantime we do need to restore VFIO support under virtio-iommu, since userspace still expects that to work, and the existing use-cases are coherent devices. Yeah, I'm not necessarily against adding this as a horrible bodge for now - the reality is that people using VFIO must be doing it on coherent systems or it wouldn't be working properly anyway - as long as we all agree that that's what it is. Next cycle I'll be sending the follow-up patches to bring device_iommu_capable() to its final form (hoping the outstanding VDPA patch lands in the meantime), at which point we get to sort-of-fix the SMMU drivers[1], and can do something similar here too. I guess the main question for virtio-iommu is whether it needs to be described/negotiated in the protocol itself, or can be reliably described by other standard firmware properties (with maybe just a spec not to clarify that coherency must be consistent). Cheers, Robin. [1] https://gitlab.arm.com/linux-arm/linux-rm/-/commit/d8256bf48c8606cbaa6f0815696c2a6dbb72f1b0 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] iommu/virtio: Advertise IOMMU_CAP_CACHE_COHERENCY
On Thu, Jul 14, 2022 at 01:01:37PM +0100, Robin Murphy wrote:
> On 2022-07-14 12:11, Jean-Philippe Brucker wrote:
> > Fix virtio-iommu interaction with VFIO, as VFIO now requires
> > IOMMU_CAP_CACHE_COHERENCY. virtio-iommu does not support non-cacheable
> > mappings, and always expects to be called with IOMMU_CACHE.
>
> Can we know this is actually true though? What if the virtio-iommu
> implementation is backed by something other than VFIO, and the underlying
> hardware isn't coherent? AFAICS the spec doesn't disallow that.
Right, I should add a note about that. If someone does actually want to
support non-coherent device, I assume we'll add a per-device property, a
'non-cacheable' mapping flag, and IOMMU_CAP_CACHE_COHERENCY will hold.
I'm also planning to add a check on (IOMMU_CACHE && !IOMMU_NOEXEC) in
viommu_map(), but not as a fix.
In the meantime we do need to restore VFIO support under virtio-iommu,
since userspace still expects that to work, and the existing use-cases are
coherent devices.
Thanks,
Jean
>
> Thanks,
> Robin.
>
> > Fixes: e8ae0e140c05 ("vfio: Require that devices support DMA cache
> > coherence")
> > Signed-off-by: Jean-Philippe Brucker
> > ---
> > 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 25be4b822aa0..bf340d779c10 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] iommu/virtio: Advertise IOMMU_CAP_CACHE_COHERENCY
On 2022-07-14 12:11, Jean-Philippe Brucker wrote:
Fix virtio-iommu interaction with VFIO, as VFIO now requires
IOMMU_CAP_CACHE_COHERENCY. virtio-iommu does not support non-cacheable
mappings, and always expects to be called with IOMMU_CACHE.
Can we know this is actually true though? What if the virtio-iommu
implementation is backed by something other than VFIO, and the
underlying hardware isn't coherent? AFAICS the spec doesn't disallow that.
Thanks,
Robin.
Fixes: e8ae0e140c05 ("vfio: Require that devices support DMA cache coherence")
Signed-off-by: Jean-Philippe Brucker
---
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 25be4b822aa0..bf340d779c10 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
[PATCH] iommu/virtio: Advertise IOMMU_CAP_CACHE_COHERENCY
Fix virtio-iommu interaction with VFIO, as VFIO now requires
IOMMU_CAP_CACHE_COHERENCY. virtio-iommu does not support non-cacheable
mappings, and always expects to be called with IOMMU_CACHE.
Fixes: e8ae0e140c05 ("vfio: Require that devices support DMA cache coherence")
Signed-off-by: Jean-Philippe Brucker
---
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 25be4b822aa0..bf340d779c10 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.36.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
