| Message ID | 20230921075138.124099-4-yi.l.liu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommufd: Add nesting infrastructure | expand |
> From: Liu, Yi L <yi.l.liu@intel.com> > Sent: Thursday, September 21, 2023 3:51 PM > > From: Nicolin Chen <nicolinc@nvidia.com> > > The struct iommufd_hw_pagetable has been representing a kernel-managed > HWPT, yet soon will be reused to represent a user-managed HWPT. These > two types of HWPTs has the same IOMMUFD object type and an > iommu_domain > object, but have quite different attributes/members. > > Add a union in struct iommufd_hw_pagetable and group all the existing > kernel-managed members. One of the following patches will add another > struct for user-managed members. > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > Signed-off-by: Yi Liu <yi.l.liu@intel.com> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
On Thu, Sep 21, 2023 at 12:51:24AM -0700, Yi Liu wrote: > From: Nicolin Chen <nicolinc@nvidia.com> > > The struct iommufd_hw_pagetable has been representing a kernel-managed > HWPT, yet soon will be reused to represent a user-managed HWPT. These > two types of HWPTs has the same IOMMUFD object type and an iommu_domain > object, but have quite different attributes/members. > > Add a union in struct iommufd_hw_pagetable and group all the existing > kernel-managed members. One of the following patches will add another > struct for user-managed members. > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > --- > drivers/iommu/iommufd/iommufd_private.h | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h > index 3064997a0181..947a797536e3 100644 > --- a/drivers/iommu/iommufd/iommufd_private.h > +++ b/drivers/iommu/iommufd/iommufd_private.h > @@ -231,13 +231,18 @@ int iommufd_vfio_ioas(struct iommufd_ucmd *ucmd); > */ > struct iommufd_hw_pagetable { > struct iommufd_object obj; > - struct iommufd_ioas *ioas; > struct iommu_domain *domain; > - bool auto_domain : 1; > - bool enforce_cache_coherency : 1; > - bool msi_cookie : 1; > - /* Head at iommufd_ioas::hwpt_list */ > - struct list_head hwpt_item; > + > + union { > + struct { /* kernel-managed */ > + struct iommufd_ioas *ioas; > + bool auto_domain : 1; Will iommufd_hw_pagetable_put() also be called on non-kernel-managed domain? If yes, hwpt->user_managed needs to be checked in iommufd_hw_pagetable_put(), (e.g. as below). Otherwise, this union will lead to hwpt->ioas and hwpt->auto_domain still being accessible though invalid. static inline void iommufd_hw_pagetable_put(struct iommufd_ctx *ictx, struct iommufd_hw_pagetable *hwpt) { - lockdep_assert_not_held(&hwpt->ioas->mutex); - if (hwpt->auto_domain) + if (!hwpt->user_managed) + lockdep_assert_not_held(&hwpt->ioas->mutex); + + if (!hwpt->user_managed && hwpt->auto_domain) iommufd_object_deref_user(ictx, &hwpt->obj); else refcount_dec(&hwpt->obj.users); } > + bool enforce_cache_coherency : 1; > + bool msi_cookie : 1; > + /* Head at iommufd_ioas::hwpt_list */ > + struct list_head hwpt_item; > + }; > + }; > }; > > struct iommufd_hw_pagetable * > -- > 2.34.1 >
On 2023/10/7 18:08, Yan Zhao wrote: > On Thu, Sep 21, 2023 at 12:51:24AM -0700, Yi Liu wrote: >> From: Nicolin Chen <nicolinc@nvidia.com> >> >> The struct iommufd_hw_pagetable has been representing a kernel-managed >> HWPT, yet soon will be reused to represent a user-managed HWPT. These >> two types of HWPTs has the same IOMMUFD object type and an iommu_domain >> object, but have quite different attributes/members. >> >> Add a union in struct iommufd_hw_pagetable and group all the existing >> kernel-managed members. One of the following patches will add another >> struct for user-managed members. >> >> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> >> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >> --- >> drivers/iommu/iommufd/iommufd_private.h | 17 +++++++++++------ >> 1 file changed, 11 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h >> index 3064997a0181..947a797536e3 100644 >> --- a/drivers/iommu/iommufd/iommufd_private.h >> +++ b/drivers/iommu/iommufd/iommufd_private.h >> @@ -231,13 +231,18 @@ int iommufd_vfio_ioas(struct iommufd_ucmd *ucmd); >> */ >> struct iommufd_hw_pagetable { >> struct iommufd_object obj; >> - struct iommufd_ioas *ioas; >> struct iommu_domain *domain; >> - bool auto_domain : 1; >> - bool enforce_cache_coherency : 1; >> - bool msi_cookie : 1; >> - /* Head at iommufd_ioas::hwpt_list */ >> - struct list_head hwpt_item; >> + >> + union { >> + struct { /* kernel-managed */ >> + struct iommufd_ioas *ioas; >> + bool auto_domain : 1; > Will iommufd_hw_pagetable_put() also be called on non-kernel-managed domain? yes. > If yes, hwpt->user_managed needs to be checked in iommufd_hw_pagetable_put(), > (e.g. as below). > Otherwise, this union will lead to hwpt->ioas and hwpt->auto_domain still being > accessible though invalid. not quite get this sentence. > > static inline void iommufd_hw_pagetable_put(struct iommufd_ctx *ictx, > struct iommufd_hw_pagetable *hwpt) > { > - lockdep_assert_not_held(&hwpt->ioas->mutex); > - if (hwpt->auto_domain) > + if (!hwpt->user_managed) > + lockdep_assert_not_held(&hwpt->ioas->mutex); this is true. this assert is not needed when hwpt is not kernel managed domain. > + > + if (!hwpt->user_managed && hwpt->auto_domain) actually, checking auto_domain is more precise. There is hwpt which is neither user managed nor auto. > iommufd_object_deref_user(ictx, &hwpt->obj); > else > refcount_dec(&hwpt->obj.users); > } > >> + bool enforce_cache_coherency : 1; >> + bool msi_cookie : 1; >> + /* Head at iommufd_ioas::hwpt_list */ >> + struct list_head hwpt_item; >> + }; >> + }; >> }; >> >> struct iommufd_hw_pagetable * >> -- >> 2.34.1 >>
On Mon, Oct 09, 2023 at 12:13:52PM +0800, Yi Liu wrote: > On 2023/10/7 18:08, Yan Zhao wrote: > > On Thu, Sep 21, 2023 at 12:51:24AM -0700, Yi Liu wrote: > > > From: Nicolin Chen <nicolinc@nvidia.com> > > > > > > The struct iommufd_hw_pagetable has been representing a kernel-managed > > > HWPT, yet soon will be reused to represent a user-managed HWPT. These > > > two types of HWPTs has the same IOMMUFD object type and an iommu_domain > > > object, but have quite different attributes/members. > > > > > > Add a union in struct iommufd_hw_pagetable and group all the existing > > > kernel-managed members. One of the following patches will add another > > > struct for user-managed members. > > > > > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > > > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > > > --- > > > drivers/iommu/iommufd/iommufd_private.h | 17 +++++++++++------ > > > 1 file changed, 11 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h > > > index 3064997a0181..947a797536e3 100644 > > > --- a/drivers/iommu/iommufd/iommufd_private.h > > > +++ b/drivers/iommu/iommufd/iommufd_private.h > > > @@ -231,13 +231,18 @@ int iommufd_vfio_ioas(struct iommufd_ucmd *ucmd); > > > */ > > > struct iommufd_hw_pagetable { > > > struct iommufd_object obj; > > > - struct iommufd_ioas *ioas; > > > struct iommu_domain *domain; > > > - bool auto_domain : 1; > > > - bool enforce_cache_coherency : 1; > > > - bool msi_cookie : 1; > > > - /* Head at iommufd_ioas::hwpt_list */ > > > - struct list_head hwpt_item; > > > + > > > + union { > > > + struct { /* kernel-managed */ > > > + struct iommufd_ioas *ioas; > > > + bool auto_domain : 1; > > Will iommufd_hw_pagetable_put() also be called on non-kernel-managed domain? > > yes. > > > If yes, hwpt->user_managed needs to be checked in iommufd_hw_pagetable_put(), > > (e.g. as below). > > Otherwise, this union will lead to hwpt->ioas and hwpt->auto_domain still being > > accessible though invalid. > > not quite get this sentence. I mean with this union, hwpt->auto_domain or hwpt->ioas should only be accessed if and only if hwpt type is kernel-managed. So, any unconditional access, as in iommufd_hw_pagetable_put() pasted below, is buggy. Therefore, do you think it's better to add a filed like "bool kernel_managed : 1", and access the union fields under /* kernel-managed */ only when hwpt->kernel_managed is true. > > > > > static inline void iommufd_hw_pagetable_put(struct iommufd_ctx *ictx, > > struct iommufd_hw_pagetable *hwpt) > > { > > - lockdep_assert_not_held(&hwpt->ioas->mutex); > > - if (hwpt->auto_domain) > > + if (!hwpt->user_managed) > > + lockdep_assert_not_held(&hwpt->ioas->mutex); > > this is true. this assert is not needed when hwpt is not kernel managed domain. > > > + > > + if (!hwpt->user_managed && hwpt->auto_domain) > > actually, checking auto_domain is more precise. There is hwpt which is > neither user managed nor auto. auto_domain is under union fields marked with kernel-managed only. Access it without type checking is invalid. struct iommufd_hw_pagetable { struct iommufd_object obj; struct iommu_domain *domain; void (*abort)(struct iommufd_object *obj); void (*destroy)(struct iommufd_object *obj); bool user_managed : 1; union { struct { /* user-managed */ struct iommufd_hw_pagetable *parent; }; struct { /* kernel-managed */ struct iommufd_ioas *ioas; struct mutex mutex; bool auto_domain : 1; bool enforce_cache_coherency : 1; bool msi_cookie : 1; bool nest_parent : 1; /* Head at iommufd_ioas::hwpt_list */ struct list_head hwpt_item; }; }; }; > > > iommufd_object_deref_user(ictx, &hwpt->obj); > > else > > refcount_dec(&hwpt->obj.users); > > } > > > > > + bool enforce_cache_coherency : 1; > > > + bool msi_cookie : 1; > > > + /* Head at iommufd_ioas::hwpt_list */ > > > + struct list_head hwpt_item; > > > + }; > > > + }; > > > }; > > > struct iommufd_hw_pagetable * > > > -- > > > 2.34.1 > > > > > -- > Regards, > Yi Liu
On 2023/10/9 13:13, Yan Zhao wrote: > On Mon, Oct 09, 2023 at 12:13:52PM +0800, Yi Liu wrote: >> On 2023/10/7 18:08, Yan Zhao wrote: >>> On Thu, Sep 21, 2023 at 12:51:24AM -0700, Yi Liu wrote: >>>> From: Nicolin Chen <nicolinc@nvidia.com> >>>> >>>> The struct iommufd_hw_pagetable has been representing a kernel-managed >>>> HWPT, yet soon will be reused to represent a user-managed HWPT. These >>>> two types of HWPTs has the same IOMMUFD object type and an iommu_domain >>>> object, but have quite different attributes/members. >>>> >>>> Add a union in struct iommufd_hw_pagetable and group all the existing >>>> kernel-managed members. One of the following patches will add another >>>> struct for user-managed members. >>>> >>>> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> >>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >>>> --- >>>> drivers/iommu/iommufd/iommufd_private.h | 17 +++++++++++------ >>>> 1 file changed, 11 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h >>>> index 3064997a0181..947a797536e3 100644 >>>> --- a/drivers/iommu/iommufd/iommufd_private.h >>>> +++ b/drivers/iommu/iommufd/iommufd_private.h >>>> @@ -231,13 +231,18 @@ int iommufd_vfio_ioas(struct iommufd_ucmd *ucmd); >>>> */ >>>> struct iommufd_hw_pagetable { >>>> struct iommufd_object obj; >>>> - struct iommufd_ioas *ioas; >>>> struct iommu_domain *domain; >>>> - bool auto_domain : 1; >>>> - bool enforce_cache_coherency : 1; >>>> - bool msi_cookie : 1; >>>> - /* Head at iommufd_ioas::hwpt_list */ >>>> - struct list_head hwpt_item; >>>> + >>>> + union { >>>> + struct { /* kernel-managed */ >>>> + struct iommufd_ioas *ioas; >>>> + bool auto_domain : 1; >>> Will iommufd_hw_pagetable_put() also be called on non-kernel-managed domain? >> >> yes. >> >>> If yes, hwpt->user_managed needs to be checked in iommufd_hw_pagetable_put(), >>> (e.g. as below). >>> Otherwise, this union will lead to hwpt->ioas and hwpt->auto_domain still being >>> accessible though invalid. >> >> not quite get this sentence. > I mean with this union, hwpt->auto_domain or hwpt->ioas should only be accessed if and > only if hwpt type is kernel-managed. ok, I get this part. just not sure about the missing words in your prior comment. > So, any unconditional access, as in iommufd_hw_pagetable_put() pasted below, is buggy. > > Therefore, do you think it's better to add a filed like "bool kernel_managed : 1", > and access the union fields under /* kernel-managed */ only when hwpt->kernel_managed > is true. > > >> >>> >>> static inline void iommufd_hw_pagetable_put(struct iommufd_ctx *ictx, >>> struct iommufd_hw_pagetable *hwpt) >>> { >>> - lockdep_assert_not_held(&hwpt->ioas->mutex); >>> - if (hwpt->auto_domain) >>> + if (!hwpt->user_managed) >>> + lockdep_assert_not_held(&hwpt->ioas->mutex); >> >> this is true. this assert is not needed when hwpt is not kernel managed domain. >> >>> + >>> + if (!hwpt->user_managed && hwpt->auto_domain) >> >> actually, checking auto_domain is more precise. There is hwpt which is >> neither user managed nor auto. > > auto_domain is under union fields marked with kernel-managed only. > Access it without type checking is invalid. I see. yes, should check user_managed as well. :) > struct iommufd_hw_pagetable { > struct iommufd_object obj; > struct iommu_domain *domain; > > void (*abort)(struct iommufd_object *obj); > void (*destroy)(struct iommufd_object *obj); > > bool user_managed : 1; > union { > struct { /* user-managed */ > struct iommufd_hw_pagetable *parent; > }; > struct { /* kernel-managed */ > struct iommufd_ioas *ioas; > struct mutex mutex; > bool auto_domain : 1; > bool enforce_cache_coherency : 1; > bool msi_cookie : 1; > bool nest_parent : 1; > /* Head at iommufd_ioas::hwpt_list */ > struct list_head hwpt_item; > }; > }; > }; > >> >>> iommufd_object_deref_user(ictx, &hwpt->obj); >>> else >>> refcount_dec(&hwpt->obj.users); >>> } >>> >>>> + bool enforce_cache_coherency : 1; >>>> + bool msi_cookie : 1; >>>> + /* Head at iommufd_ioas::hwpt_list */ >>>> + struct list_head hwpt_item; >>>> + }; >>>> + }; >>>> }; >>>> struct iommufd_hw_pagetable * >>>> -- >>>> 2.34.1 >>>> >> >> -- >> Regards, >> Yi Liu
diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h index 3064997a0181..947a797536e3 100644 --- a/drivers/iommu/iommufd/iommufd_private.h +++ b/drivers/iommu/iommufd/iommufd_private.h @@ -231,13 +231,18 @@ int iommufd_vfio_ioas(struct iommufd_ucmd *ucmd); */ struct iommufd_hw_pagetable { struct iommufd_object obj; - struct iommufd_ioas *ioas; struct iommu_domain *domain; - bool auto_domain : 1; - bool enforce_cache_coherency : 1; - bool msi_cookie : 1; - /* Head at iommufd_ioas::hwpt_list */ - struct list_head hwpt_item; + + union { + struct { /* kernel-managed */ + struct iommufd_ioas *ioas; + bool auto_domain : 1; + bool enforce_cache_coherency : 1; + bool msi_cookie : 1; + /* Head at iommufd_ioas::hwpt_list */ + struct list_head hwpt_item; + }; + }; }; struct iommufd_hw_pagetable *