* [Qemu-devel] [PATCH] hw/vfio/common: Refactor container initialization
@ 2019-01-11 16:45 Eric Auger
2019-01-14 6:32 ` Alexey Kardashevskiy
0 siblings, 1 reply; 4+ messages in thread
From: Eric Auger @ 2019-01-11 16:45 UTC (permalink / raw)
To: eric.auger.pro, eric.auger, qemu-devel, alex.williamson; +Cc: groug
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>
Reviewed-by: Greg Kurz <groug@kaod.org>
---
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);
--
2.17.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/vfio/common: Refactor container initialization
2019-01-11 16:45 [Qemu-devel] [PATCH] hw/vfio/common: Refactor container initialization Eric Auger
@ 2019-01-14 6:32 ` Alexey Kardashevskiy
2019-01-14 8:34 ` Auger Eric
0 siblings, 1 reply; 4+ messages in thread
From: Alexey Kardashevskiy @ 2019-01-14 6:32 UTC (permalink / raw)
To: Eric Auger, eric.auger.pro, qemu-devel, alex.williamson; +Cc: groug
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
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reviewed-by: Greg Kurz <groug@kaod.org>
>
> ---
>
> 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);
>
--
Alexey
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/vfio/common: Refactor container initialization
2019-01-14 6:32 ` Alexey Kardashevskiy
@ 2019-01-14 8:34 ` Auger Eric
2019-01-14 8:54 ` Greg Kurz
0 siblings, 1 reply; 4+ messages in thread
From: Auger Eric @ 2019-01-14 8:34 UTC (permalink / raw)
To: Alexey Kardashevskiy, eric.auger.pro, qemu-devel, alex.williamson; +Cc: groug
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 <eric.auger@redhat.com>
>> Reviewed-by: Greg Kurz <groug@kaod.org>
>>
>> ---
>>
>> 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);
>>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/vfio/common: Refactor container initialization
2019-01-14 8:34 ` Auger Eric
@ 2019-01-14 8:54 ` Greg Kurz
0 siblings, 0 replies; 4+ messages in thread
From: Greg Kurz @ 2019-01-14 8:54 UTC (permalink / raw)
To: Auger Eric
Cc: Alexey Kardashevskiy, eric.auger.pro, qemu-devel, alex.williamson
On Mon, 14 Jan 2019 09:34:12 +0100
Auger Eric <eric.auger@redhat.com> 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 <eric.auger@redhat.com>
> >> Reviewed-by: Greg Kurz <groug@kaod.org>
> >>
> >> ---
> >>
> >> 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);
> >>
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-01-14 8:55 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-11 16:45 [Qemu-devel] [PATCH] hw/vfio/common: Refactor container initialization Eric Auger
2019-01-14 6:32 ` Alexey Kardashevskiy
2019-01-14 8:34 ` Auger Eric
2019-01-14 8:54 ` Greg Kurz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.