Re: [Qemu-devel] [RFC v9 08/18] vfio: improve vfio_get_group to support adding group without devices

[Alex Williamson <alex.williamson@redhat.com>]

On Tue, 2015-06-09 at 11:37 +0800, Chen Fan wrote:
> Pre-adding all affected groups for aer devices, it could
> ensure the affected groups are owned in VM.
> 
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> ---
>  hw/vfio/common.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index b1045da..4230f83 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -793,8 +793,15 @@ VFIOGroup *vfio_get_group(int groupid, AddressSpace *as)
>  
>      QLIST_FOREACH(group, &vfio_group_list, next) {
>          if (group->groupid == groupid) {
> +            if (as && !group->container) {
> +                if (vfio_connect_container(group, as)) {
> +                    error_report("vfio: failed to setup container for group %d",
> +                                 groupid);
> +                    return NULL;
> +                }
> +            }
>              /* Found it.  Now is it already in the right context? */
> -            if (group->container->space->as == as) {
> +            if (!as || group->container->space->as == as) {
>                  return group;
>              } else {
>                  error_report("vfio: group %d used in multiple address spaces",
> @@ -828,7 +835,7 @@ VFIOGroup *vfio_get_group(int groupid, AddressSpace *as)
>      group->groupid = groupid;
>      QLIST_INIT(&group->device_list);
>  
> -    if (vfio_connect_container(group, as)) {
> +    if (as && vfio_connect_container(group, as)) {
>          error_report("vfio: failed to setup container for group %d", groupid);
>          goto close_fd_exit;
>      }
> @@ -859,7 +866,9 @@ void vfio_put_group(VFIOGroup *group)
>      }
>  
>      vfio_kvm_device_del_group(group);
> -    vfio_disconnect_container(group);
> +    if (group->container) {
> +        vfio_disconnect_container(group);
> +    }
>      QLIST_REMOVE(group, next);
>      trace_vfio_put_group(group->fd);
>      close(group->fd);

It's an interesting idea, but should we instead separate the container
from vfio_get/put_group() to be done as a separate step?  Using the
AddressSpace pointer is a bit obfuscated.

Also, this gets you the group, but the vfio_group_get_external_user()
interface used by vfio-pci to do a bus reset requires that the group is
active, with an iommu.  If it didn't, we could potentially be resetting
a non-viable group, with devices still owned by the host or not isolated
by the iommu.  So you're a step closer, but if we go back to the example
of a dual port NIC with isolated functions, QEMU may be able to get the
group for the second port, it may even be a viable group, but if the
second port doesn't eventually get connected to the VM, the bus reset
won't be allowed.