[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2] vhost: Warn if DEVIOTLB_UNMAP is not supported and ats is
From: |
Jason Wang |
Subject: |
Re: [PATCH v2] vhost: Warn if DEVIOTLB_UNMAP is not supported and ats is set |
Date: |
Thu, 20 Oct 2022 11:58:32 +0800 |
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
>