RE: [PATCH v1 6/6] intel_iommu: Block migration if cap is updated

[Duan, Zhenzhong]

>-----Original Message-----
>From: Prasad Pandit <ppandit@redhat.com>
>Subject: Re: [PATCH v1 6/6] intel_iommu: Block migration if cap is updated
>
>On Wed, 28 Feb 2024 at 15:17, Zhenzhong Duan
><zhenzhong.duan@intel.com> wrote:
>> When there is VFIO device and vIOMMU cap/ecap is updated based on
>host
>
>* cap/ecap -> capability/extended capability registers are updated ...

Will do.

>
>> IOMMU cap/ecap, migration should be blocked.
>
>* It'll help to mention why migration should be blocked in this case?

Refine the patch description as below:

This is to deal with a special case when cold plugged vfio device is unplugged
at runtime, then migration triggers.

When qemu on source side starts with cold plugged vfio device, vIOMMU
capability/extended capability registers(cap/ecap) can be updated based
on host cap/ecap, then vIOMMU cap/ecap is frozen after machine creation
done, vIOMMU cap/ecap isn’t restored to default after vfio device is unplugged.
In this case source and dest(default cap/ecap) will have incompatible cap/ecap
value. So migration is blocked if there is cap/ecap update on source side.

If vfio device isn't unplugged at runtime, vfio device's own vIOMMU migration 
blocker
is redundant with blocker here, but that's harmless.

If vfio devices are all hot plugged after qemu on source side starts, then 
vIOMMU
cap/ecap is frozen with the default value, we don't make a blocker in this case.

>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>  hw/i386/intel_iommu.c | 16 ++++++++++++++--
>> +static Error *vtd_mig_blocker;
>> +
>>  static int vtd_check_iommufd_hdev(IntelIOMMUState *s,
>>                                    IOMMUFDDevice *idev,
>>                                    Error **errp)
>> @@ -3861,8 +3864,17 @@ static int
>vtd_check_iommufd_hdev(IntelIOMMUState *s,
>>          tmp_cap |= VTD_CAP_MGAW(host_mgaw + 1);
>>      }
>>
>> -    s->cap = tmp_cap;
>> -    return 0;
>> +    if (s->cap != tmp_cap) {
>> +        if (vtd_mig_blocker == NULL) {
>> +            error_setg(&vtd_mig_blocker,
>> +                       "cap/ecap update from host IOMMU block migration");
>> +            ret = migrate_add_blocker(&vtd_mig_blocker, errp);
>> +        }
>> +        if (!ret) {
>> +            s->cap = tmp_cap;
>> +        }
>> +    }
>> +    return ret;
>
>* I couldn't find vtd_check_* function in the tree, but what happens
>if vtd_mig_blocker != NULL? What will be 'ret' then?

vtd_mig_blocker != NULL means cap is already updated once at least.
In this case, ret is return value 0 from iommufd_device_get_info().

    ret = iommufd_device_get_info(idev, &type, sizeof(vtd), &vtd, errp);
    if (ret) {
        return ret;
    }

Then cap is updated with host cap value tmp_cap. This update only happen
before machine creation done.

Vtd_check_* is in patch3 and patch4:
https://lists.gnu.org/archive/html/qemu-devel/2024-02/msg06418.html
https://lists.gnu.org/archive/html/qemu-devel/2024-02/msg06419.html

Thanks
Zhenzhong