Messages in this thread | | | Date | Tue, 10 Oct 2023 11:26:37 +0800 | Subject | Re: [PATCH v4 03/17] iommufd: Unite all kernel-managed members into a struct | From | 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
-- Regards, Yi Liu
| |