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 Mon, 4 Mar 2024 at 13:41, Duan, Zhenzhong
><zhenzhong.duan@intel.com> wrote:
>> 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
>
>qemu -> QEMU
>
>> 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.
>>
>
>Nice +1
>
>> >> @@ -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;
>>
>> 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.
>
>* After iommufd_device_get_info() ret is != 0. So s->cap won't be
>updated then. Hope that is intended.

Yes, iommufd_device_get_info() return ret !=0 means error happen,
we should not update s->cap definitely.

Normally if there are multiple vfio devices backed by different host IOMMUs,
It's possible s->cap updated more than once, e.g., MGAW update: 57->52->48.

>
>* With the above tweaks included:
>    Reviewed-by: Prasad Pandit <pjp@fedoraproject.org>

Thanks for your review.

BRs.
Zhenzhong