Hi Peter,
On 5/28/19 4:47 AM, Peter Xu wrote:
> On Mon, May 27, 2019 at 01:41:44PM +0200, Eric Auger wrote:
>> In case we detect the address space is translated by
>> a virtual IOMMU which requires nested stages, let's set up
>> the container with the VFIO_TYPE1_NESTING_IOMMU iommu_type.
>>
>> Signed-off-by: Eric Auger <[email protected]>
>>
>> ---
>>
>> v2 -> v3:
>> - add "nested only is selected if requested by @force_nested"
>> comment in this patch
>> ---
>> hw/vfio/common.c | 27 +++++++++++++++++++++++----
>> 1 file changed, 23 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 1f1deff360..99ade21056 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -1136,14 +1136,19 @@ static void vfio_put_address_space(VFIOAddressSpace
>> *space)
>> * vfio_get_iommu_type - selects the richest iommu_type (v2 first)
>> */
>> static int vfio_get_iommu_type(VFIOContainer *container,
>> + bool force_nested,
>> Error **errp)
>> {
>> - int iommu_types[] = { VFIO_TYPE1v2_IOMMU, VFIO_TYPE1_IOMMU,
>> + int iommu_types[] = { VFIO_TYPE1_NESTING_IOMMU,
>> + VFIO_TYPE1v2_IOMMU, VFIO_TYPE1_IOMMU,
>> VFIO_SPAPR_TCE_v2_IOMMU, VFIO_SPAPR_TCE_IOMMU };
>> int i;
>>
>> for (i = 0; i < ARRAY_SIZE(iommu_types); i++) {
>> if (ioctl(container->fd, VFIO_CHECK_EXTENSION, iommu_types[i])) {
>> + if (iommu_types[i] == VFIO_TYPE1_NESTING_IOMMU &&
>> !force_nested) {
>
> If force_nested==true and if the kernel does not support
> VFIO_TYPE1_NESTING_IOMMU, we will still return other iommu types?
> That seems to not match with what "force" mean here.
>
> What I feel like is that we want an "iommu_nest_types[]" which only
> contains VFIO_TYPE1_NESTING_IOMMU. Then:
>
> if (nested) {
> target_types = iommu_nest_types;
> } else {
> target_types = iommu_types;
> }
>
> foreach (target_types)
> ...
>
> return -EINVAL;
>
> Might be clearer? Then we can drop [2] below since we'll fail earlier
> at [1].
agreed. I can fail immediately in case the nested mode was requested and
not supported. This will be clearer.
Thanks!
Eric
>
>> + continue;
>> + }
>> return iommu_types[i];
>> }
>> }
>> @@ -1152,11 +1157,11 @@ static int vfio_get_iommu_type(VFIOContainer
>> *container,
>> }
>>
>> static int vfio_init_container(VFIOContainer *container, int group_fd,
>> - Error **errp)
>> + bool force_nested, Error **errp)
>> {
>> int iommu_type, ret;
>>
>> - iommu_type = vfio_get_iommu_type(container, errp);
>> + iommu_type = vfio_get_iommu_type(container, force_nested, errp);
>> if (iommu_type < 0) {
>> return iommu_type;
>
> [1]
>
>> }
>> @@ -1192,6 +1197,14 @@ static int vfio_connect_container(VFIOGroup *group,
>> AddressSpace *as,
>> VFIOContainer *container;
>> int ret, fd;
>> VFIOAddressSpace *space;
>> + IOMMUMemoryRegion *iommu_mr;
>> + bool force_nested = false;
>> +
>> + if (as != &address_space_memory && memory_region_is_iommu(as->root)) {
>> + iommu_mr = IOMMU_MEMORY_REGION(as->root);
>> + memory_region_iommu_get_attr(iommu_mr, IOMMU_ATTR_VFIO_NESTED,
>> + (void *)&force_nested);
>> + }
>>
>> space = vfio_get_address_space(as);
>>
>> @@ -1252,12 +1265,18 @@ static int vfio_connect_container(VFIOGroup *group,
>> AddressSpace *as,
>> QLIST_INIT(&container->giommu_list);
>> QLIST_INIT(&container->hostwin_list);
>>
>> - ret = vfio_init_container(container, group->fd, errp);
>> + ret = vfio_init_container(container, group->fd, force_nested, errp);
>> if (ret) {
>> goto free_container_exit;
>> }
>>
>> + if (force_nested && container->iommu_type != VFIO_TYPE1_NESTING_IOMMU) {
>> + error_setg(errp, "nested mode requested by the virtual IOMMU "
>> + "but not supported by the vfio iommu");
>> + }
>
> [2]
>
>> +
>> switch (container->iommu_type) {
>> + case VFIO_TYPE1_NESTING_IOMMU:
>> case VFIO_TYPE1v2_IOMMU:
>> case VFIO_TYPE1_IOMMU:
>> {
>> --
>> 2.20.1
>>
>
> Regards,
>
