On Mon, 14 Jan 2019 09:34:12 +0100 Auger Eric <[email protected]> wrote:
> 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. > So did I :( Please drop my R-b tag then and I'll review the next version. Cheers, -- Greg > 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); > >> > >
