Hi Alexey, On 1/18/19 5:51 AM, Alexey Kardashevskiy wrote: > > > On 18/01/2019 08:02, Eric Auger wrote: >> In vfio_connect_container() the code that selects the >> iommu type can benefit from helpers such as >> vfio_iommu_get_type() and vfio_init_container(). As >> a result we end up with a switch/case on the iommu type >> that makes the code a little bit more readable and ready >> for addition of new iommu types. Also ioctl's get called >> once per iommu_type. > > > I'd argue that. This adds 2 more functions to deal with different IOMMU > types: 75 insertions(+), 37 deletions(-).
The rationale behind this patch is I plan to introduce a new nested mode for ARM: see https://github.com/eauger/qemu/commit/e20cc45073e753f314578eb83ce77b11a4879b2d If we keep the existing code organization I don't think this kind of addition will be readable. > > >> >> Signed-off-by: Eric Auger <eric.au...@redhat.com> >> >> --- >> >> 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 | 112 +++++++++++++++++++++++++++++++---------------- >> 1 file changed, 75 insertions(+), 37 deletions(-) >> >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >> index 4262b80c44..33335cee47 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> @@ -1036,12 +1036,65 @@ static void vfio_put_address_space(VFIOAddressSpace >> *space) >> } >> } >> >> +/* >> + * vfio_iommu_get_type - selects the richest iommu_type (v2 first) >> + */ >> +static int vfio_iommu_get_type(VFIOContainer *container, >> + Error **errp) >> +{ >> + int fd = container->fd; >> + >> + if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_TYPE1v2_IOMMU)) { >> + return VFIO_TYPE1v2_IOMMU; >> + } else if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_TYPE1_IOMMU)) { >> + return VFIO_TYPE1_IOMMU; >> + } else if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_v2_IOMMU)) { >> + return VFIO_SPAPR_TCE_v2_IOMMU; >> + } else if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_IOMMU)) { >> + return VFIO_SPAPR_TCE_IOMMU; >> + } else { >> + error_setg(errp, "No available IOMMU models"); >> + return -EINVAL; >> + } >> +} >> + >> +static int vfio_init_container(VFIOContainer *container, int group_fd, >> + int iommu_type, Error **errp) >> +{ >> + int ret; >> + >> + ret = ioctl(group_fd, VFIO_GROUP_SET_CONTAINER, &container->fd); >> + if (ret) { >> + error_setg_errno(errp, -ret, "failed to set group container"); > > > > This replaces errno with ret which is not the same thing. yes going to fix that :-( Thanks Eric > > >> + return ret; >> + } >> + >> + ret = ioctl(container->fd, VFIO_SET_IOMMU, iommu_type); >> + if (ret && 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; >> + ret = ioctl(container->fd, VFIO_SET_IOMMU, iommu_type); >> + } >> + if (ret) { >> + error_setg_errno(errp, -ret, "failed to set iommu for container"); >> + return ret; >> + } >> + container->iommu_type = iommu_type; >> + return ret; >> +} >> + >> static int vfio_connect_container(VFIOGroup *group, AddressSpace *as, >> Error **errp) >> { >> VFIOContainer *container; >> int ret, fd; >> VFIOAddressSpace *space; >> + int iommu_type; >> >> space = vfio_get_address_space(as); >> >> @@ -1101,23 +1154,21 @@ 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); >> + >> + iommu_type = vfio_iommu_get_type(container, errp); >> + if (iommu_type < 0) { >> + ret = -EINVAL; >> + goto free_container_exit; >> + } >> + >> + switch (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); >> + ret = vfio_init_container(container, group->fd, iommu_type, errp); >> 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; >> } >> >> @@ -1137,31 +1188,21 @@ 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); >> + bool v2; >> >> - ret = ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &fd); >> + ret = vfio_init_container(container, group->fd, iommu_type, errp); >> 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; >> } >> >> + v2 = container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU; >> + >> /* >> * The host kernel code implementing VFIO_IOMMU_DISABLE is called >> * when container fd is closed so we do not call it explicitly >> @@ -1222,10 +1263,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); >> >
