diff mbox series

[v2] hw/vfio/common: Refactor container initialization

Message ID 20190117210245.18364-1-eric.auger@redhat.com
State New
Headers show
Series [v2] hw/vfio/common: Refactor container initialization | expand

Commit Message

Eric Auger Jan. 17, 2019, 9:02 p.m. UTC 
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.

Signed-off-by: Eric Auger <eric.auger@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(-)

Comments

Alexey Kardashevskiy Jan. 18, 2019, 4:51 a.m. UTC | #1 
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(-).


> 
> Signed-off-by: Eric Auger <eric.auger@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.


> +        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);
>
Greg Kurz Jan. 18, 2019, 8:52 a.m. UTC | #2 
On Thu, 17 Jan 2019 22:02:45 +0100
Eric Auger <eric.auger@redhat.com> 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.
> 
> Signed-off-by: Eric Auger <eric.auger@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");
> +        return ret;

This should be:

        error_setg_errno(errp, errno, "failed to set group container");
        return -errno;

> +    }
> +
> +    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;

Same here.

> +    }
> +    container->iommu_type = iommu_type;
> +    return ret;

    return 0;

> +}
> +
>  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);
Eric Auger Jan. 18, 2019, 8:52 a.m. UTC | #3 
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.auger@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);
>>
>
no-reply@patchew.org Jan. 23, 2019, 2:23 p.m. UTC | #4 
Patchew URL: https://patchew.org/QEMU/20190117210245.18364-1-eric.auger@redhat.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-mingw@fedora SHOW_ENV=1 J=14
=== TEST SCRIPT END ===

  CC      job-qmp.o
  CC      qdev-monitor.o
/tmp/qemu-test/src/block/sheepdog.c: In function 'find_vdi_name':
/tmp/qemu-test/src/block/sheepdog.c:1239:5: error: 'strncpy' specified bound 256 equals destination size [-Werror=stringop-truncation]
     strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors


The full log is available at
http://patchew.org/logs/20190117210245.18364-1-eric.auger@redhat.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Eric Auger Jan. 23, 2019, 2:44 p.m. UTC | #5 
Hi,

On 1/23/19 3:23 PM, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20190117210245.18364-1-eric.auger@redhat.com/
> 
> 
> 
> Hi,
> 
> This series failed the docker-mingw@fedora build test. Please find the testing commands and
> their output below. If you have Docker installed, you can probably reproduce it
> locally.
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> time make docker-test-mingw@fedora SHOW_ENV=1 J=14
> === TEST SCRIPT END ===
> 
>   CC      job-qmp.o
>   CC      qdev-monitor.o
> /tmp/qemu-test/src/block/sheepdog.c: In function 'find_vdi_name':
> /tmp/qemu-test/src/block/sheepdog.c:1239:5: error: 'strncpy' specified bound 256 equals destination size [-Werror=stringop-truncation]
>      strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
>      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
does not seem to be related to my patch.

Thanks

Eric

> cc1: all warnings being treated as errors
> 
> 
> The full log is available at
> http://patchew.org/logs/20190117210245.18364-1-eric.auger@redhat.com/testing.docker-mingw@fedora/?type=message.
> ---
> Email generated automatically by Patchew [http://patchew.org/].
> Please send your feedback to patchew-devel@redhat.com
>
Eric Blake Jan. 23, 2019, 3:01 p.m. UTC | #6 
On 1/23/19 8:44 AM, Auger Eric wrote:

>> /tmp/qemu-test/src/block/sheepdog.c: In function 'find_vdi_name':
>> /tmp/qemu-test/src/block/sheepdog.c:1239:5: error: 'strncpy' specified bound 256 equals destination size [-Werror=stringop-truncation]
>>      strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
>>      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> does not seem to be related to my patch.

Known false alarm; the patchew processing queue is backlogged, and is
testing patches against a version of qemu.git that does not yet have
patches for newer gcc compliance incorporated, while at the same time
having recently upgraded its testing environment to use the newer gcc
that warns.  Commit 97b583f4 will silence the noise, once patchew
catches up to using it.
Eric Auger Jan. 23, 2019, 3:31 p.m. UTC | #7 
Hi Eric,

On 1/23/19 4:01 PM, Eric Blake wrote:
> On 1/23/19 8:44 AM, Auger Eric wrote:
> 
>>> /tmp/qemu-test/src/block/sheepdog.c: In function 'find_vdi_name':
>>> /tmp/qemu-test/src/block/sheepdog.c:1239:5: error: 'strncpy' specified bound 256 equals destination size [-Werror=stringop-truncation]
>>>      strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
>>>      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> does not seem to be related to my patch.
> 
> Known false alarm; the patchew processing queue is backlogged, and is
> testing patches against a version of qemu.git that does not yet have
> patches for newer gcc compliance incorporated, while at the same time
> having recently upgraded its testing environment to use the newer gcc
> that warns.  Commit 97b583f4 will silence the noise, once patchew
> catches up to using it.
Thank you for the explanation.

Eric
>
diff mbox series

Patch

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");
+        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);