Re: [PATCH v2 2/4] virtio-iommu: Default to bypass during boot

[Eric Auger <eric.auger@redhat.com>]

On 1/27/22 3:29 PM, Jean-Philippe Brucker wrote:
> Currently the virtio-iommu device must be programmed before it allows
> DMA from any PCI device. This can make the VM entirely unusable when a
> virtio-iommu driver isn't present, for example in a bootloader that
> loads the OS from storage.
>
> Similarly to the other vIOMMU implementations, default to DMA bypassing
> the IOMMU during boot. Add a "boot-bypass" property, defaulting to true,
> that lets users change this behavior.
>
> Replace the VIRTIO_IOMMU_F_BYPASS feature, which didn't support bypass
> before feature negotiation, with VIRTIO_IOMMU_F_BYPASS_CONFIG.
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric
> ---
>  include/hw/virtio/virtio-iommu.h |  1 +
>  hw/virtio/virtio-iommu.c         | 51 +++++++++++++++++++++++++++++---
>  hw/virtio/trace-events           |  4 ++-
>  3 files changed, 51 insertions(+), 5 deletions(-)
>
> diff --git a/include/hw/virtio/virtio-iommu.h b/include/hw/virtio/virtio-iommu.h
> index e2339e5b72..84391f8448 100644
> --- a/include/hw/virtio/virtio-iommu.h
> +++ b/include/hw/virtio/virtio-iommu.h
> @@ -58,6 +58,7 @@ struct VirtIOIOMMU {
>      GTree *domains;
>      QemuMutex mutex;
>      GTree *endpoints;
> +    bool boot_bypass;
>  };
>  
>  #endif
> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
> index aa9c16a17b..ec02029bb6 100644
> --- a/hw/virtio/virtio-iommu.c
> +++ b/hw/virtio/virtio-iommu.c
> @@ -24,6 +24,7 @@
>  #include "hw/qdev-properties.h"
>  #include "hw/virtio/virtio.h"
>  #include "sysemu/kvm.h"
> +#include "sysemu/reset.h"
>  #include "qapi/error.h"
>  #include "qemu/error-report.h"
>  #include "trace.h"
> @@ -728,8 +729,7 @@ static IOMMUTLBEntry virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>          .perm = IOMMU_NONE,
>      };
>  
> -    bypass_allowed = virtio_vdev_has_feature(&s->parent_obj,
> -                                             VIRTIO_IOMMU_F_BYPASS);
> +    bypass_allowed = s->config.bypass;
>  
>      sid = virtio_iommu_get_bdf(sdev);
>  
> @@ -831,13 +831,37 @@ static void virtio_iommu_get_config(VirtIODevice *vdev, uint8_t *config_data)
>      out_config->domain_range.start = cpu_to_le32(dev_config->domain_range.start);
>      out_config->domain_range.end = cpu_to_le32(dev_config->domain_range.end);
>      out_config->probe_size = cpu_to_le32(dev_config->probe_size);
> +    out_config->bypass = dev_config->bypass;
>  
>      trace_virtio_iommu_get_config(dev_config->page_size_mask,
>                                    dev_config->input_range.start,
>                                    dev_config->input_range.end,
>                                    dev_config->domain_range.start,
>                                    dev_config->domain_range.end,
> -                                  dev_config->probe_size);
> +                                  dev_config->probe_size,
> +                                  dev_config->bypass);
> +}
> +
> +static void virtio_iommu_set_config(VirtIODevice *vdev,
> +                                    const uint8_t *config_data)
> +{
> +    VirtIOIOMMU *dev = VIRTIO_IOMMU(vdev);
> +    struct virtio_iommu_config *dev_config = &dev->config;
> +    const struct virtio_iommu_config *in_config = (void *)config_data;
> +
> +    if (in_config->bypass != dev_config->bypass) {
> +        if (!virtio_vdev_has_feature(vdev, VIRTIO_IOMMU_F_BYPASS_CONFIG)) {
> +            virtio_error(vdev, "cannot set config.bypass");
> +            return;
> +        } else if (in_config->bypass != 0 && in_config->bypass != 1) {
> +            virtio_error(vdev, "invalid config.bypass value '%u'",
> +                         in_config->bypass);
> +            return;
> +        }
> +        dev_config->bypass = in_config->bypass;
> +    }
> +
> +    trace_virtio_iommu_set_config(in_config->bypass);
>  }
>  
>  static uint64_t virtio_iommu_get_features(VirtIODevice *vdev, uint64_t f,
> @@ -963,6 +987,19 @@ static int virtio_iommu_set_page_size_mask(IOMMUMemoryRegion *mr,
>      return 0;
>  }
>  
> +static void virtio_iommu_system_reset(void *opaque)
> +{
> +    VirtIOIOMMU *s = opaque;
> +
> +    trace_virtio_iommu_system_reset();
> +
> +    /*
> +     * config.bypass is sticky across device reset, but should be restored on
> +     * system reset
> +     */
> +    s->config.bypass = s->boot_bypass;
> +}
> +
>  static void virtio_iommu_device_realize(DeviceState *dev, Error **errp)
>  {
>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> @@ -988,9 +1025,9 @@ static void virtio_iommu_device_realize(DeviceState *dev, Error **errp)
>      virtio_add_feature(&s->features, VIRTIO_IOMMU_F_INPUT_RANGE);
>      virtio_add_feature(&s->features, VIRTIO_IOMMU_F_DOMAIN_RANGE);
>      virtio_add_feature(&s->features, VIRTIO_IOMMU_F_MAP_UNMAP);
> -    virtio_add_feature(&s->features, VIRTIO_IOMMU_F_BYPASS);
>      virtio_add_feature(&s->features, VIRTIO_IOMMU_F_MMIO);
>      virtio_add_feature(&s->features, VIRTIO_IOMMU_F_PROBE);
> +    virtio_add_feature(&s->features, VIRTIO_IOMMU_F_BYPASS_CONFIG);
>  
>      qemu_mutex_init(&s->mutex);
>  
> @@ -1001,6 +1038,8 @@ static void virtio_iommu_device_realize(DeviceState *dev, Error **errp)
>      } else {
>          error_setg(errp, "VIRTIO-IOMMU is not attached to any PCI bus!");
>      }
> +
> +    qemu_register_reset(virtio_iommu_system_reset, s);
>  }
>  
>  static void virtio_iommu_device_unrealize(DeviceState *dev)
> @@ -1008,6 +1047,8 @@ static void virtio_iommu_device_unrealize(DeviceState *dev)
>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>      VirtIOIOMMU *s = VIRTIO_IOMMU(dev);
>  
> +    qemu_unregister_reset(virtio_iommu_system_reset, s);
> +
>      g_hash_table_destroy(s->as_by_busptr);
>      if (s->domains) {
>          g_tree_destroy(s->domains);
> @@ -1164,6 +1205,7 @@ static const VMStateDescription vmstate_virtio_iommu = {
>  
>  static Property virtio_iommu_properties[] = {
>      DEFINE_PROP_LINK("primary-bus", VirtIOIOMMU, primary_bus, "PCI", PCIBus *),
> +    DEFINE_PROP_BOOL("boot-bypass", VirtIOIOMMU, boot_bypass, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -1180,6 +1222,7 @@ static void virtio_iommu_class_init(ObjectClass *klass, void *data)
>      vdc->unrealize = virtio_iommu_device_unrealize;
>      vdc->reset = virtio_iommu_device_reset;
>      vdc->get_config = virtio_iommu_get_config;
> +    vdc->set_config = virtio_iommu_set_config;
>      vdc->get_features = virtio_iommu_get_features;
>      vdc->set_status = virtio_iommu_set_status;
>      vdc->vmsd = &vmstate_virtio_iommu_device;
> diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
> index f7ad6be5fb..a5102eac9e 100644
> --- a/hw/virtio/trace-events
> +++ b/hw/virtio/trace-events
> @@ -89,9 +89,11 @@ virtio_mmio_setting_irq(int level) "virtio_mmio setting IRQ %d"
>  
>  # virtio-iommu.c
>  virtio_iommu_device_reset(void) "reset!"
> +virtio_iommu_system_reset(void) "system reset!"
>  virtio_iommu_get_features(uint64_t features) "device supports features=0x%"PRIx64
>  virtio_iommu_device_status(uint8_t status) "driver status = %d"
> -virtio_iommu_get_config(uint64_t page_size_mask, uint64_t start, uint64_t end, uint32_t domain_start, uint32_t domain_end, uint32_t probe_size) "page_size_mask=0x%"PRIx64" input range start=0x%"PRIx64" input range end=0x%"PRIx64" domain range start=%d domain range end=%d probe_size=0x%x"
> +virtio_iommu_get_config(uint64_t page_size_mask, uint64_t start, uint64_t end, uint32_t domain_start, uint32_t domain_end, uint32_t probe_size, uint8_t bypass) "page_size_mask=0x%"PRIx64" input range start=0x%"PRIx64" input range end=0x%"PRIx64" domain range start=%d domain range end=%d probe_size=0x%x bypass=0x%x"
> +virtio_iommu_set_config(uint8_t bypass) "bypass=0x%x"
>  virtio_iommu_attach(uint32_t domain_id, uint32_t ep_id) "domain=%d endpoint=%d"
>  virtio_iommu_detach(uint32_t domain_id, uint32_t ep_id) "domain=%d endpoint=%d"
>  virtio_iommu_map(uint32_t domain_id, uint64_t virt_start, uint64_t virt_end, uint64_t phys_start, uint32_t flags) "domain=%d virt_start=0x%"PRIx64" virt_end=0x%"PRIx64 " phys_start=0x%"PRIx64" flags=%d"