Re: [PATCH v2] vhost: Warn if DEVIOTLB_UNMAP is not supported and ats is set

[Jason Wang <jasowang@redhat.com>]

On Wed, Oct 19, 2022 at 8:27 PM Eric Auger <eric.auger@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.auger@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
>