Re: [PATCH/RFC 4/5] vfio: No-IOMMU mode support

[Alex Williamson]

On Fri, 9 Feb 2018 17:06:34 +0100
Geert Uytterhoeven  wrote:

> Hi Alex,
> 
> On Fri, Feb 9, 2018 at 4:50 PM, Alex Williamson
>  wrote:
> > On Fri,  9 Feb 2018 16:17:35 +0100
> > Geert Uytterhoeven  wrote:  
> >> From: Xiao Feng Ren 
> >>
> >> Add qemu support for the newly introduced VFIO No-IOMMU driver.
> >>
> >> We need to add special handling for:
> >> - Group character device is /dev/vfio/noiommu-$GROUP.
> >> - No-IOMMU does not rely on a memory listener.
> >> - No IOMMU will be set for its group, so no need to call
> >>   vfio_kvm_device_add_group.
> >>
> >> Signed-off-by: Xiao Feng Ren 
> >> [geert: Rebase]
> >> Signed-off-by: Geert Uytterhoeven 
> >> ---
> >>  hw/vfio/common.c  | 61 
> >> ++-
> >>  include/hw/vfio/vfio-common.h |  2 ++
> >>  2 files changed, 50 insertions(+), 13 deletions(-)  
> >
> > NAK.  I'm opposed to no-iommu support in QEMU in general, but accepting
> > vfio devices with no-iommu (which provide no DMA translation!!!)
> > transparently as if they might actually work like a regular vfio device
> > is absolutely unacceptable.  Without DMA translation and isolation, you
> > might want to think about another interface, I'm not keen on the idea  
> 
> What if the device cannot do DMA?
> 
> There are other devices that cannot do DMA, but that can be useful to
> access from a guest in an embedded system.
> E.g. smart ADC monitor blocks that monitor and average several ADCs in an
> automotive environment (drivers/iio/adc/rcar-gyroadc.c).
> 
> Should all such devices be paravirtualized?

How do we know that a device is not capable of DMA?  The moment we add
no-iommu support generically to QEMU, I get to deal with a swarm of
people trying to use it to assign DMA capable PCI devices to VMs and
failing miserably.  We added no-iommu support because the UIO PCI driver
was under pressure to add support for MSI/X interrupts and we figured
we could at least taint the kernel an give people a path to a more
secure setup with an IOMMU by allowing use of the vfio device
interface.  For PCI, this makes some sense because PCI has architected
interrupts and config space and standard layouts.  What's the value of
vfio over UIO for ad-hoc, non-DMA platform devices?  UIO is intended for
non-DMA devices.  vfio is intended for IOMMU protected, DMA capable
devices.  vfio no-IOMMU is a band-aide that hopefully goes away
eventually.

> > of corrupting vfio support in order to blink some LEDs.  Thanks,  
> 
> This GPIO+LED prototype is just a proof-of-concept. There's no plan to
> upstream it.

If vfio with no-IOMMU is to be used, it must not enable users to
casually assign DMA capable devices.  Certainly any PCI/e device needs
to be assumed to be DMA capable.  I don't know how you tell with
platform devices since there's no standard interface.  Thanks,

Alex

Re: [PATCH/RFC 4/5] vfio: No-IOMMU mode support

[Geert Uytterhoeven]

Hi Alex,

On Fri, Feb 9, 2018 at 4:50 PM, Alex Williamson
 wrote:
> On Fri,  9 Feb 2018 16:17:35 +0100
> Geert Uytterhoeven  wrote:
>> From: Xiao Feng Ren 
>>
>> Add qemu support for the newly introduced VFIO No-IOMMU driver.
>>
>> We need to add special handling for:
>> - Group character device is /dev/vfio/noiommu-$GROUP.
>> - No-IOMMU does not rely on a memory listener.
>> - No IOMMU will be set for its group, so no need to call
>>   vfio_kvm_device_add_group.
>>
>> Signed-off-by: Xiao Feng Ren 
>> [geert: Rebase]
>> Signed-off-by: Geert Uytterhoeven 
>> ---
>>  hw/vfio/common.c  | 61 
>> ++-
>>  include/hw/vfio/vfio-common.h |  2 ++
>>  2 files changed, 50 insertions(+), 13 deletions(-)
>
> NAK.  I'm opposed to no-iommu support in QEMU in general, but accepting
> vfio devices with no-iommu (which provide no DMA translation!!!)
> transparently as if they might actually work like a regular vfio device
> is absolutely unacceptable.  Without DMA translation and isolation, you
> might want to think about another interface, I'm not keen on the idea

What if the device cannot do DMA?

There are other devices that cannot do DMA, but that can be useful to
access from a guest in an embedded system.
E.g. smart ADC monitor blocks that monitor and average several ADCs in an
automotive environment (drivers/iio/adc/rcar-gyroadc.c).

Should all such devices be paravirtualized?

> of corrupting vfio support in order to blink some LEDs.  Thanks,

This GPIO+LED prototype is just a proof-of-concept. There's no plan to
upstream it.

Thanks!

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH/RFC 4/5] vfio: No-IOMMU mode support

[Alex Williamson]

On Fri,  9 Feb 2018 16:17:35 +0100
Geert Uytterhoeven  wrote:

> From: Xiao Feng Ren 
> 
> Add qemu support for the newly introduced VFIO No-IOMMU driver.
> 
> We need to add special handling for:
> - Group character device is /dev/vfio/noiommu-$GROUP.
> - No-IOMMU does not rely on a memory listener.
> - No IOMMU will be set for its group, so no need to call
>   vfio_kvm_device_add_group.
> 
> Signed-off-by: Xiao Feng Ren 
> [geert: Rebase]
> Signed-off-by: Geert Uytterhoeven 
> ---
>  hw/vfio/common.c  | 61 
> ++-
>  include/hw/vfio/vfio-common.h |  2 ++
>  2 files changed, 50 insertions(+), 13 deletions(-)

NAK.  I'm opposed to no-iommu support in QEMU in general, but accepting
vfio devices with no-iommu (which provide no DMA translation!!!)
transparently as if they might actually work like a regular vfio device
is absolutely unacceptable.  Without DMA translation and isolation, you
might want to think about another interface, I'm not keen on the idea
of corrupting vfio support in order to blink some LEDs.  Thanks,

Alex

> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index f895e3c3359af779..2ee94a3304202a74 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1019,6 +1019,33 @@ static int vfio_connect_container(VFIOGroup *group, 
> AddressSpace *as,
>  container->fd = fd;
>  QLIST_INIT(>giommu_list);
>  QLIST_INIT(>hostwin_list);
> +container->noiommu = group->noiommu;
> +
> +if (container->noiommu) {
> +ret = ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, );
> +if (ret) {
> +error_report("vfio: failed to set group container: %m");
> +ret = -errno;
> +goto free_container_exit;
> +}
> +
> +ret = ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_NOIOMMU_IOMMU);
> +if (!ret) {
> +error_report("vfio: No available IOMMU models");
> +ret = -EINVAL;
> +goto free_container_exit;
> +}
> +
> +ret = ioctl(fd, VFIO_SET_IOMMU, VFIO_NOIOMMU_IOMMU);
> +if (ret) {
> +error_report("vfio: failed to set iommu for container: %m");
> +ret = -errno;
> +goto free_container_exit;
> +}
> +
> +goto listener_register;
> +}
> +
>  if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_TYPE1_IOMMU) ||
>  ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_TYPE1v2_IOMMU)) {
>  bool v2 = !!ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_TYPE1v2_IOMMU);
> @@ -1151,15 +1178,16 @@ static int vfio_connect_container(VFIOGroup *group, 
> AddressSpace *as,
>  group->container = container;
>  QLIST_INSERT_HEAD(>group_list, group, container_next);
>  
> -container->listener = vfio_memory_listener;
> -
> -memory_listener_register(>listener, container->space->as);
> -
> -if (container->error) {
> -ret = container->error;
> -error_setg_errno(errp, -ret,
> - "memory listener initialization failed for 
> container");
> -goto listener_release_exit;
> +listener_register:
> +if (!container->noiommu) {
> +container->listener = vfio_memory_listener;
> +memory_listener_register(>listener, container->space->as);
> +if (container->error) {
> +ret = container->error;
> +error_setg_errno(errp, -ret,
> +"memory listener initialization failed for container");
> +goto listener_release_exit;
> +}
>  }
>  
>  container->initialized = true;
> @@ -1169,7 +1197,9 @@ listener_release_exit:
>  QLIST_REMOVE(group, container_next);
>  QLIST_REMOVE(container, next);
>  vfio_kvm_device_del_group(group);
> -vfio_listener_release(container);
> +if (!container->noiommu) {
> +vfio_listener_release(container);
> +}
>  
>  free_container_exit:
>  g_free(container);
> @@ -1195,7 +1225,7 @@ static void vfio_disconnect_container(VFIOGroup *group)
>   * since unset may destroy the backend container if it's the last
>   * group.
>   */
> -if (QLIST_EMPTY(>group_list)) {
> +if (QLIST_EMPTY(>group_list) && !container->noiommu) {
>  vfio_listener_release(container);
>  }
>  
> @@ -1249,8 +1279,13 @@ VFIOGroup *vfio_get_group(int groupid, AddressSpace 
> *as, Error **errp)
>  snprintf(path, sizeof(path), "/dev/vfio/%d", groupid);
>  group->fd = qemu_open(path, O_RDWR);
>  if (group->fd < 0) {
> -error_setg_errno(errp, errno, "failed to open %s", path);
> -goto free_group_exit;
> +snprintf(path, sizeof(path), "/dev/vfio/noiommu-%d", groupid);
> +group->fd = qemu_open(path, O_RDWR);
> +if (group->fd < 0) {
> +error_setg_errno(errp, errno, "failed to open %s", path);
> +goto free_group_exit;
> +}
> +group->noiommu = 1;
>  

[PATCH/RFC 4/5] vfio: No-IOMMU mode support

[Geert Uytterhoeven]

From: Xiao Feng Ren 

Add qemu support for the newly introduced VFIO No-IOMMU driver.

We need to add special handling for:
- Group character device is /dev/vfio/noiommu-$GROUP.
- No-IOMMU does not rely on a memory listener.
- No IOMMU will be set for its group, so no need to call
  vfio_kvm_device_add_group.

Signed-off-by: Xiao Feng Ren 
[geert: Rebase]
Signed-off-by: Geert Uytterhoeven 
---
 hw/vfio/common.c  | 61 ++-
 include/hw/vfio/vfio-common.h |  2 ++
 2 files changed, 50 insertions(+), 13 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index f895e3c3359af779..2ee94a3304202a74 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1019,6 +1019,33 @@ static int vfio_connect_container(VFIOGroup *group, 
AddressSpace *as,
 container->fd = fd;
 QLIST_INIT(>giommu_list);
 QLIST_INIT(>hostwin_list);
+container->noiommu = group->noiommu;
+
+if (container->noiommu) {
+ret = ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, );
+if (ret) {
+error_report("vfio: failed to set group container: %m");
+ret = -errno;
+goto free_container_exit;
+}
+
+ret = ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_NOIOMMU_IOMMU);
+if (!ret) {
+error_report("vfio: No available IOMMU models");
+ret = -EINVAL;
+goto free_container_exit;
+}
+
+ret = ioctl(fd, VFIO_SET_IOMMU, VFIO_NOIOMMU_IOMMU);
+if (ret) {
+error_report("vfio: failed to set iommu for container: %m");
+ret = -errno;
+goto free_container_exit;
+}
+
+goto listener_register;
+}
+
 if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_TYPE1_IOMMU) ||
 ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_TYPE1v2_IOMMU)) {
 bool v2 = !!ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_TYPE1v2_IOMMU);
@@ -1151,15 +1178,16 @@ static int vfio_connect_container(VFIOGroup *group, 
AddressSpace *as,
 group->container = container;
 QLIST_INSERT_HEAD(>group_list, group, container_next);
 
-container->listener = vfio_memory_listener;
-
-memory_listener_register(>listener, container->space->as);
-
-if (container->error) {
-ret = container->error;
-error_setg_errno(errp, -ret,
- "memory listener initialization failed for 
container");
-goto listener_release_exit;
+listener_register:
+if (!container->noiommu) {
+container->listener = vfio_memory_listener;
+memory_listener_register(>listener, container->space->as);
+if (container->error) {
+ret = container->error;
+error_setg_errno(errp, -ret,
+"memory listener initialization failed for container");
+goto listener_release_exit;
+}
 }
 
 container->initialized = true;
@@ -1169,7 +1197,9 @@ listener_release_exit:
 QLIST_REMOVE(group, container_next);
 QLIST_REMOVE(container, next);
 vfio_kvm_device_del_group(group);
-vfio_listener_release(container);
+if (!container->noiommu) {
+vfio_listener_release(container);
+}
 
 free_container_exit:
 g_free(container);
@@ -1195,7 +1225,7 @@ static void vfio_disconnect_container(VFIOGroup *group)
  * since unset may destroy the backend container if it's the last
  * group.
  */
-if (QLIST_EMPTY(>group_list)) {
+if (QLIST_EMPTY(>group_list) && !container->noiommu) {
 vfio_listener_release(container);
 }
 
@@ -1249,8 +1279,13 @@ VFIOGroup *vfio_get_group(int groupid, AddressSpace *as, 
Error **errp)
 snprintf(path, sizeof(path), "/dev/vfio/%d", groupid);
 group->fd = qemu_open(path, O_RDWR);
 if (group->fd < 0) {
-error_setg_errno(errp, errno, "failed to open %s", path);
-goto free_group_exit;
+snprintf(path, sizeof(path), "/dev/vfio/noiommu-%d", groupid);
+group->fd = qemu_open(path, O_RDWR);
+if (group->fd < 0) {
+error_setg_errno(errp, errno, "failed to open %s", path);
+goto free_group_exit;
+}
+group->noiommu = 1;
 }
 
 if (ioctl(group->fd, VFIO_GROUP_GET_STATUS, )) {
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index f3a2ac9fee02066f..aca2e33efb9b118c 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -77,6 +77,7 @@ struct VFIOGroup;
 typedef struct VFIOContainer {
 VFIOAddressSpace *space;
 int fd; /* /dev/vfio/vfio, empowered by the attached groups */
+bool noiommu;
 MemoryListener listener;
 MemoryListener prereg_listener;
 unsigned iommu_type;
@@ -136,6 +137,7 @@ struct VFIODeviceOps {
 typedef struct VFIOGroup {
 int fd;
 int groupid;
+bool noiommu;
 VFIOContainer *container;
 QLIST_HEAD(, VFIODevice) device_list;