Hi Jason,
On 10/20/22 05:58, Jason Wang wrote:
> 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.
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
>>
