diff mbox series

[05/12] iommufd/hw_pagetable: Do not populate user-managed hw_pagetables

Message ID 20230309080910.607396-6-yi.l.liu@intel.com
State New
Headers
Series iommufd: Add nesting infrastructure |

Commit Message

Yi Liu March 9, 2023, 8:09 a.m. UTC 
  From: Nicolin Chen <nicolinc@nvidia.com>

A user-managed hw_pagetable does not need to get populated, since it is
managed by a guest OS. Move the iopt_table_add_domain and list_add_tail
calls into a helper, where the hwpt pointer will be redirected to its
hwpt->parent if it's available.

Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/iommufd/hw_pagetable.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

Comments

Baolu Lu March 10, 2023, 2:25 a.m. UTC | #1 
On 3/9/23 4:09 PM, Yi Liu wrote:
> From: Nicolin Chen <nicolinc@nvidia.com>
> 
> A user-managed hw_pagetable does not need to get populated, since it is
> managed by a guest OS. Move the iopt_table_add_domain and list_add_tail
> calls into a helper, where the hwpt pointer will be redirected to its
> hwpt->parent if it's available.
> 
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>   drivers/iommu/iommufd/hw_pagetable.c | 20 ++++++++++++++++++--
>   1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c
> index 16e92a1c150b..6e45ec0a66fa 100644
> --- a/drivers/iommu/iommufd/hw_pagetable.c
> +++ b/drivers/iommu/iommufd/hw_pagetable.c
> @@ -43,6 +43,23 @@ int iommufd_hw_pagetable_enforce_cc(struct iommufd_hw_pagetable *hwpt)
>   	return 0;
>   }
>   
> +static int iommufd_hw_pagetable_link_ioas(struct iommufd_hw_pagetable *hwpt)
> +{
> +	int rc;
> +
> +	if (hwpt->parent)
> +		hwpt = hwpt->parent;
> +
> +	if (!list_empty(&hwpt->hwpt_item))
> +		return 0;

What is above check for? Is it "the hwpt has already been inserted into
the hwpt list of its ioas in another place"?

If so, is it possible that hwpt will be deleted from the list even when
this user hwpt is still linked to the ioas?

> +
> +	rc = iopt_table_add_domain(&hwpt->ioas->iopt, hwpt->domain);
> +	if (rc)
> +		return rc;
> +	list_add_tail(&hwpt->hwpt_item, &hwpt->ioas->hwpt_list);
> +	return 0;
> +}
> +
>   /**
>    * iommufd_hw_pagetable_alloc() - Get an iommu_domain for a device
>    * @ictx: iommufd context
> @@ -131,10 +148,9 @@ iommufd_hw_pagetable_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas,
>   			goto out_unlock;
>   	}
>   
> -	rc = iopt_table_add_domain(&hwpt->ioas->iopt, hwpt->domain);
> +	rc = iommufd_hw_pagetable_link_ioas(hwpt);
>   	if (rc)
>   		goto out_detach;
> -	list_add_tail(&hwpt->hwpt_item, &hwpt->ioas->hwpt_list);
>   
>   	mutex_unlock(&idev->igroup->lock);
>   	return hwpt;

Best regards,
baolu
Nicolin Chen March 10, 2023, 6:50 a.m. UTC | #2 
On Fri, Mar 10, 2023 at 10:25:10AM +0800, Baolu Lu wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 3/9/23 4:09 PM, Yi Liu wrote:
> > From: Nicolin Chen <nicolinc@nvidia.com>
> > 
> > A user-managed hw_pagetable does not need to get populated, since it is
> > managed by a guest OS. Move the iopt_table_add_domain and list_add_tail
> > calls into a helper, where the hwpt pointer will be redirected to its
> > hwpt->parent if it's available.
> > 
> > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> > Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> > ---
> >   drivers/iommu/iommufd/hw_pagetable.c | 20 ++++++++++++++++++--
> >   1 file changed, 18 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c
> > index 16e92a1c150b..6e45ec0a66fa 100644
> > --- a/drivers/iommu/iommufd/hw_pagetable.c
> > +++ b/drivers/iommu/iommufd/hw_pagetable.c
> > @@ -43,6 +43,23 @@ int iommufd_hw_pagetable_enforce_cc(struct iommufd_hw_pagetable *hwpt)
> >       return 0;
> >   }
> > 
> > +static int iommufd_hw_pagetable_link_ioas(struct iommufd_hw_pagetable *hwpt)
> > +{
> > +     int rc;
> > +
> > +     if (hwpt->parent)
> > +             hwpt = hwpt->parent;
> > +
> > +     if (!list_empty(&hwpt->hwpt_item))
> > +             return 0;
> 
> What is above check for? Is it "the hwpt has already been inserted into
> the hwpt list of its ioas in another place"?
> 
> If so, is it possible that hwpt will be deleted from the list even when
> this user hwpt is still linked to the ioas?

It means that the hwpt is already linked to the ioas. And the
hwpt_item can be only empty after a destroy().

With that being said, after I think it through, perhaps Yi's
previous change removing it might be better. So, it could be:

-------------------------------------------------------------------------------
+	/*
+	 * Only a parent hwpt needs to be linked to the IOAS. And a hwpt->parent
+	 * must be linked to the IOAS already, when it's being allocated.
+	 */
 	if (hwpt->parent)
-		hwpt = hwpt->parent;
-
-	if (!list_empty(&hwpt->hwpt_item))
 		return 0;
 
-------------------------------------------------------------------------------

I was concerned about the case where a device gets attached to
the nested hwpt without staging at the parent hwpt first. But,
the link between the parent hwpt and the IOAS happened inside
the allocation function now, not attach() any more.

Thanks
Nic
Baolu Lu March 10, 2023, 12:51 p.m. UTC | #3 
On 2023/3/10 14:50, Nicolin Chen wrote:
> On Fri, Mar 10, 2023 at 10:25:10AM +0800, Baolu Lu wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On 3/9/23 4:09 PM, Yi Liu wrote:
>>> From: Nicolin Chen <nicolinc@nvidia.com>
>>>
>>> A user-managed hw_pagetable does not need to get populated, since it is
>>> managed by a guest OS. Move the iopt_table_add_domain and list_add_tail
>>> calls into a helper, where the hwpt pointer will be redirected to its
>>> hwpt->parent if it's available.
>>>
>>> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>>> ---
>>>    drivers/iommu/iommufd/hw_pagetable.c | 20 ++++++++++++++++++--
>>>    1 file changed, 18 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c
>>> index 16e92a1c150b..6e45ec0a66fa 100644
>>> --- a/drivers/iommu/iommufd/hw_pagetable.c
>>> +++ b/drivers/iommu/iommufd/hw_pagetable.c
>>> @@ -43,6 +43,23 @@ int iommufd_hw_pagetable_enforce_cc(struct iommufd_hw_pagetable *hwpt)
>>>        return 0;
>>>    }
>>>
>>> +static int iommufd_hw_pagetable_link_ioas(struct iommufd_hw_pagetable *hwpt)
>>> +{
>>> +     int rc;
>>> +
>>> +     if (hwpt->parent)
>>> +             hwpt = hwpt->parent;
>>> +
>>> +     if (!list_empty(&hwpt->hwpt_item))
>>> +             return 0;
>>
>> What is above check for? Is it "the hwpt has already been inserted into
>> the hwpt list of its ioas in another place"?
>>
>> If so, is it possible that hwpt will be deleted from the list even when
>> this user hwpt is still linked to the ioas?
> 
> It means that the hwpt is already linked to the ioas. And the
> hwpt_item can be only empty after a destroy().
> 
> With that being said, after I think it through, perhaps Yi's
> previous change removing it might be better. So, it could be:
> 
> -------------------------------------------------------------------------------
> +	/*
> +	 * Only a parent hwpt needs to be linked to the IOAS. And a hwpt->parent
> +	 * must be linked to the IOAS already, when it's being allocated.
> +	 */
>   	if (hwpt->parent)
> -		hwpt = hwpt->parent;
> -
> -	if (!list_empty(&hwpt->hwpt_item))
>   		return 0;
>   
> -------------------------------------------------------------------------------
> 
> I was concerned about the case where a device gets attached to
> the nested hwpt without staging at the parent hwpt first. But,
> the link between the parent hwpt and the IOAS happened inside
> the allocation function now, not attach() any more.

Yes, it's clearer.

Best regards,
baolu
Jason Gunthorpe March 10, 2023, 3:29 p.m. UTC | #4 
On Thu, Mar 09, 2023 at 12:09:03AM -0800, Yi Liu wrote:
> From: Nicolin Chen <nicolinc@nvidia.com>
> 
> A user-managed hw_pagetable does not need to get populated, since it is
> managed by a guest OS. Move the iopt_table_add_domain and list_add_tail
> calls into a helper, where the hwpt pointer will be redirected to its
> hwpt->parent if it's available.
> 
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/iommu/iommufd/hw_pagetable.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c
> index 16e92a1c150b..6e45ec0a66fa 100644
> --- a/drivers/iommu/iommufd/hw_pagetable.c
> +++ b/drivers/iommu/iommufd/hw_pagetable.c
> @@ -43,6 +43,23 @@ int iommufd_hw_pagetable_enforce_cc(struct iommufd_hw_pagetable *hwpt)
>  	return 0;
>  }
>  
> +static int iommufd_hw_pagetable_link_ioas(struct iommufd_hw_pagetable *hwpt)
> +{
> +	int rc;
> +
> +	if (hwpt->parent)

This should be:

   hwpt->domain->type != IOMMU_DOMAIN_UNMANAGED

Ie if we asked the driver to alloc a domain and it allocated an
UNMANAGED domain then it means IOMMUFD manages the mappings and it
should be populated from the IOAS.

Arguably drivers should EOPNOTSUPP if presented with a parent in this
situation, but still this code should be clear about the purpose.

> +		hwpt = hwpt->parent;

And we definately shouldn't touch the parent. That is already setup
and owned by someone else. Just return and don't do anything.

Jason
Nicolin Chen March 10, 2023, 11:31 p.m. UTC | #5 
On Fri, Mar 10, 2023 at 11:29:14AM -0400, Jason Gunthorpe wrote:
> On Thu, Mar 09, 2023 at 12:09:03AM -0800, Yi Liu wrote:
> > From: Nicolin Chen <nicolinc@nvidia.com>
> > 
> > A user-managed hw_pagetable does not need to get populated, since it is
> > managed by a guest OS. Move the iopt_table_add_domain and list_add_tail
> > calls into a helper, where the hwpt pointer will be redirected to its
> > hwpt->parent if it's available.
> > 
> > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> > Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> > ---
> >  drivers/iommu/iommufd/hw_pagetable.c | 20 ++++++++++++++++++--
> >  1 file changed, 18 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c
> > index 16e92a1c150b..6e45ec0a66fa 100644
> > --- a/drivers/iommu/iommufd/hw_pagetable.c
> > +++ b/drivers/iommu/iommufd/hw_pagetable.c
> > @@ -43,6 +43,23 @@ int iommufd_hw_pagetable_enforce_cc(struct iommufd_hw_pagetable *hwpt)
> >  	return 0;
> >  }
> >  
> > +static int iommufd_hw_pagetable_link_ioas(struct iommufd_hw_pagetable *hwpt)
> > +{
> > +	int rc;
> > +
> > +	if (hwpt->parent)
> 
> This should be:
> 
>    hwpt->domain->type != IOMMU_DOMAIN_UNMANAGED
> 
> Ie if we asked the driver to alloc a domain and it allocated an
> UNMANAGED domain then it means IOMMUFD manages the mappings and it
> should be populated from the IOAS.

OK. That looks better to me.

> Arguably drivers should EOPNOTSUPP if presented with a parent in this
> situation, but still this code should be clear about the purpose.
> 
> > +		hwpt = hwpt->parent;
> 
> And we definately shouldn't touch the parent. That is already setup
> and owned by someone else. Just return and don't do anything.

Yes.

Nic
Yi Liu March 23, 2023, 8:06 a.m. UTC | #6 
> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Friday, March 10, 2023 2:51 PM
 > 
> On Fri, Mar 10, 2023 at 10:25:10AM +0800, Baolu Lu wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > On 3/9/23 4:09 PM, Yi Liu wrote:
> > > From: Nicolin Chen <nicolinc@nvidia.com>
> > >
> > > A user-managed hw_pagetable does not need to get populated, since it
> is
> > > managed by a guest OS. Move the iopt_table_add_domain and
> list_add_tail
> > > calls into a helper, where the hwpt pointer will be redirected to its
> > > hwpt->parent if it's available.
> > >
> > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> > > Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> > > ---
> > >   drivers/iommu/iommufd/hw_pagetable.c | 20 ++++++++++++++++++--
> > >   1 file changed, 18 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/iommu/iommufd/hw_pagetable.c
> b/drivers/iommu/iommufd/hw_pagetable.c
> > > index 16e92a1c150b..6e45ec0a66fa 100644
> > > --- a/drivers/iommu/iommufd/hw_pagetable.c
> > > +++ b/drivers/iommu/iommufd/hw_pagetable.c
> > > @@ -43,6 +43,23 @@ int iommufd_hw_pagetable_enforce_cc(struct
> iommufd_hw_pagetable *hwpt)
> > >       return 0;
> > >   }
> > >
> > > +static int iommufd_hw_pagetable_link_ioas(struct
> iommufd_hw_pagetable *hwpt)
> > > +{
> > > +     int rc;
> > > +
> > > +     if (hwpt->parent)
> > > +             hwpt = hwpt->parent;
> > > +
> > > +     if (!list_empty(&hwpt->hwpt_item))
> > > +             return 0;
> >
> > What is above check for? Is it "the hwpt has already been inserted into
> > the hwpt list of its ioas in another place"?
> >
> > If so, is it possible that hwpt will be deleted from the list even when
> > this user hwpt is still linked to the ioas?
> 
> It means that the hwpt is already linked to the ioas. And the
> hwpt_item can be only empty after a destroy().
> 
> With that being said, after I think it through, perhaps Yi's
> previous change removing it might be better. So, it could be:
> 
> -------------------------------------------------------------------------------
> +	/*
> +	 * Only a parent hwpt needs to be linked to the IOAS. And a hwpt-
> >parent
> +	 * must be linked to the IOAS already, when it's being allocated.
> +	 */
>  	if (hwpt->parent)
> -		hwpt = hwpt->parent;
> -
> -	if (!list_empty(&hwpt->hwpt_item))
>  		return 0;
> 
> -------------------------------------------------------------------------------
> 
> I was concerned about the case where a device gets attached to
> the nested hwpt without staging at the parent hwpt first.

I think I was convinced with the reason that this helper may be
called by allocation for both standalone s2 hwpt and the nested
hwpt. So my change was not enough. Yours covers both cases.

> But,
> the link between the parent hwpt and the IOAS happened inside
> the allocation function now, not attach() any more.

Not quite get. This helper is also called in the allocation path. Is
it? Anyhow, with Jason's comment, this helper may be reworked.
We can sync later on the next version.

Regards,
Yi Liu
Nicolin Chen March 23, 2023, 8:12 a.m. UTC | #7 
On Thu, Mar 23, 2023 at 08:06:26AM +0000, Liu, Yi L wrote:
> External email: Use caution opening links or attachments
> 
> 
> > From: Nicolin Chen <nicolinc@nvidia.com>
> > Sent: Friday, March 10, 2023 2:51 PM
>  >
> > On Fri, Mar 10, 2023 at 10:25:10AM +0800, Baolu Lu wrote:
> > > External email: Use caution opening links or attachments
> > >
> > >
> > > On 3/9/23 4:09 PM, Yi Liu wrote:
> > > > From: Nicolin Chen <nicolinc@nvidia.com>
> > > >
> > > > A user-managed hw_pagetable does not need to get populated, since it
> > is
> > > > managed by a guest OS. Move the iopt_table_add_domain and
> > list_add_tail
> > > > calls into a helper, where the hwpt pointer will be redirected to its
> > > > hwpt->parent if it's available.
> > > >
> > > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> > > > Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> > > > ---
> > > >   drivers/iommu/iommufd/hw_pagetable.c | 20 ++++++++++++++++++--
> > > >   1 file changed, 18 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/iommu/iommufd/hw_pagetable.c
> > b/drivers/iommu/iommufd/hw_pagetable.c
> > > > index 16e92a1c150b..6e45ec0a66fa 100644
> > > > --- a/drivers/iommu/iommufd/hw_pagetable.c
> > > > +++ b/drivers/iommu/iommufd/hw_pagetable.c
> > > > @@ -43,6 +43,23 @@ int iommufd_hw_pagetable_enforce_cc(struct
> > iommufd_hw_pagetable *hwpt)
> > > >       return 0;
> > > >   }
> > > >
> > > > +static int iommufd_hw_pagetable_link_ioas(struct
> > iommufd_hw_pagetable *hwpt)
> > > > +{
> > > > +     int rc;
> > > > +
> > > > +     if (hwpt->parent)
> > > > +             hwpt = hwpt->parent;
> > > > +
> > > > +     if (!list_empty(&hwpt->hwpt_item))
> > > > +             return 0;
> > >
> > > What is above check for? Is it "the hwpt has already been inserted into
> > > the hwpt list of its ioas in another place"?
> > >
> > > If so, is it possible that hwpt will be deleted from the list even when
> > > this user hwpt is still linked to the ioas?
> >
> > It means that the hwpt is already linked to the ioas. And the
> > hwpt_item can be only empty after a destroy().
> >
> > With that being said, after I think it through, perhaps Yi's
> > previous change removing it might be better. So, it could be:
> >
> > -------------------------------------------------------------------------------
> > +     /*
> > +      * Only a parent hwpt needs to be linked to the IOAS. And a hwpt-
> > >parent
> > +      * must be linked to the IOAS already, when it's being allocated.
> > +      */
> >       if (hwpt->parent)
> > -             hwpt = hwpt->parent;
> > -
> > -     if (!list_empty(&hwpt->hwpt_item))
> >               return 0;
> >
> > -------------------------------------------------------------------------------
> >
> > I was concerned about the case where a device gets attached to
> > the nested hwpt without staging at the parent hwpt first.
> 
> I think I was convinced with the reason that this helper may be
> called by allocation for both standalone s2 hwpt and the nested
> hwpt. So my change was not enough. Yours covers both cases.
> 
> > But,
> > the link between the parent hwpt and the IOAS happened inside
> > the allocation function now, not attach() any more.
> 
> Not quite get. This helper is also called in the allocation path. Is
> it? Anyhow, with Jason's comment, this helper may be reworked.
> We can sync later on the next version.

We previously had this link_ioas() in attach() routine so we
needed to make sure hwpt->parent got populated, because the
device could be attached to an S1 HWPT directly. But now this
is in the alloc() routine, so by the time an S1 HWPT is being
allocated, an S2 HWPT must be allocated first and populated
already.

Nic
Yi Liu March 23, 2023, 8:28 a.m. UTC | #8 
> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Thursday, March 23, 2023 4:12 PM
> 
> On Thu, Mar 23, 2023 at 08:06:26AM +0000, Liu, Yi L wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > > From: Nicolin Chen <nicolinc@nvidia.com>
> > > Sent: Friday, March 10, 2023 2:51 PM
> >  >
> > > On Fri, Mar 10, 2023 at 10:25:10AM +0800, Baolu Lu wrote:
> > > > External email: Use caution opening links or attachments
> > > >
> > > >
> > > > On 3/9/23 4:09 PM, Yi Liu wrote:
> > > > > From: Nicolin Chen <nicolinc@nvidia.com>
> > > > >
> > > > > A user-managed hw_pagetable does not need to get populated,
> since it
> > > is
> > > > > managed by a guest OS. Move the iopt_table_add_domain and
> > > list_add_tail
> > > > > calls into a helper, where the hwpt pointer will be redirected to its
> > > > > hwpt->parent if it's available.
> > > > >
> > > > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> > > > > Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> > > > > ---
> > > > >   drivers/iommu/iommufd/hw_pagetable.c | 20
> ++++++++++++++++++--
> > > > >   1 file changed, 18 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/iommu/iommufd/hw_pagetable.c
> > > b/drivers/iommu/iommufd/hw_pagetable.c
> > > > > index 16e92a1c150b..6e45ec0a66fa 100644
> > > > > --- a/drivers/iommu/iommufd/hw_pagetable.c
> > > > > +++ b/drivers/iommu/iommufd/hw_pagetable.c
> > > > > @@ -43,6 +43,23 @@ int
> iommufd_hw_pagetable_enforce_cc(struct
> > > iommufd_hw_pagetable *hwpt)
> > > > >       return 0;
> > > > >   }
> > > > >
> > > > > +static int iommufd_hw_pagetable_link_ioas(struct
> > > iommufd_hw_pagetable *hwpt)
> > > > > +{
> > > > > +     int rc;
> > > > > +
> > > > > +     if (hwpt->parent)
> > > > > +             hwpt = hwpt->parent;
> > > > > +
> > > > > +     if (!list_empty(&hwpt->hwpt_item))
> > > > > +             return 0;
> > > >
> > > > What is above check for? Is it "the hwpt has already been inserted into
> > > > the hwpt list of its ioas in another place"?
> > > >
> > > > If so, is it possible that hwpt will be deleted from the list even when
> > > > this user hwpt is still linked to the ioas?
> > >
> > > It means that the hwpt is already linked to the ioas. And the
> > > hwpt_item can be only empty after a destroy().
> > >
> > > With that being said, after I think it through, perhaps Yi's
> > > previous change removing it might be better. So, it could be:
> > >
> > > -------------------------------------------------------------------------------
> > > +     /*
> > > +      * Only a parent hwpt needs to be linked to the IOAS. And a hwpt-
> > > >parent
> > > +      * must be linked to the IOAS already, when it's being allocated.
> > > +      */
> > >       if (hwpt->parent)
> > > -             hwpt = hwpt->parent;
> > > -
> > > -     if (!list_empty(&hwpt->hwpt_item))
> > >               return 0;
> > >
> > > -------------------------------------------------------------------------------
> > >
> > > I was concerned about the case where a device gets attached to
> > > the nested hwpt without staging at the parent hwpt first.
> >
> > I think I was convinced with the reason that this helper may be
> > called by allocation for both standalone s2 hwpt and the nested
> > hwpt. So my change was not enough. Yours covers both cases.
> >
> > > But,
> > > the link between the parent hwpt and the IOAS happened inside
> > > the allocation function now, not attach() any more.
> >
> > Not quite get. This helper is also called in the allocation path. Is
> > it? Anyhow, with Jason's comment, this helper may be reworked.
> > We can sync later on the next version.
> 
> We previously had this link_ioas() in attach() routine so we
> needed to make sure hwpt->parent got populated, because the
> device could be attached to an S1 HWPT directly. But now this
> is in the alloc() routine, so by the time an S1 HWPT is being
> allocated, an S2 HWPT must be allocated first and populated
> already.

Aha, yes. 😊
diff mbox series

Patch

diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c
index 16e92a1c150b..6e45ec0a66fa 100644
--- a/drivers/iommu/iommufd/hw_pagetable.c
+++ b/drivers/iommu/iommufd/hw_pagetable.c
@@ -43,6 +43,23 @@  int iommufd_hw_pagetable_enforce_cc(struct iommufd_hw_pagetable *hwpt)
 	return 0;
 }
 
+static int iommufd_hw_pagetable_link_ioas(struct iommufd_hw_pagetable *hwpt)
+{
+	int rc;
+
+	if (hwpt->parent)
+		hwpt = hwpt->parent;
+
+	if (!list_empty(&hwpt->hwpt_item))
+		return 0;
+
+	rc = iopt_table_add_domain(&hwpt->ioas->iopt, hwpt->domain);
+	if (rc)
+		return rc;
+	list_add_tail(&hwpt->hwpt_item, &hwpt->ioas->hwpt_list);
+	return 0;
+}
+
 /**
  * iommufd_hw_pagetable_alloc() - Get an iommu_domain for a device
  * @ictx: iommufd context
@@ -131,10 +148,9 @@  iommufd_hw_pagetable_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas,
 			goto out_unlock;
 	}
 
-	rc = iopt_table_add_domain(&hwpt->ioas->iopt, hwpt->domain);
+	rc = iommufd_hw_pagetable_link_ioas(hwpt);
 	if (rc)
 		goto out_detach;
-	list_add_tail(&hwpt->hwpt_item, &hwpt->ioas->hwpt_list);
 
 	mutex_unlock(&idev->igroup->lock);
 	return hwpt;