Re: [PATCH V3 10/42] vfio/container: restore DMA vaddr

[Steven Sistare <steven.sistare@oracle.com>]

On 5/22/2025 2:37 AM, Cédric Le Goater wrote:
> On 5/12/25 17:32, Steve Sistare wrote:
>> In new QEMU, do not register the memory listener at device creation time.
>> Register it later, in the container post_load handler, after all vmstate
>> that may affect regions and mapping boundaries has been loaded.  The
>> post_load registration will cause the listener to invoke its callback on
>> each flat section, and the calls will match the mappings remembered by the
>> kernel.
>>
>> The listener calls a special dma_map handler that passes the new VA of each
>> section to the kernel using VFIO_DMA_MAP_FLAG_VADDR.  Restore the normal
>> handler at the end.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>>   hw/vfio/container.c  | 15 +++++++++++++--
>>   hw/vfio/cpr-legacy.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 61 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>> index a554683..0e02726 100644
>> --- a/hw/vfio/container.c
>> +++ b/hw/vfio/container.c
>> @@ -137,6 +137,8 @@ static int vfio_legacy_dma_unmap_one(const VFIOContainerBase *bcontainer,
>>       int ret;
>>       Error *local_err = NULL;
>> +    assert(!container->cpr.reused);
> 
> assert -> g_assert

will do.

> this can be called at runtime, which would mean crashing QEMU in case
> of error. Doing an error_report() call is more friendly.

It is an internal error if this assertion is hit, so the state of the system
cannot be trusted.  Hence assert rather than error_report and attempt to recover.

- Steve

>> +
>>       if (iotlb && vfio_container_dirty_tracking_is_started(bcontainer)) {
>>           if (!vfio_container_devices_dirty_tracking_is_supported(bcontainer) &&
>>               bcontainer->dirty_pages_supported) {
>> @@ -691,8 +693,17 @@ static bool vfio_container_connect(VFIOGroup *group, AddressSpace *as,
>>       }
>>       group_was_added = true;
>> -    if (!vfio_listener_register(bcontainer, errp)) {
>> -        goto fail;
>> +    /*
>> +     * If reused, register the listener later, after all state that may
>> +     * affect regions and mapping boundaries has been cpr load'ed.  Later,
>> +     * the listener will invoke its callback on each flat section and call
>> +     * dma_map to supply the new vaddr, and the calls will match the mappings
>> +     * remembered by the kernel.
>> +     */
>> +    if (!cpr_reused) {
>> +        if (!vfio_listener_register(bcontainer, errp)) {
>> +            goto fail;
>> +        }
>>       }
>>       bcontainer->initialized = true;
>> diff --git a/hw/vfio/cpr-legacy.c b/hw/vfio/cpr-legacy.c
>> index 519d772..bbcf71e 100644
>> --- a/hw/vfio/cpr-legacy.c
>> +++ b/hw/vfio/cpr-legacy.c
>> @@ -11,11 +11,13 @@
>>   #include "hw/vfio/vfio-container.h"
>>   #include "hw/vfio/vfio-cpr.h"
>>   #include "hw/vfio/vfio-device.h"
>> +#include "hw/vfio/vfio-listener.h"
>>   #include "migration/blocker.h"
>>   #include "migration/cpr.h"
>>   #include "migration/migration.h"
>>   #include "migration/vmstate.h"
>>   #include "qapi/error.h"
>> +#include "qemu/error-report.h"
>>   static bool vfio_dma_unmap_vaddr_all(VFIOContainer *container, Error **errp)
>>   {
>> @@ -32,6 +34,34 @@ static bool vfio_dma_unmap_vaddr_all(VFIOContainer *container, Error **errp)
>>       return true;
>>   }
>> +/*
>> + * Set the new @vaddr for any mappings registered during cpr load.
>> + * Reused is cleared thereafter.
>> + */
>> +static int vfio_legacy_cpr_dma_map(const VFIOContainerBase *bcontainer,
>> +                                   hwaddr iova, ram_addr_t size, void *vaddr,
>> +                                   bool readonly)
>> +{
>> +    const VFIOContainer *container = container_of(bcontainer, VFIOContainer,
>> +                                                  bcontainer);
>> +    struct vfio_iommu_type1_dma_map map = {
>> +        .argsz = sizeof(map),
>> +        .flags = VFIO_DMA_MAP_FLAG_VADDR,
>> +        .vaddr = (__u64)(uintptr_t)vaddr,
>> +        .iova = iova,
>> +        .size = size,
>> +    };
>> +
>> +    assert(container->cpr.reused);
>> +> +    if (ioctl(container->fd, VFIO_IOMMU_MAP_DMA, &map)) {
>> +        error_report("vfio_legacy_cpr_dma_map (iova %lu, size %ld, va %p): %s",
>> +                     iova, size, vaddr, strerror(errno));
>> +        return -errno;
>> +    }
>> +
>> +    return 0;
>> +}
>>   static bool vfio_cpr_supported(VFIOContainer *container, Error **errp)
>>   {
>> @@ -63,12 +93,24 @@ static int vfio_container_pre_save(void *opaque)
>>   static int vfio_container_post_load(void *opaque, int version_id)
>>   {
>>       VFIOContainer *container = opaque;
>> +    VFIOContainerBase *bcontainer = &container->bcontainer;
>>       VFIOGroup *group;
>>       VFIODevice *vbasedev;
>> +    Error *err = NULL;
>> +
>> +    if (!vfio_listener_register(bcontainer, &err)) {
>> +        error_report_err(err);
>> +        return -1;
>> +    }
>>       container->cpr.reused = false;
>>       QLIST_FOREACH(group, &container->group_list, container_next) {
>> +        VFIOIOMMUClass *vioc = VFIO_IOMMU_GET_CLASS(bcontainer);
>> +
>> +        /* Restore original dma_map function */
>> +        vioc->dma_map = vfio_legacy_dma_map;
>> +
>>           QLIST_FOREACH(vbasedev, &group->device_list, next) {
>>               vbasedev->cpr.reused = false;
>>           }
>> @@ -80,6 +122,7 @@ static const VMStateDescription vfio_container_vmstate = {
>>       .name = "vfio-container",
>>       .version_id = 0,
>>       .minimum_version_id = 0,
>> +    .priority = MIG_PRI_LOW,  /* Must happen after devices and groups */
>>       .pre_save = vfio_container_pre_save,
>>       .post_load = vfio_container_post_load,
>>       .needed = cpr_needed_for_reuse,
>> @@ -104,6 +147,11 @@ bool vfio_legacy_cpr_register_container(VFIOContainer *container, Error **errp)
>>       vmstate_register(NULL, -1, &vfio_container_vmstate, container);
>> +    /* During incoming CPR, divert calls to dma_map. */
>> +    if (container->cpr.reused) {
>> +        VFIOIOMMUClass *vioc = VFIO_IOMMU_GET_CLASS(bcontainer);
>> +        vioc->dma_map = vfio_legacy_cpr_dma_map;
>> +    }
>>       return true;
>>   }
>