Re: [PATCH] iommufd/device: Enforce reserved IOVA also when attached to hwpt_nested

[Nicolin Chen <nicolinc@nvidia.com>]

On Mon, Aug 05, 2024 at 07:40:45AM +0000, Tian, Kevin wrote:
> > From: Nicolin Chen <nicolinc@nvidia.com>
> > Sent: Friday, August 2, 2024 1:35 PM
> >
> >  static int
> > -iommufd_group_do_replace_paging(struct iommufd_group *igroup,
> > -                             struct iommufd_hwpt_paging *hwpt_paging)
> > +iommufd_group_do_replace_reserved_iova(struct iommufd_group *igroup,
> > +                                    struct iommufd_hw_pagetable *hwpt)
> >  {
> > +     struct iommufd_hwpt_paging *hwpt_paging =
> > to_hwpt_paging(hwpt);
> >       struct iommufd_hw_pagetable *old_hwpt = igroup->hwpt;
> >       struct iommufd_device *cur;
> >       int rc;
> >
> >       lockdep_assert_held(&igroup->lock);
> >
> > -     if (!hwpt_is_paging(old_hwpt) ||
> > -         hwpt_paging->ioas != to_hwpt_paging(old_hwpt)->ioas) {
> > +     if (!hwpt_paging)
> > +             return 0;
> > +
> > +     if (iommufd_hw_pagetable_compare_ioas(old_hwpt, hwpt)) {
> 
> hmm this change is broken. In this helper:
> 
>         if (!old_hwpt_paging || !new_hwpt_paging)
>                 return false;
>         return old_hwpt_paging->ioas != new_hwpt_paging->ioas;
> 
> Obviously the original code wants to enforce reserved regions if
> new_hwpt is paging && old_hwpt is not paging, but this change
> skips this scenario.

Hmm..I think that is the intention of this patch?

The original code does that because it didn't enforce reserved
region (to the parent paging hwpt) when attaching the group to
a nested one. Now, it does. So, we basically check whether the
associated ioas has changed or not. Right?

Perhaps I should have added a line of comments to highlight it
getting the parent hwpt pointer now for a nested hwpt.

> >
> >       rc = iommufd_hwpt_replace_device(idev, hwpt, old_hwpt);
> >       if (rc)
> >               goto err_unresv;
> >
> > -     if (hwpt_is_paging(old_hwpt) &&
> > -         (!hwpt_is_paging(hwpt) ||
> > -          to_hwpt_paging(hwpt)->ioas != to_hwpt_paging(old_hwpt)->ioas))
> > -             iommufd_group_remove_reserved_iova(igroup,
> > -                                                to_hwpt_paging(old_hwpt));
> > +     if (iommufd_hw_pagetable_compare_ioas(old_hwpt, hwpt))
> > +             iommufd_group_remove_reserved_iova(igroup, old_hwpt);
> 
> this is also broken.
> 
> Probably it's clearer to continue open-coding those conditions in
> iommufd_group_do_replace_reserved_iova() and
> iommufd_group_remove_reserved_iova().

Here it basically compares the ioas too. The original code too,
while having an additional hwpt_is_paging() check on both hwpts
because of the same reason.

Thanks
Nicolin