diff mbox series

[2/3] virtio-iommu: Default to bypass during boot

Message ID 20210930185050.262759-3-jean-philippe@linaro.org
State New
Headers show
Series virtio-iommu: Support VIRTIO_IOMMU_F_BYPASS_CONFIG | expand

Commit Message

Jean-Philippe Brucker Sept. 30, 2021, 6:50 p.m. UTC 
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" option that lets users change
this behavior.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>

---
 include/hw/virtio/virtio-iommu.h |  1 +
 hw/virtio/virtio-iommu.c         | 28 +++++++++++++++++++++++-----
 hw/virtio/trace-events           |  4 ++--
 3 files changed, 26 insertions(+), 7 deletions(-)

-- 
2.33.0

Comments

Eric Auger Oct. 6, 2021, 1:04 p.m. UTC | #1 
Hi jean,

On 9/30/21 8:50 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" option that lets users change

> this behavior.

s/option/property
set to true by default.

Also mention this deprecates the legacy VIRTIO_IOMMU_F_BYPASS feature
>

> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>

> ---

>  include/hw/virtio/virtio-iommu.h |  1 +

>  hw/virtio/virtio-iommu.c         | 28 +++++++++++++++++++++++-----

>  hw/virtio/trace-events           |  4 ++--

>  3 files changed, 26 insertions(+), 7 deletions(-)

>

> diff --git a/include/hw/virtio/virtio-iommu.h b/include/hw/virtio/virtio-iommu.h

> index 273e35c04b..4c66989ca4 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 1b23e8e18c..82edeaa101 100644

> --- a/hw/virtio/virtio-iommu.c

> +++ b/hw/virtio/virtio-iommu.c

> @@ -728,8 +728,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);

>  

> @@ -828,7 +827,8 @@ static void virtio_iommu_get_config(VirtIODevice *vdev, uint8_t *config_data)

>                                    config->input_range.start,

>                                    config->input_range.end,

>                                    config->domain_range.end,

> -                                  config->probe_size);

> +                                  config->probe_size,

> +                                  config->bypass);

>      memcpy(config_data, &dev->config, sizeof(struct virtio_iommu_config));

>  }

>  

> @@ -836,13 +836,29 @@ static void virtio_iommu_set_config(VirtIODevice *vdev,

>                                        const uint8_t *config_data)

>  {

>      struct virtio_iommu_config config;

> +    VirtIOIOMMU *dev = VIRTIO_IOMMU(vdev);

>  

>      memcpy(&config, config_data, sizeof(struct virtio_iommu_config));

> +

> +    if (config.bypass != dev->config.bypass) {

> +        if (!virtio_vdev_has_feature(vdev, VIRTIO_IOMMU_F_BYPASS_CONFIG)) {

> +            virtio_error(vdev, "cannot set config.bypass");

> +            return;

> +        }

> +        if (config.bypass != 0 && config.bypass != 1) {

> +            warn_report("invalid config.bypass value '%d'", config.bypass);

> +            dev->config.bypass = 0;

Shouldn't we leave the old value instead. The spec proposal says it
takes 0 though.
> +            return;

> +        }

> +        dev->config.bypass = config.bypass;

> +    }

> +

>      trace_virtio_iommu_set_config(config.page_size_mask,

>                                    config.input_range.start,

>                                    config.input_range.end,

>                                    config.domain_range.end,

> -                                  config.probe_size);

> +                                  config.probe_size,

> +                                  config.bypass);

>  }

>  

>  static uint64_t virtio_iommu_get_features(VirtIODevice *vdev, uint64_t f,

> @@ -986,6 +1002,7 @@ static void virtio_iommu_device_realize(DeviceState *dev, Error **errp)

>      s->config.input_range.end = -1UL;

>      s->config.domain_range.end = 32;

>      s->config.probe_size = VIOMMU_PROBE_SIZE;

> +    s->config.bypass = s->boot_bypass;

>  

>      virtio_add_feature(&s->features, VIRTIO_RING_F_EVENT_IDX);

>      virtio_add_feature(&s->features, VIRTIO_RING_F_INDIRECT_DESC);

> @@ -993,9 +1010,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);

>  

> @@ -1169,6 +1186,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(),

>  };

>  

> diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events

> index 8ed19e9d0c..6bc3821ba3 100644

> --- a/hw/virtio/trace-events

> +++ b/hw/virtio/trace-events

> @@ -90,8 +90,8 @@ virtio_mmio_setting_irq(int level) "virtio_mmio setting IRQ %d"

>  virtio_iommu_device_reset(void) "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_range, uint32_t probe_size) "page_size_mask=0x%"PRIx64" start=0x%"PRIx64" end=0x%"PRIx64" domain_range=%d probe_size=0x%x"

> -virtio_iommu_set_config(uint64_t page_size_mask, uint64_t start, uint64_t end, uint32_t domain_range, uint32_t probe_size) "page_size_mask=0x%"PRIx64" start=0x%"PRIx64" end=0x%"PRIx64" domain_bits=%d probe_size=0x%x"

> +virtio_iommu_get_config(uint64_t page_size_mask, uint64_t start, uint64_t end, uint32_t domain_range, uint32_t probe_size, uint8_t bypass) "page_size_mask=0x%"PRIx64" start=0x%"PRIx64" end=0x%"PRIx64" domain_range=%d probe_size=0x%x bypass=0x%x"

> +virtio_iommu_set_config(uint64_t page_size_mask, uint64_t start, uint64_t end, uint32_t domain_range, uint32_t probe_size, uint8_t bypass) "page_size_mask=0x%"PRIx64" start=0x%"PRIx64" end=0x%"PRIx64" domain_bits=%d probe_size=0x%x 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"

Thanks

Eric
Jean-Philippe Brucker Oct. 8, 2021, 10:21 a.m. UTC | #2 
On Wed, Oct 06, 2021 at 03:04:15PM +0200, Eric Auger wrote:
> > +    if (config.bypass != dev->config.bypass) {

> > +        if (!virtio_vdev_has_feature(vdev, VIRTIO_IOMMU_F_BYPASS_CONFIG)) {

> > +            virtio_error(vdev, "cannot set config.bypass");

> > +            return;

> > +        }

> > +        if (config.bypass != 0 && config.bypass != 1) {

> > +            warn_report("invalid config.bypass value '%d'", config.bypass);

> > +            dev->config.bypass = 0;

> Shouldn't we leave the old value instead. The spec proposal says it

> takes 0 though.


Yes keeping the old value is better, I'll change the spec. Writing a
different value is a driver bug since new values for the bypass field will
be keyed off a new feature bit.

Thanks,
Jean
diff mbox series

Patch

diff --git a/include/hw/virtio/virtio-iommu.h b/include/hw/virtio/virtio-iommu.h
index 273e35c04b..4c66989ca4 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 1b23e8e18c..82edeaa101 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -728,8 +728,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);
 
@@ -828,7 +827,8 @@  static void virtio_iommu_get_config(VirtIODevice *vdev, uint8_t *config_data)
                                   config->input_range.start,
                                   config->input_range.end,
                                   config->domain_range.end,
-                                  config->probe_size);
+                                  config->probe_size,
+                                  config->bypass);
     memcpy(config_data, &dev->config, sizeof(struct virtio_iommu_config));
 }
 
@@ -836,13 +836,29 @@  static void virtio_iommu_set_config(VirtIODevice *vdev,
                                       const uint8_t *config_data)
 {
     struct virtio_iommu_config config;
+    VirtIOIOMMU *dev = VIRTIO_IOMMU(vdev);
 
     memcpy(&config, config_data, sizeof(struct virtio_iommu_config));
+
+    if (config.bypass != dev->config.bypass) {
+        if (!virtio_vdev_has_feature(vdev, VIRTIO_IOMMU_F_BYPASS_CONFIG)) {
+            virtio_error(vdev, "cannot set config.bypass");
+            return;
+        }
+        if (config.bypass != 0 && config.bypass != 1) {
+            warn_report("invalid config.bypass value '%d'", config.bypass);
+            dev->config.bypass = 0;
+            return;
+        }
+        dev->config.bypass = config.bypass;
+    }
+
     trace_virtio_iommu_set_config(config.page_size_mask,
                                   config.input_range.start,
                                   config.input_range.end,
                                   config.domain_range.end,
-                                  config.probe_size);
+                                  config.probe_size,
+                                  config.bypass);
 }
 
 static uint64_t virtio_iommu_get_features(VirtIODevice *vdev, uint64_t f,
@@ -986,6 +1002,7 @@  static void virtio_iommu_device_realize(DeviceState *dev, Error **errp)
     s->config.input_range.end = -1UL;
     s->config.domain_range.end = 32;
     s->config.probe_size = VIOMMU_PROBE_SIZE;
+    s->config.bypass = s->boot_bypass;
 
     virtio_add_feature(&s->features, VIRTIO_RING_F_EVENT_IDX);
     virtio_add_feature(&s->features, VIRTIO_RING_F_INDIRECT_DESC);
@@ -993,9 +1010,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);
 
@@ -1169,6 +1186,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(),
 };
 
diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 8ed19e9d0c..6bc3821ba3 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -90,8 +90,8 @@  virtio_mmio_setting_irq(int level) "virtio_mmio setting IRQ %d"
 virtio_iommu_device_reset(void) "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_range, uint32_t probe_size) "page_size_mask=0x%"PRIx64" start=0x%"PRIx64" end=0x%"PRIx64" domain_range=%d probe_size=0x%x"
-virtio_iommu_set_config(uint64_t page_size_mask, uint64_t start, uint64_t end, uint32_t domain_range, uint32_t probe_size) "page_size_mask=0x%"PRIx64" start=0x%"PRIx64" end=0x%"PRIx64" domain_bits=%d probe_size=0x%x"
+virtio_iommu_get_config(uint64_t page_size_mask, uint64_t start, uint64_t end, uint32_t domain_range, uint32_t probe_size, uint8_t bypass) "page_size_mask=0x%"PRIx64" start=0x%"PRIx64" end=0x%"PRIx64" domain_range=%d probe_size=0x%x bypass=0x%x"
+virtio_iommu_set_config(uint64_t page_size_mask, uint64_t start, uint64_t end, uint32_t domain_range, uint32_t probe_size, uint8_t bypass) "page_size_mask=0x%"PRIx64" start=0x%"PRIx64" end=0x%"PRIx64" domain_bits=%d probe_size=0x%x 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"