Re: [PATCH v4 03/17] iommufd: Unite all kernel-managed members into a struct

From: Yi Liu <yi.l.liu@intel.com>
To: Yan Zhao <yan.y.zhao@intel.com>
Cc: <joro@8bytes.org>, <alex.williamson@redhat.com>, <jgg@nvidia.com>,
	<kevin.tian@intel.com>, <robin.murphy@arm.com>,
	<baolu.lu@linux.intel.com>, <cohuck@redhat.com>,
	<eric.auger@redhat.com>, <nicolinc@nvidia.com>,
	<kvm@vger.kernel.org>, <mjrosato@linux.ibm.com>,
	<chao.p.peng@linux.intel.com>, <yi.y.sun@linux.intel.com>,
	<peterx@redhat.com>, <jasowang@redhat.com>,
	<shameerali.kolothum.thodi@huawei.com>, <lulu@redhat.com>,
	<suravee.suthikulpanit@amd.com>, <iommu@lists.linux.dev>,
	<linux-kernel@vger.kernel.org>, <linux-kselftest@vger.kernel.org>,
	<zhenzhong.duan@intel.com>, <joao.m.martins@oracle.com>
Subject: Re: [PATCH v4 03/17] iommufd: Unite all kernel-managed members into a struct
Date: Tue, 10 Oct 2023 11:26:37 +0800	[thread overview]
Message-ID: <1b27b31a-3eaa-0acc-b9ae-756605170220@intel.com> (raw)
In-Reply-To: <ZSOMCXBzUMRujp89@yzhao56-desk.sh.intel.com>

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