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

[Eric Auger]

Hi Jason,

On 10/20/22 05:58, Jason Wang wrote:
> 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.
Does pf sound for page fault?
>
>> +{
>> +    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
Isn't the problem due to the fact that vhost, by construction, requires
"pf" (would it be though DEVIOTLB_UNMAP or UNMAP). Don't UNMAP
notifications also implement part of ATS protocol? I guess this is the
reason why you mandated ats in the first place.

So if ATS is not enabled, shouldn't we fallback to virtio instead of vhost?

Thanks

Eric
>
>> +        } 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
>>