On 23/02/2023 21:05, Alex Williamson wrote:
> On Thu, 23 Feb 2023 10:37:10 +0000
> Joao Martins <[email protected]> wrote:
>> On 22/02/2023 22:10, Alex Williamson wrote:
>>> On Wed, 22 Feb 2023 19:49:05 +0200
>>> Avihai Horon <[email protected]> wrote:
>>>> From: Joao Martins <[email protected]>
>>>> @@ -612,6 +665,16 @@ static int vfio_dma_map(VFIOContainer *container,
>>>> hwaddr iova,
>>>> .iova = iova,
>>>> .size = size,
>>>> };
>>>> + int ret;
>>>> +
>>>> + ret = vfio_record_mapping(container, iova, size, readonly);
>>>> + if (ret) {
>>>> + error_report("vfio: Failed to record mapping, iova: 0x%"
>>>> HWADDR_PRIx
>>>> + ", size: 0x" RAM_ADDR_FMT ", ret: %d (%s)",
>>>> + iova, size, ret, strerror(-ret));
>>>> +
>>>> + return ret;
>>>> + }
>>>
>>> Is there no way to replay the mappings when a migration is started?
>>> This seems like a horrible latency and bloat trade-off for the
>>> possibility that the VM might migrate and the device might support
>>> these features. Our performance with vIOMMU is already terrible, I
>>> can't help but believe this makes it worse. Thanks,
>>>
>>
>> It is a nop if the vIOMMU is being used (entries in container->giommu_list)
>> as
>> that uses a max-iova based IOVA range. So this is really for iommu identity
>> mapping and no-VIOMMU.
>
> Ok, yes, there are no mappings recorded for any containers that have a
> non-empty giommu_list.
>
>> We could replay them if they were tracked/stored anywhere.
>
> Rather than piggybacking on vfio_memory_listener, why not simply
> register a new MemoryListener when migration is started? That will
> replay all the existing ranges and allow tracking to happen separate
> from mapping, and only when needed.
>
The problem with that is that *starting* dirty tracking needs to have all the
range, we aren't supposed to start each range separately. So on a memory
listener callback you don't have introspection when you are dealing with the
last range, do we?
>> I suppose we could move the vfio_devices_all_device_dirty_tracking() into
>> this
>> patch and then conditionally call this vfio_{record,erase}_mapping() in case
>> we
>> are passing through a device that doesn't have live-migration support? Would
>> that address the impact you're concerned wrt to non-live-migrateable devices?
>>
>> On the other hand, the PCI device hotplug hypothetical even makes this a bit
>> complicated as we can still attempt to hotplug a device before migration is
>> even
>> attempted. Meaning that we start with live-migrateable devices, and we added
>> the
>> tracking, up to hotpluging a device without such support (adding a blocker)
>> leaving the mappings there with no further use. So it felt simpler to just
>> track
>> always and avoid any mappings recording if the vIOMMU is in active use?
>
> My preference would be that there's no runtime overhead for migration
> support until a migration is initiated. I currently don't see why we
> can't achieve that by dynamically adding a new MemoryListener around
> migration for that purpose. Do you? Thanks,
I definitely agree with the general sentiment of being more dynamic, but perhaps
I am not seeing how.
