Re: [PATCH rfcv2 14/18] intel_iommu: Add a framework to check and sync host IOMMU cap/ecap
Hi Zhenzhong, On 2/26/24 08:36, Duan, Zhenzhong wrote: > >> -Original Message- >> From: Eric Auger >> 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 >>> >>> 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 >>> Signed-off-by: Yi Sun >>> Signed-off-by: Zhenzhong Duan >>> --- >>> 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
RE: [PATCH rfcv2 14/18] intel_iommu: Add a framework to check and sync host IOMMU cap/ecap
>-Original Message- >From: Eric Auger >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 >> >> 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 >> Signed-off-by: Yi Sun >> Signed-off-by: Zhenzhong Duan >> --- >> 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
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 > > 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 > Signed-off-by: Yi Sun > Signed-off-by: Zhenzhong Duan > --- > 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); > }