Re: [PATCH] virtio-iommu: Default to bypass during boot

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Feb 18, 2021 at 11:59:30AM +0100, 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.
> 
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>

wouldn't this be a spec violation?


When the device is reset, endpoints are not attached to any domain.

If the VIRTIO_IOMMU_F_BYPASS feature is negotiated, all accesses from
unattached endpoints are allowed and translated by the IOMMU using the
identity function. If the feature is not negotiated, any memory access
from an unattached endpoint fails. Upon attaching an endpoint in
bypass mode to a new domain, any memory access from the endpoint fails,
since the domain does not contain any mapping.


Maybe it's not too late to change the spec here - want to try sending
a spec patch?




> ---
>  include/hw/virtio/virtio-iommu.h |  1 +
>  hw/virtio/virtio-iommu.c         | 23 +++++++++++++++++++++--
>  2 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/virtio/virtio-iommu.h b/include/hw/virtio/virtio-iommu.h
> index 273e35c04bc..4c66989ca4e 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 c2883a2f6c8..cd08dc39eca 100644
> --- a/hw/virtio/virtio-iommu.c
> +++ b/hw/virtio/virtio-iommu.c
> @@ -689,6 +689,25 @@ static void virtio_iommu_report_fault(VirtIOIOMMU *viommu, uint8_t reason,
>  
>  }
>  
> +static bool virtio_iommu_bypass_is_allowed(VirtIOIOMMU *s)
> +{
> +    VirtIODevice *vdev = &s->parent_obj;
> +
> +    /*
> +     * Allow bypass if:
> +     * - boot_bypass is enabled and the BYPASS feature hasn't yet been
> +     *   acknowledged.
> +     * - the BYPASS feature has been negotiated.
> +     */
> +    if (s->boot_bypass && !(vdev->status & VIRTIO_CONFIG_S_FEATURES_OK)) {
> +        return true;
> +    }
> +    if (virtio_vdev_has_feature(vdev, VIRTIO_IOMMU_F_BYPASS)) {
> +        return true;
> +    }
> +    return false;
> +}
> +
>  static IOMMUTLBEntry virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>                                              IOMMUAccessFlags flag,
>                                              int iommu_idx)
> @@ -715,8 +734,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 = virtio_iommu_bypass_is_allowed(s);
>  
>      sid = virtio_iommu_get_bdf(sdev);
>  
> @@ -1156,6 +1174,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(),
>  };
>  
> -- 
> 2.30.0