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

From: Baolu Lu
Date: Fri Mar 10 2023 - 07:52:09 EST


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@xxxxxxxxxx>

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@xxxxxxxxxx>
Signed-off-by: Yi Liu <yi.l.liu@xxxxxxxxx>
---
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