Hi Alexey, On 1/14/19 7:32 AM, Alexey Kardashevskiy wrote: > > > On 12/01/2019 03:45, 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. > > > ioctl(VFIO_SET_IOMMU) is called twice for a reason - on sPAPR, the IOMMU > subdriver is always capable of v1 and v2 but the running platform is not > - powernv supports v2, pseries does not but there is no way telling this > until an IOMMU group is added to a container. So the existing code tries > v2, if that fails, it tries v1: > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=c6e7958eb76ed267f7
I definitively missed that point. I will add this behavior in vfio_init_container then. Thanks! Eric > > > >> >> Signed-off-by: Eric Auger <[email protected]> >> Reviewed-by: Greg Kurz <[email protected]> >> >> --- >> >> v2 -> v3: >> - originally submitted in [RFC v2 00/28] vSMMUv3/pSMMUv3 >> 2 stage VFIO integration >> - remove "nested only is selected if requested by @force_nested" >> - added Greg's R-b >> --- >> hw/vfio/common.c | 101 ++++++++++++++++++++++++++++++----------------- >> 1 file changed, 64 insertions(+), 37 deletions(-) >> >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >> index 7c185e5a2e..8535bfe66f 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> @@ -1036,12 +1036,57 @@ 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, errno, "failed to set group container"); >> + return -errno; >> + } >> + >> + ret = ioctl(container->fd, VFIO_SET_IOMMU, iommu_type); >> + if (ret) { >> + 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) >> { >> VFIOContainer *container; >> int ret, fd; >> VFIOAddressSpace *space; >> + int iommu_type; >> + bool v2 = false; >> + >> >> space = vfio_get_address_space(as); >> >> @@ -1101,23 +1146,20 @@ 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) { >> + 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,28 +1179,16 @@ 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: >> + v2 = true; >> + 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); >> + 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; >> } >> >> @@ -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); >> >
