Re: [EXTERNAL] Re: [PATCH] vdpa: Add support for no-IOMMU mode

From: "Michael S. Tsirkin" <mst@redhat.com>
Date: 2024-09-10 05:56:56
Also in: kvm

On Wed, Aug 28, 2024 at 09:08:13AM +0000, Srujana Challa wrote:

quoted

Subject: RE: [EXTERNAL] Re: [PATCH] vdpa: Add support for no-IOMMU mode

quoted

On Tue, Jul 23, 2024 at 07:10:52AM +0000, Srujana Challa wrote:

quoted

On Mon, Jul 22, 2024 at 03:22:22PM +0800, Jason Wang wrote:

quoted

On Fri, Jul 19, 2024 at 11:40 PM Srujana Challa
[off-list ref]

wrote:

quoted

On Thu, May 30, 2024 at 03:48:23PM +0530, Srujana Challa wrote:

quoted

This commit introduces support for an UNSAFE, no-IOMMU
mode in the vhost-vdpa driver. When enabled, this mode
provides no device isolation, no DMA translation, no host
kernel protection, and cannot be used for device
assignment to virtual machines. It requires RAWIO
permissions and will taint the

kernel.

quoted

This mode requires enabling the

"enable_vhost_vdpa_unsafe_noiommu_mode"

quoted

option on the vhost-vdpa driver. This mode would be useful
to get better performance on specifice low end machines
and can be leveraged by embedded platforms where
applications run in controlled

environment.

quoted

Signed-off-by: Srujana Challa <schalla@marvell.com>

Thought hard about that.
I think given vfio supports this, we can do that too, and
the extension is

small.

quoted

However, it looks like setting this parameter will
automatically change the behaviour for existing userspace
when

IOMMU_DOMAIN_IDENTITY is set.

Our initial thought was to support only for no-iommu case, in which
domain

itself

quoted

won't be exist.   So, we can modify the code as below to check for only

presence of domain.

quoted

I think,  only handling of no-iommu case wouldn't effect the
existing

userspace.

quoted

+   if ((!domain) && vhost_vdpa_noiommu && capable(CAP_SYS_RAWIO))

quoted

I would prefer some explicit action.
Just not specifying a domain is something I'd like to keep reserved
for something of more wide usefulness.

Can we introduce a new feature like VHOST_BACKEND_F_NOIOMMU in
VHOST_VDPA_BACKEND_FEATURES?  We can have below logic based on this
feature bit negotiation.
Thanks.

Michael, could you please confirm if adding a new feature to VHOST_VDPA_BACKEND_FEATURES
is an appropriate solution to support no-IOMMU for the vhost-vdpa backend?


Yes. So the idea is to require both a module parameter, and a
flag set by userspace, to make sure users do not mistakenly
try to assign such devices to VMs.

Thanks.

quoted

I suggest a new domain type for use just for this purpose.

I'm not sure I get this, we want to bypass IOMMU, so it doesn't
even have a doman.

yes, a fake one. or come up with some other flag that userspace will set.

quoted

This way if host has

quoted

an iommu, then the same kernel can run both VMs with
isolation and unsafe embedded apps without.

Could you provide further details on this concept? What
criteria would determine the configuration of the new domain
type? Would this require a boot parameter similar to
IOMMU_DOMAIN_IDENTITY, such as

iommu.passthrough=1 or iommu.pt?

quoted

Thanks

quoted

---
 drivers/vhost/vdpa.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index bc4a51e4638b..d071c30125aa 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c

@@ -36,6 +36,11 @@ enum {

 #define VHOST_VDPA_IOTLB_BUCKETS 16

+bool vhost_vdpa_noiommu;

+module_param_named(enable_vhost_vdpa_unsafe_noiommu_mode,

quoted

quoted
quoted
quoted
quoted
quoted
+              vhost_vdpa_noiommu, bool, 0644);

+MODULE_PARM_DESC(enable_vhost_vdpa_unsafe_noiommu_mode,

quoted

"Enable

quoted

+UNSAFE, no-IOMMU mode.  This mode provides no device
+isolation, no DMA translation, no host kernel protection,
+cannot be used for device assignment to virtual machines,
+requires RAWIO permissions, and will taint the kernel.
+If you do not know what this is

for, step away.

quoted

+(default: false)");
+
 struct vhost_vdpa_as {
    struct hlist_node hash_link;
    struct vhost_iotlb iotlb; @@ -60,6 +65,7 @@ struct
vhost_vdpa {
    struct vdpa_iova_range range;
    u32 batch_asid;
    bool suspended;
+   bool noiommu_en;
 };

 static DEFINE_IDA(vhost_vdpa_ida); @@ -887,6 +893,10 @@
static void vhost_vdpa_general_unmap(struct vhost_vdpa *v,  {
    struct vdpa_device *vdpa = v->vdpa;
    const struct vdpa_config_ops *ops = vdpa->config;
+
+   if (v->noiommu_en)
+           return;
+
    if (ops->dma_map) {
            ops->dma_unmap(vdpa, asid, map->start, map->size);
    } else if (ops->set_map == NULL) { @@ -980,6 +990,9 @@
static int vhost_vdpa_map(struct vhost_vdpa *v,

struct vhost_iotlb *iotlb,

quoted

    if (r)
            return r;

+   if (v->noiommu_en)
+           goto skip_map;
+
    if (ops->dma_map) {
            r = ops->dma_map(vdpa, asid, iova, size, pa, perm, opaque);
    } else if (ops->set_map) { @@ -995,6 +1008,7 @@ static
int vhost_vdpa_map(struct vhost_vdpa *v,

struct vhost_iotlb *iotlb,

quoted

            return r;
    }

+skip_map:
    if (!vdpa->use_va)
            atomic64_add(PFN_DOWN(size),
&dev->mm->pinned_vm);

@@ -1298,6 +1312,7 @@ static int

vhost_vdpa_alloc_domain(struct

vhost_vdpa *v)

quoted

    struct vdpa_device *vdpa = v->vdpa;
    const struct vdpa_config_ops *ops = vdpa->config;
    struct device *dma_dev = vdpa_get_dma_dev(vdpa);
+   struct iommu_domain *domain;
    const struct bus_type *bus;
    int ret;

@@ -1305,6 +1320,14 @@ static int

vhost_vdpa_alloc_domain(struct

vhost_vdpa *v)

quoted

    if (ops->set_map || ops->dma_map)
            return 0;

+   domain = iommu_get_domain_for_dev(dma_dev);
+   if ((!domain || domain->type == IOMMU_DOMAIN_IDENTITY)

&&

quoted

+       vhost_vdpa_noiommu && capable(CAP_SYS_RAWIO)) {

So if userspace does not have CAP_SYS_RAWIO instead of
failing with a permission error the functionality changes silently?
That's confusing, I think.

Yes, you are correct. I will modify the code to return error
when vhost_vdpa_noiommu is set and CAP_SYS_RAWIO is not set.

Thanks.

quoted

+           add_taint(TAINT_USER, LOCKDEP_STILL_OK);
+           dev_warn(&v->dev, "Adding kernel taint for
+ noiommu on

device\n");

quoted

+           v->noiommu_en = true;
+           return 0;
+   }
    bus = dma_dev->bus;
    if (!bus)
            return -EFAULT;
--
2.25.1

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help