| Message ID | 20240228094432.1092748-7-zhenzhong.duan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Check and sync host IOMMU cap/ecap with vIOMMU | expand |
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 ... > IOMMU cap/ecap, migration should be blocked. * It'll help to mention why migration should be blocked 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? Thank you. --- - Prasad
>-----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
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. * With the above tweaks included: Reviewed-by: Prasad Pandit <pjp@fedoraproject.org> Thank you. --- - Prasad
>-----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
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index e474284e43..9ca47dbf9a 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -40,6 +40,7 @@ #include "hw/i386/apic_internal.h" #include "kvm/kvm_i386.h" #include "migration/vmstate.h" +#include "migration/blocker.h" #include "trace.h" #define S_AW_BITS (VTD_MGAW_FROM_CAP(s->cap) + 1) @@ -3830,6 +3831,8 @@ static int vtd_check_legacy_hdev(IntelIOMMUState *s, return 0; } +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; } static int vtd_check_hdev(IntelIOMMUState *s, VTDHostIOMMUDevice *vtd_hdev,
When there is VFIO device and vIOMMU cap/ecap is updated based on host IOMMU cap/ecap, migration should be blocked. Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> --- hw/i386/intel_iommu.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)