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

From: "Duan, Zhenzhong" <zhenzhong.duan@intel.com>
To: Prasad Pandit <ppandit@redhat.com>
Cc: "qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
	"alex.williamson@redhat.com" <alex.williamson@redhat.com>,
	"clg@redhat.com" <clg@redhat.com>,
	"eric.auger@redhat.com" <eric.auger@redhat.com>,
	"peterx@redhat.com" <peterx@redhat.com>,
	"jasowang@redhat.com" <jasowang@redhat.com>,
	"mst@redhat.com" <mst@redhat.com>,
	"jgg@nvidia.com" <jgg@nvidia.com>,
	"nicolinc@nvidia.com" <nicolinc@nvidia.com>,
	"joao.m.martins@oracle.com" <joao.m.martins@oracle.com>,
	"Tian, Kevin" <kevin.tian@intel.com>,
	"Liu, Yi L" <yi.l.liu@intel.com>,
	"Sun, Yi Y" <yi.y.sun@intel.com>,
	"Peng, Chao P" <chao.p.peng@intel.com>,
	"Marcel Apfelbaum" <marcel.apfelbaum@gmail.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	 Richard Henderson <richard.henderson@linaro.org>,
	Eduardo Habkost <eduardo@habkost.net>
Subject: RE: [PATCH v1 6/6] intel_iommu: Block migration if cap is updated
Date: Mon, 4 Mar 2024 10:10:47 +0000	[thread overview]
Message-ID: <SJ0PR11MB67445C9089A62476223F50D192232@SJ0PR11MB6744.namprd11.prod.outlook.com> (raw)
In-Reply-To: <CAE8KmOwuTpFSr=ft2cdiWKJmiy2uZMunO+v1UWFa1MbGqCXH7g@mail.gmail.com>

>-----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