| Message ID | 20240201072818.327930-15-zhenzhong.duan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Check and sync host IOMMU cap/ecap with vIOMMU | expand |
On 2/1/24 08:28, Zhenzhong Duan wrote: > From: Yi Liu <yi.l.liu@intel.com> > > Add a framework to check and synchronize host IOMMU cap/ecap with > vIOMMU cap/ecap. > > The sequence will be: > > vtd_cap_init() initializes iommu->cap/ecap. > vtd_check_hdev() update iommu->cap/ecap based on host cap/ecap. > iommu->cap_frozen set when machine create done, iommu->cap/ecap become readonly. > > Implementation details for different backends will be in following patches. > > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > include/hw/i386/intel_iommu.h | 1 + > hw/i386/intel_iommu.c | 41 ++++++++++++++++++++++++++++++++++- > 2 files changed, 41 insertions(+), 1 deletion(-) > > diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h > index bbc7b96add..c71a133820 100644 > --- a/include/hw/i386/intel_iommu.h > +++ b/include/hw/i386/intel_iommu.h > @@ -283,6 +283,7 @@ struct IntelIOMMUState { > > uint64_t cap; /* The value of capability reg */ > uint64_t ecap; /* The value of extended capability reg */ > + bool cap_frozen; /* cap/ecap become read-only after frozen */ > > uint32_t context_cache_gen; /* Should be in [1,MAX] */ > GHashTable *iotlb; /* IOTLB */ > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > index ffa1ad6429..7ed2b79669 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -3819,6 +3819,31 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, > return vtd_dev_as; > } > > +static int vtd_check_legacy_hdev(IntelIOMMUState *s, > + IOMMULegacyDevice *ldev, > + Error **errp) > +{ > + return 0; > +} > + > +static int vtd_check_iommufd_hdev(IntelIOMMUState *s, > + IOMMUFDDevice *idev, > + Error **errp) > +{ > + return 0; > +} > + > +static int vtd_check_hdev(IntelIOMMUState *s, VTDHostIOMMUDevice *vtd_hdev, > + Error **errp) > +{ > + HostIOMMUDevice *base_dev = vtd_hdev->dev; > + > + if (base_dev->type == HID_LEGACY) { > + return vtd_check_legacy_hdev(s, vtd_hdev->ldev, errp); > + } > + return vtd_check_iommufd_hdev(s, vtd_hdev->idev, errp); Couldn't we have HostIOMMUDevice ops instead of having this check here? Eric > +} > + > static int vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn, > HostIOMMUDevice *base_dev, Error **errp) > { > @@ -3829,6 +3854,7 @@ static int vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn, > .devfn = devfn, > }; > struct vtd_as_key *new_key; > + int ret; > > assert(base_dev); > > @@ -3848,6 +3874,13 @@ static int vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn, > vtd_hdev->iommu_state = s; > vtd_hdev->dev = base_dev; > > + ret = vtd_check_hdev(s, vtd_hdev, errp); > + if (ret) { > + g_free(vtd_hdev); > + vtd_iommu_unlock(s); > + return ret; > + } > + > new_key = g_malloc(sizeof(*new_key)); > new_key->bus = bus; > new_key->devfn = devfn; > @@ -4083,7 +4116,9 @@ static void vtd_init(IntelIOMMUState *s) > s->iq_dw = false; > s->next_frcd_reg = 0; > > - vtd_cap_init(s); > + if (!s->cap_frozen) { > + vtd_cap_init(s); > + } > > /* > * Rsvd field masks for spte > @@ -4254,6 +4289,10 @@ static int vtd_machine_done_notify_one(Object *child, void *unused) > > static void vtd_machine_done_hook(Notifier *notifier, void *unused) > { > + IntelIOMMUState *iommu = INTEL_IOMMU_DEVICE(x86_iommu_get_default()); > + > + iommu->cap_frozen = true; > + > object_child_foreach_recursive(object_get_root(), > vtd_machine_done_notify_one, NULL); > }
>-----Original Message----- >From: Eric Auger <eric.auger@redhat.com> >Subject: Re: [PATCH rfcv2 14/18] intel_iommu: Add a framework to check >and sync host IOMMU cap/ecap > > > >On 2/1/24 08:28, Zhenzhong Duan wrote: >> From: Yi Liu <yi.l.liu@intel.com> >> >> Add a framework to check and synchronize host IOMMU cap/ecap with >> vIOMMU cap/ecap. >> >> The sequence will be: >> >> vtd_cap_init() initializes iommu->cap/ecap. >> vtd_check_hdev() update iommu->cap/ecap based on host cap/ecap. >> iommu->cap_frozen set when machine create done, iommu->cap/ecap >become readonly. >> >> Implementation details for different backends will be in following patches. >> >> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> --- >> include/hw/i386/intel_iommu.h | 1 + >> hw/i386/intel_iommu.c | 41 >++++++++++++++++++++++++++++++++++- >> 2 files changed, 41 insertions(+), 1 deletion(-) >> >> diff --git a/include/hw/i386/intel_iommu.h >b/include/hw/i386/intel_iommu.h >> index bbc7b96add..c71a133820 100644 >> --- a/include/hw/i386/intel_iommu.h >> +++ b/include/hw/i386/intel_iommu.h >> @@ -283,6 +283,7 @@ struct IntelIOMMUState { >> >> uint64_t cap; /* The value of capability reg */ >> uint64_t ecap; /* The value of extended capability reg */ >> + bool cap_frozen; /* cap/ecap become read-only after frozen */ >> >> uint32_t context_cache_gen; /* Should be in [1,MAX] */ >> GHashTable *iotlb; /* IOTLB */ >> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c >> index ffa1ad6429..7ed2b79669 100644 >> --- a/hw/i386/intel_iommu.c >> +++ b/hw/i386/intel_iommu.c >> @@ -3819,6 +3819,31 @@ VTDAddressSpace >*vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, >> return vtd_dev_as; >> } >> >> +static int vtd_check_legacy_hdev(IntelIOMMUState *s, >> + IOMMULegacyDevice *ldev, >> + Error **errp) >> +{ >> + return 0; >> +} >> + >> +static int vtd_check_iommufd_hdev(IntelIOMMUState *s, >> + IOMMUFDDevice *idev, >> + Error **errp) >> +{ >> + return 0; >> +} >> + >> +static int vtd_check_hdev(IntelIOMMUState *s, VTDHostIOMMUDevice >*vtd_hdev, >> + Error **errp) >> +{ >> + HostIOMMUDevice *base_dev = vtd_hdev->dev; >> + >> + if (base_dev->type == HID_LEGACY) { >> + return vtd_check_legacy_hdev(s, vtd_hdev->ldev, errp); >> + } >> + return vtd_check_iommufd_hdev(s, vtd_hdev->idev, errp); >Couldn't we have HostIOMMUDevice ops instead of having this check here? Hmm, not sure if this is deserved. If we define such ops, it has only one function check_hdev and we still need to check base_dev->type to assign different function to HostIOMMUDevice.ops.check_hdev in vtd_dev_set_iommu_device(). Thanks Zhenzhong
Hi Zhenzhong, On 2/26/24 08:36, Duan, Zhenzhong wrote: > >> -----Original Message----- >> From: Eric Auger <eric.auger@redhat.com> >> Subject: Re: [PATCH rfcv2 14/18] intel_iommu: Add a framework to check >> and sync host IOMMU cap/ecap >> >> >> >> On 2/1/24 08:28, Zhenzhong Duan wrote: >>> From: Yi Liu <yi.l.liu@intel.com> >>> >>> Add a framework to check and synchronize host IOMMU cap/ecap with >>> vIOMMU cap/ecap. >>> >>> The sequence will be: >>> >>> vtd_cap_init() initializes iommu->cap/ecap. >>> vtd_check_hdev() update iommu->cap/ecap based on host cap/ecap. >>> iommu->cap_frozen set when machine create done, iommu->cap/ecap >> become readonly. >>> Implementation details for different backends will be in following patches. >>> >>> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >>> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> >>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>> --- >>> include/hw/i386/intel_iommu.h | 1 + >>> hw/i386/intel_iommu.c | 41 >> ++++++++++++++++++++++++++++++++++- >>> 2 files changed, 41 insertions(+), 1 deletion(-) >>> >>> diff --git a/include/hw/i386/intel_iommu.h >> b/include/hw/i386/intel_iommu.h >>> index bbc7b96add..c71a133820 100644 >>> --- a/include/hw/i386/intel_iommu.h >>> +++ b/include/hw/i386/intel_iommu.h >>> @@ -283,6 +283,7 @@ struct IntelIOMMUState { >>> >>> uint64_t cap; /* The value of capability reg */ >>> uint64_t ecap; /* The value of extended capability reg */ >>> + bool cap_frozen; /* cap/ecap become read-only after frozen */ >>> >>> uint32_t context_cache_gen; /* Should be in [1,MAX] */ >>> GHashTable *iotlb; /* IOTLB */ >>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c >>> index ffa1ad6429..7ed2b79669 100644 >>> --- a/hw/i386/intel_iommu.c >>> +++ b/hw/i386/intel_iommu.c >>> @@ -3819,6 +3819,31 @@ VTDAddressSpace >> *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, >>> return vtd_dev_as; >>> } >>> >>> +static int vtd_check_legacy_hdev(IntelIOMMUState *s, >>> + IOMMULegacyDevice *ldev, >>> + Error **errp) >>> +{ >>> + return 0; >>> +} >>> + >>> +static int vtd_check_iommufd_hdev(IntelIOMMUState *s, >>> + IOMMUFDDevice *idev, >>> + Error **errp) >>> +{ >>> + return 0; >>> +} >>> + >>> +static int vtd_check_hdev(IntelIOMMUState *s, VTDHostIOMMUDevice >> *vtd_hdev, >>> + Error **errp) >>> +{ >>> + HostIOMMUDevice *base_dev = vtd_hdev->dev; >>> + >>> + if (base_dev->type == HID_LEGACY) { >>> + return vtd_check_legacy_hdev(s, vtd_hdev->ldev, errp); >>> + } >>> + return vtd_check_iommufd_hdev(s, vtd_hdev->idev, errp); >> Couldn't we have HostIOMMUDevice ops instead of having this check here? > Hmm, not sure if this is deserved. If we define such ops, it has only one function > check_hdev and we still need to check base_dev->type to assign different > function to HostIOMMUDevice.ops.check_hdev in vtd_dev_set_iommu_device(). OK maybe this is over engineered. Drop that comment for now Thanks Eric > > Thanks > Zhenzhong
diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h index bbc7b96add..c71a133820 100644 --- a/include/hw/i386/intel_iommu.h +++ b/include/hw/i386/intel_iommu.h @@ -283,6 +283,7 @@ struct IntelIOMMUState { uint64_t cap; /* The value of capability reg */ uint64_t ecap; /* The value of extended capability reg */ + bool cap_frozen; /* cap/ecap become read-only after frozen */ uint32_t context_cache_gen; /* Should be in [1,MAX] */ GHashTable *iotlb; /* IOTLB */ diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index ffa1ad6429..7ed2b79669 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -3819,6 +3819,31 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, return vtd_dev_as; } +static int vtd_check_legacy_hdev(IntelIOMMUState *s, + IOMMULegacyDevice *ldev, + Error **errp) +{ + return 0; +} + +static int vtd_check_iommufd_hdev(IntelIOMMUState *s, + IOMMUFDDevice *idev, + Error **errp) +{ + return 0; +} + +static int vtd_check_hdev(IntelIOMMUState *s, VTDHostIOMMUDevice *vtd_hdev, + Error **errp) +{ + HostIOMMUDevice *base_dev = vtd_hdev->dev; + + if (base_dev->type == HID_LEGACY) { + return vtd_check_legacy_hdev(s, vtd_hdev->ldev, errp); + } + return vtd_check_iommufd_hdev(s, vtd_hdev->idev, errp); +} + static int vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn, HostIOMMUDevice *base_dev, Error **errp) { @@ -3829,6 +3854,7 @@ static int vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn, .devfn = devfn, }; struct vtd_as_key *new_key; + int ret; assert(base_dev); @@ -3848,6 +3874,13 @@ static int vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn, vtd_hdev->iommu_state = s; vtd_hdev->dev = base_dev; + ret = vtd_check_hdev(s, vtd_hdev, errp); + if (ret) { + g_free(vtd_hdev); + vtd_iommu_unlock(s); + return ret; + } + new_key = g_malloc(sizeof(*new_key)); new_key->bus = bus; new_key->devfn = devfn; @@ -4083,7 +4116,9 @@ static void vtd_init(IntelIOMMUState *s) s->iq_dw = false; s->next_frcd_reg = 0; - vtd_cap_init(s); + if (!s->cap_frozen) { + vtd_cap_init(s); + } /* * Rsvd field masks for spte @@ -4254,6 +4289,10 @@ static int vtd_machine_done_notify_one(Object *child, void *unused) static void vtd_machine_done_hook(Notifier *notifier, void *unused) { + IntelIOMMUState *iommu = INTEL_IOMMU_DEVICE(x86_iommu_get_default()); + + iommu->cap_frozen = true; + object_child_foreach_recursive(object_get_root(), vtd_machine_done_notify_one, NULL); }