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

Jason Gunthorpe
2024-08-14T22:38:47Z

On Fri, Aug 09, 2024 at 01:23:12PM -0700, Nicolin Chen wrote:
> On Fri, Aug 09, 2024 at 02:00:10AM +0000, Tian, Kevin wrote:
> > > From: Nicolin Chen <nicolinc@nvidia.com>
> > > Sent: Wednesday, August 7, 2024 8:35 AM
> > >
> > > Currently, device reserved regions are only enforced when the device is
> > > attached to an hwpt_paging. In other words, if the device gets attached
> > > to an hwpt_nested directly, the parent hwpt_paging of the hwpt_nested's
> > > would not enforce those reserved IOVAs. This works for most of reserved
> > > region types, but not for IOMMU_RESV_SW_MSI, which is a unique software
> > > defined window, required by a nesting case too to setup an MSI doorbell
> > > on the parent stage-2 hwpt/domain.
> > >
> > > Kevin pointed out that:
> > > 1) there is no usage using up closely the entire IOVA space yet,
> > > 2) guest may change the viommu mode to switch between nested
> > > and paging then VMM has to take all devices' reserved regions
> > > into consideration anyway, when composing the GPA space.
> > > Link:
> > > https://lore.kernel.org/all/BN9PR11MB5276497781C96415272E6FED8CB12@
> > > BN9PR11MB5276.namprd11.prod.outlook.com/
> > >
> > > So it would be actually convenient for us to also enforce reserved IOVA
> > > onto the parent hwpt_paging, when attaching a device to an hwpt_nested.
> > >
> > > Repurpose the existing attach/replace_paging helpers to attach device's
> > > reserved IOVAs exclusively.
> > >
> > > Add a new find_hwpt_paging helper, which is only used by these reserved
> > > IOVA functions, to allow an IOMMUFD_OBJ_HWPT_NESTED hwpt to redirect
> > > to
> > > its parent hwpt_paging. Return a NULL in these two helpers for any new
> > > HWPT type in the future.
> > >
> > > Suggested-by: Tian, Kevin <kevin.tian@intel.com>
> > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> >
> > I'm not sure the name find_hwpt_paging() is good enough but can't
> > find a better alternative. So,
> >
> > Reviewed-by: Kevin Tian <kevin.tian@intel.com>
>
> Thanks for the review. I couldn't figure out a better name but
> that :)
>
> With the ongoing discussion with Robin at the other thread, we
> we might be able to get rid of the msi_cookie. If so, this one
> will be unnecessary. So, we might want to put this on hold?

Regardless this seems like a bug fix to me that we may as well
take. Unless we intend to retire the entire RESV_SW mechanism it
should work correctly.

When you say "enforce" you mean both that the reserved region list
reported to userspace does not include the region and that the
required mapping was never installed in the parent?

Jason