Message ID | 20190123154035.29425-1-eric.auger@redhat.com |
---|---|
State | New |
Headers | show |
Series | [v4] hw/vfio/common: Refactor container initialization | expand |
On Wed, 23 Jan 2019 16:40:35 +0100 Eric Auger <eric.auger@redhat.com> wrote: > We introduce the vfio_init_container_type() helper. > It computes the highest usable iommu type and then > set the container and the iommu type. > > Its usage in vfio_connect_container() makes the code > ready for addition of new iommu types. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > > --- > > v3 -> v4: > - Take into account Alex' suggestions. Only exception is > I kept v2 bool. > > v2 -> v3: > - fix ret/errno > > v1 -> v2: > - handle SPAPR case, s/ret/errno in error error_setg_errno, > fix ret value when vfio_iommu_get_type fails > - removed Greg's R-b > > v1: > - originally submitted in [RFC v2 00/28] vSMMUv3/pSMMUv3 > 2 stage VFIO integration > --- > hw/vfio/common.c | 117 +++++++++++++++++++++++++++++------------------ > 1 file changed, 72 insertions(+), 45 deletions(-) > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index 4262b80c44..eb209c776c 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -1036,6 +1036,61 @@ static void vfio_put_address_space(VFIOAddressSpace *space) > } > } > > +/* > + * vfio_get_iommu_type - selects the richest iommu_type (v2 first) > + */ > +static int vfio_get_iommu_type(VFIOContainer *container, > + Error **errp) > +{ > + int iommu_types[] = { VFIO_TYPE1v2_IOMMU, VFIO_TYPE1_IOMMU, > + VFIO_SPAPR_TCE_v2_IOMMU, VFIO_SPAPR_TCE_IOMMU }; > + int fd = container->fd; > + int i; nit, do we really need an fd variable since it's only used once now? > + > + for (i = 0; i < ARRAY_SIZE(iommu_types); i++) { > + if (ioctl(fd, VFIO_CHECK_EXTENSION, iommu_types[i])) { > + return iommu_types[i]; > + } > + } > + error_setg(errp, "No available IOMMU models"); > + return -EINVAL; > +} > + > +static int vfio_init_container_type(VFIOContainer *container, int group_fd, > + Error **errp) Sorry, I was a bit unclear with the suggestion of this function name, I meant to suggest this would have been a slightly more descriptive name for the previous version which had an iommu_type parameter. As the function now determines the type itself, the original name of simply vfio_init_container() looks correct. Looks good otherwise. Thanks, Alex > +{ > + int iommu_type, ret; > + > + iommu_type = vfio_get_iommu_type(container, errp); > + if (iommu_type < 0) { > + return iommu_type; > + } > + > + ret = ioctl(group_fd, VFIO_GROUP_SET_CONTAINER, &container->fd); > + if (ret) { > + error_setg_errno(errp, errno, "Failed to set group container"); > + return -errno; > + } > + > + while (ioctl(container->fd, VFIO_SET_IOMMU, iommu_type)) { > + if (iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) { > + /* > + * On sPAPR, despite the IOMMU subdriver always advertises v1 and > + * v2, the running platform may not support v2 and there is no > + * way to guess it until an IOMMU group gets added to the container. > + * So in case it fails with v2, try v1 as a fallback. > + */ > + iommu_type = VFIO_SPAPR_TCE_IOMMU; > + continue; > + } > + error_setg_errno(errp, errno, "Failed to set iommu for container"); > + return -errno; > + } > + > + container->iommu_type = iommu_type; > + return 0; > +} > + > static int vfio_connect_container(VFIOGroup *group, AddressSpace *as, > Error **errp) > { > @@ -1101,26 +1156,18 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as, > container->fd = fd; > QLIST_INIT(&container->giommu_list); > QLIST_INIT(&container->hostwin_list); > - 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); > + > + ret = vfio_init_container_type(container, group->fd, errp); > + if (ret) { > + goto free_container_exit; > + } > + > + switch (container->iommu_type) { > + case VFIO_TYPE1v2_IOMMU: > + case VFIO_TYPE1_IOMMU: > + { > struct vfio_iommu_type1_info info; > > - ret = ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &fd); > - if (ret) { > - error_setg_errno(errp, errno, "failed to set group container"); > - ret = -errno; > - goto free_container_exit; > - } > - > - container->iommu_type = v2 ? VFIO_TYPE1v2_IOMMU : VFIO_TYPE1_IOMMU; > - ret = ioctl(fd, VFIO_SET_IOMMU, container->iommu_type); > - if (ret) { > - error_setg_errno(errp, errno, "failed to set iommu for container"); > - ret = -errno; > - goto free_container_exit; > - } > - > /* > * FIXME: This assumes that a Type1 IOMMU can map any 64-bit > * IOVA whatsoever. That's not actually true, but the current > @@ -1137,30 +1184,13 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as, > } > vfio_host_win_add(container, 0, (hwaddr)-1, info.iova_pgsizes); > container->pgsizes = info.iova_pgsizes; > - } else if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_IOMMU) || > - ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_v2_IOMMU)) { > + break; > + } > + case VFIO_SPAPR_TCE_v2_IOMMU: > + case VFIO_SPAPR_TCE_IOMMU: > + { > struct vfio_iommu_spapr_tce_info info; > - bool v2 = !!ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_v2_IOMMU); > - > - ret = ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &fd); > - if (ret) { > - error_setg_errno(errp, errno, "failed to set group container"); > - ret = -errno; > - goto free_container_exit; > - } > - container->iommu_type = > - v2 ? VFIO_SPAPR_TCE_v2_IOMMU : VFIO_SPAPR_TCE_IOMMU; > - ret = ioctl(fd, VFIO_SET_IOMMU, container->iommu_type); > - if (ret) { > - container->iommu_type = VFIO_SPAPR_TCE_IOMMU; > - v2 = false; > - ret = ioctl(fd, VFIO_SET_IOMMU, container->iommu_type); > - } > - if (ret) { > - error_setg_errno(errp, errno, "failed to set iommu for container"); > - ret = -errno; > - goto free_container_exit; > - } > + bool v2 = container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU; > > /* > * The host kernel code implementing VFIO_IOMMU_DISABLE is called > @@ -1222,10 +1252,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as, > info.dma32_window_size - 1, > 0x1000); > } > - } else { > - error_setg(errp, "No available IOMMU models"); > - ret = -EINVAL; > - goto free_container_exit; > + } > } > > vfio_kvm_device_add_group(group);
diff --git a/hw/vfio/common.c b/hw/vfio/common.c index 4262b80c44..eb209c776c 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -1036,6 +1036,61 @@ static void vfio_put_address_space(VFIOAddressSpace *space) } } +/* + * vfio_get_iommu_type - selects the richest iommu_type (v2 first) + */ +static int vfio_get_iommu_type(VFIOContainer *container, + Error **errp) +{ + int iommu_types[] = { VFIO_TYPE1v2_IOMMU, VFIO_TYPE1_IOMMU, + VFIO_SPAPR_TCE_v2_IOMMU, VFIO_SPAPR_TCE_IOMMU }; + int fd = container->fd; + int i; + + for (i = 0; i < ARRAY_SIZE(iommu_types); i++) { + if (ioctl(fd, VFIO_CHECK_EXTENSION, iommu_types[i])) { + return iommu_types[i]; + } + } + error_setg(errp, "No available IOMMU models"); + return -EINVAL; +} + +static int vfio_init_container_type(VFIOContainer *container, int group_fd, + Error **errp) +{ + int iommu_type, ret; + + iommu_type = vfio_get_iommu_type(container, errp); + if (iommu_type < 0) { + return iommu_type; + } + + ret = ioctl(group_fd, VFIO_GROUP_SET_CONTAINER, &container->fd); + if (ret) { + error_setg_errno(errp, errno, "Failed to set group container"); + return -errno; + } + + while (ioctl(container->fd, VFIO_SET_IOMMU, iommu_type)) { + if (iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) { + /* + * On sPAPR, despite the IOMMU subdriver always advertises v1 and + * v2, the running platform may not support v2 and there is no + * way to guess it until an IOMMU group gets added to the container. + * So in case it fails with v2, try v1 as a fallback. + */ + iommu_type = VFIO_SPAPR_TCE_IOMMU; + continue; + } + error_setg_errno(errp, errno, "Failed to set iommu for container"); + return -errno; + } + + container->iommu_type = iommu_type; + return 0; +} + static int vfio_connect_container(VFIOGroup *group, AddressSpace *as, Error **errp) { @@ -1101,26 +1156,18 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as, container->fd = fd; QLIST_INIT(&container->giommu_list); QLIST_INIT(&container->hostwin_list); - 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); + + ret = vfio_init_container_type(container, group->fd, errp); + if (ret) { + goto free_container_exit; + } + + switch (container->iommu_type) { + case VFIO_TYPE1v2_IOMMU: + case VFIO_TYPE1_IOMMU: + { struct vfio_iommu_type1_info info; - ret = ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &fd); - if (ret) { - error_setg_errno(errp, errno, "failed to set group container"); - ret = -errno; - goto free_container_exit; - } - - container->iommu_type = v2 ? VFIO_TYPE1v2_IOMMU : VFIO_TYPE1_IOMMU; - ret = ioctl(fd, VFIO_SET_IOMMU, container->iommu_type); - if (ret) { - error_setg_errno(errp, errno, "failed to set iommu for container"); - ret = -errno; - goto free_container_exit; - } - /* * FIXME: This assumes that a Type1 IOMMU can map any 64-bit * IOVA whatsoever. That's not actually true, but the current @@ -1137,30 +1184,13 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as, } vfio_host_win_add(container, 0, (hwaddr)-1, info.iova_pgsizes); container->pgsizes = info.iova_pgsizes; - } else if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_IOMMU) || - ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_v2_IOMMU)) { + break; + } + case VFIO_SPAPR_TCE_v2_IOMMU: + case VFIO_SPAPR_TCE_IOMMU: + { struct vfio_iommu_spapr_tce_info info; - bool v2 = !!ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_v2_IOMMU); - - ret = ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &fd); - if (ret) { - error_setg_errno(errp, errno, "failed to set group container"); - ret = -errno; - goto free_container_exit; - } - container->iommu_type = - v2 ? VFIO_SPAPR_TCE_v2_IOMMU : VFIO_SPAPR_TCE_IOMMU; - ret = ioctl(fd, VFIO_SET_IOMMU, container->iommu_type); - if (ret) { - container->iommu_type = VFIO_SPAPR_TCE_IOMMU; - v2 = false; - ret = ioctl(fd, VFIO_SET_IOMMU, container->iommu_type); - } - if (ret) { - error_setg_errno(errp, errno, "failed to set iommu for container"); - ret = -errno; - goto free_container_exit; - } + bool v2 = container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU; /* * The host kernel code implementing VFIO_IOMMU_DISABLE is called @@ -1222,10 +1252,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as, info.dma32_window_size - 1, 0x1000); } - } else { - error_setg(errp, "No available IOMMU models"); - ret = -EINVAL; - goto free_container_exit; + } } vfio_kvm_device_add_group(group);
We introduce the vfio_init_container_type() helper. It computes the highest usable iommu type and then set the container and the iommu type. Its usage in vfio_connect_container() makes the code ready for addition of new iommu types. Signed-off-by: Eric Auger <eric.auger@redhat.com> --- v3 -> v4: - Take into account Alex' suggestions. Only exception is I kept v2 bool. v2 -> v3: - fix ret/errno v1 -> v2: - handle SPAPR case, s/ret/errno in error error_setg_errno, fix ret value when vfio_iommu_get_type fails - removed Greg's R-b v1: - originally submitted in [RFC v2 00/28] vSMMUv3/pSMMUv3 2 stage VFIO integration --- hw/vfio/common.c | 117 +++++++++++++++++++++++++++++------------------ 1 file changed, 72 insertions(+), 45 deletions(-)