hw/vfio/common.c | 112 +++++++++++++++++++++++++++++++---------------- 1 file changed, 75 insertions(+), 37 deletions(-)
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;
+ }
+
+ 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);
--
2.20.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);
>
--
Alexey
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);
>>
>
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);
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
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 >
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 Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
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 >
© 2016 - 2025 Red Hat, Inc.