On Wed, Oct 19, 2022 at 8:27 PM Eric Auger <eric.au...@redhat.com> wrote: > > Since b68ba1ca5767 ("memory: Add IOMMU_NOTIFIER_DEVIOTLB_UNMAP > IOMMUTLBNotificationType"), vhost attempts to register DEVIOTLB_UNMAP > notifier. This latter is supported by the intel-iommu which supports > device-iotlb if the corresponding option is set. Then 958ec334bca3 > ("vhost: Unbreak SMMU and virtio-iommu on dev-iotlb support") allowed > silent fallback to the legacy UNMAP notifier if the viommu does not > support device iotlb. > > Initially vhost/viommu integration was introduced with intel iommu > assuming ats=on was set on virtio-pci device and device-iotlb was set > on the intel iommu. vhost acts as an ATS capable device since it > implements an IOTLB on kernel side. However translated transactions > that hit the device IOTLB do not transit through the vIOMMU. So this > requires a limited ATS support on viommu side. Anyway this assumed > ATS was eventually enabled . > > But neither SMMUv3 nor virtio-iommu do support ATS and the integration > with vhost just relies on the fact those vIOMMU send UNMAP notifications > whenever the guest trigger them. This works without ATS being enabled. > > This patch makes sure we get a warning if ATS is set on a device > protected by virtio-iommu or vsmmuv3, reminding that we don't have > full support of ATS on those vIOMMUs and setting ats=on on the > virtio-pci end-point is not a requirement. > > Signed-off-by: Eric Auger <eric.au...@redhat.com> > > --- > > v1 -> v2: > - s/enabled/capable > - tweak the error message on vhost side > --- > include/hw/virtio/virtio-bus.h | 3 +++ > hw/virtio/vhost.c | 21 ++++++++++++++++++++- > hw/virtio/virtio-bus.c | 14 ++++++++++++++ > hw/virtio/virtio-pci.c | 11 +++++++++++ > 4 files changed, 48 insertions(+), 1 deletion(-) > > diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h > index 7ab8c9dab0..23360a1daa 100644 > --- a/include/hw/virtio/virtio-bus.h > +++ b/include/hw/virtio/virtio-bus.h > @@ -94,6 +94,7 @@ struct VirtioBusClass { > bool has_variable_vring_alignment; > AddressSpace *(*get_dma_as)(DeviceState *d); > bool (*iommu_enabled)(DeviceState *d); > + bool (*ats_capable)(DeviceState *d); > }; > > struct VirtioBusState { > @@ -157,4 +158,6 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int > n, bool assign); > void virtio_bus_cleanup_host_notifier(VirtioBusState *bus, int n); > /* Whether the IOMMU is enabled for this device */ > bool virtio_bus_device_iommu_enabled(VirtIODevice *vdev); > +/* Whether ATS is enabled for this device */ > +bool virtio_bus_device_ats_capable(VirtIODevice *vdev); > #endif /* VIRTIO_BUS_H */ > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index 5185c15295..3cf9efce5e 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -324,6 +324,16 @@ static bool vhost_dev_has_iommu(struct vhost_dev *dev) > } > } > > +static bool vhost_dev_ats_capable(struct vhost_dev *dev)
I suggest to rename this as pf_capable() since ATS is PCI specific but vhost isn't. > +{ > + VirtIODevice *vdev = dev->vdev; > + > + if (vdev && virtio_bus_device_ats_capable(vdev)) { > + return true; > + } > + return false; > +} > + > static void *vhost_memory_map(struct vhost_dev *dev, hwaddr addr, > hwaddr *plen, bool is_write) > { > @@ -737,6 +747,7 @@ static void vhost_iommu_region_add(MemoryListener > *listener, > Int128 end; > int iommu_idx; > IOMMUMemoryRegion *iommu_mr; > + Error *err = NULL; > int ret; > > if (!memory_region_is_iommu(section->mr)) { > @@ -760,8 +771,16 @@ static void vhost_iommu_region_add(MemoryListener > *listener, > iommu->iommu_offset = section->offset_within_address_space - > section->offset_within_region; > iommu->hdev = dev; > - ret = memory_region_register_iommu_notifier(section->mr, &iommu->n, > NULL); > + ret = memory_region_register_iommu_notifier(section->mr, &iommu->n, > &err); > if (ret) { > + if (vhost_dev_ats_capable(dev)) { > + error_reportf_err(err, > + "%s: Although the device exposes ATS > capability, " > + "fallback to legacy IOMMU UNMAP notifier: ", > + iommu_mr->parent_obj.name); I'm not sure if it's a real error, or I wonder what we need to do is 1) check is ATS is enabled 2) fallback to UNMAP is ATS is not enabled > + } else { > + error_free(err); > + } > /* > * Some vIOMMUs do not support dev-iotlb yet. If so, try to use the > * UNMAP legacy message > diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c > index 896feb37a1..d46c3f8ec4 100644 > --- a/hw/virtio/virtio-bus.c > +++ b/hw/virtio/virtio-bus.c > @@ -348,6 +348,20 @@ bool virtio_bus_device_iommu_enabled(VirtIODevice *vdev) > return klass->iommu_enabled(qbus->parent); > } > > +bool virtio_bus_device_ats_capable(VirtIODevice *vdev) > +{ > + DeviceState *qdev = DEVICE(vdev); > + BusState *qbus = BUS(qdev_get_parent_bus(qdev)); > + VirtioBusState *bus = VIRTIO_BUS(qbus); > + VirtioBusClass *klass = VIRTIO_BUS_GET_CLASS(bus); > + > + if (!klass->ats_capable) { > + return false; I think it's better to check whether or not ATS is enabled. Guest may choose to not enable ATS for various reasons. Thanks > + } > + > + return klass->ats_capable(qbus->parent); > +} > + > static void virtio_bus_class_init(ObjectClass *klass, void *data) > { > BusClass *bus_class = BUS_CLASS(klass); > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index e7d80242b7..c2ceba98a6 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -1141,6 +1141,16 @@ static bool virtio_pci_iommu_enabled(DeviceState *d) > return true; > } > > +static bool virtio_pci_ats_capable(DeviceState *d) > +{ > + VirtIOPCIProxy *proxy = VIRTIO_PCI(d); > + > + if (proxy->flags & VIRTIO_PCI_FLAG_ATS) { > + return true; > + } > + return false; > +} > + > static bool virtio_pci_queue_enabled(DeviceState *d, int n) > { > VirtIOPCIProxy *proxy = VIRTIO_PCI(d); > @@ -2229,6 +2239,7 @@ static void virtio_pci_bus_class_init(ObjectClass > *klass, void *data) > k->ioeventfd_assign = virtio_pci_ioeventfd_assign; > k->get_dma_as = virtio_pci_get_dma_as; > k->iommu_enabled = virtio_pci_iommu_enabled; > + k->ats_capable = virtio_pci_ats_capable; > k->queue_enabled = virtio_pci_queue_enabled; > } > > -- > 2.35.3 >