From: Eric Auger <eric.auger@redhat.com>
To: Jean-Philippe Brucker <jean-philippe@linaro.org>
Cc: qemu-devel@nongnu.org, mst@redhat.com
Subject: Re: [PATCH 2/3] virtio-iommu: Default to bypass during boot
Date: Wed, 6 Oct 2021 15:04:15 +0200 [thread overview]
Message-ID: <8fc7acc5-2ce5-293a-0db3-40ff52b91803@redhat.com> (raw)
In-Reply-To: <20210930185050.262759-3-jean-philippe@linaro.org>
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
next prev parent reply other threads:[~2021-10-06 13:06 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-30 18:50 [PATCH 0/3] virtio-iommu: Support VIRTIO_IOMMU_F_BYPASS_CONFIG Jean-Philippe Brucker
2021-09-30 18:50 ` [PATCH 1/3] NOMERGE: virtio-iommu: Add definitions for VIRTIO_IOMMU_F_BYPASS_CONFIG Jean-Philippe Brucker
2021-09-30 18:50 ` [PATCH 2/3] virtio-iommu: Default to bypass during boot Jean-Philippe Brucker
2021-10-06 13:04 ` Eric Auger [this message]
2021-10-08 10:21 ` Jean-Philippe Brucker
2021-09-30 18:50 ` [PATCH 3/3] virtio-iommu: Support bypass domain Jean-Philippe Brucker
2021-10-06 13:10 ` Eric Auger
2022-01-11 9:02 ` [PATCH 0/3] virtio-iommu: Support VIRTIO_IOMMU_F_BYPASS_CONFIG Eric Auger
2022-01-11 10:13 ` Jean-Philippe Brucker
2022-01-11 10:33 ` Eric Auger
2022-01-11 15:40 ` Michael S. Tsirkin
2022-01-11 16:24 ` Jean-Philippe Brucker
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8fc7acc5-2ce5-293a-0db3-40ff52b91803@redhat.com \
--to=eric.auger@redhat.com \
--cc=jean-philippe@linaro.org \
--cc=mst@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).