Hi Alex,
On 9/19/23 19:22, Alex Williamson wrote:
> On Wed, 13 Sep 2023 10:01:47 +0200
> Eric Auger <eric.au...@redhat.com> wrote:
>
>> Now we retrieve the usable IOVA ranges from the host,
>> we now the physical IOMMU aperture and we can remove
>> the assumption of 64b IOVA space when calling
>> vfio_host_win_add().
>>
>> This works fine in general but in case of an IOMMU memory
>> region this becomes more tricky. For instance the virtio-iommu
>> MR has a 64b aperture by default. If the physical IOMMU has a
>> smaller aperture (typically the case for VTD), this means we
>> would need to resize the IOMMU MR when this latter is linked
>> to a container. However this happens on vfio_listener_region_add()
>> when calling the IOMMU MR set_iova_ranges() callback and this
>> would mean we would have a recursive call the
>> vfio_listener_region_add(). This looks like a wrong usage of
>> the memory API causing duplicate IOMMU MR notifier registration
>> for instance.
>>
>> Until we find a better solution, make sure the vfio_find_hostwin()
>> is not called anymore for IOMMU region.
>>
>> Signed-off-by: Eric Auger <eric.au...@redhat.com>
>>
>> ---
>>
>> I have not found any working solution to the IOMMU MR resizing.
>> So I can remove this patch or remove the check for IOMMU MR. Maybe
>> this is an issue which can be handled separately?
>> ---
>> hw/vfio/common.c | 25 ++++++++++++-------------
>> 1 file changed, 12 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 26da38de05..40cac1ca91 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -1112,13 +1112,6 @@ static void vfio_listener_region_add(MemoryListener
>> *listener,
>> #endif
>> }
>>
>> - hostwin = vfio_find_hostwin(container, iova, end);
>> - if (!hostwin) {
>> - error_setg(&err, "Container %p can't map guest IOVA region"
>> - " 0x%"HWADDR_PRIx"..0x%"HWADDR_PRIx, container, iova,
>> end);
>> - goto fail;
>> - }
>> -
>> memory_region_ref(section->mr);
>>
>> if (memory_region_is_iommu(section->mr)) {
>> @@ -1177,6 +1170,14 @@ static void vfio_listener_region_add(MemoryListener
>> *listener,
>> return;
>> }
>>
>> + hostwin = vfio_find_hostwin(container, iova, end);
>> + if (!hostwin) {
>> + error_setg(&err, "Container %p can't map guest IOVA region"
>> + " 0x%"HWADDR_PRIx"..0x%"HWADDR_PRIx, container, iova,
>> end);
>> + goto fail;
>> + }
>> +
>> +
>> /* Here we assume that memory_region_is_ram(section->mr)==true */
>>
>> /*
>> @@ -2594,12 +2595,10 @@ static int vfio_connect_container(VFIOGroup *group,
>> AddressSpace *as,
>> vfio_get_iommu_info_migration(container, info);
>> g_free(info);
>>
>> - /*
>> - * FIXME: We should parse VFIO_IOMMU_TYPE1_INFO_CAP_IOVA_RANGE
>> - * information to get the actual window extent rather than assume
>> - * a 64-bit IOVA address space.
>> - */
>> - vfio_host_win_add(container, 0, (hwaddr)-1, container->pgsizes);
>> + g_assert(container->nr_iovas);
> This assert is a problem for older kernels.
yes I will fix this.
>
>> + vfio_host_win_add(container, 0,
>> + container->iova_ranges[container->nr_iovas -
>> 1].end,
>> + container->pgsizes);
> This doesn't address the assumption about the min_iova and adds an
> assumption that the kernel provided list is sorted. Thanks,
yup.
Thanks!
Eric
>
> Alex
>
