Message-ID: <Zr0+RSaQk8e/8pl1@Asurada-Nvidia>
Date: Wed, 14 Aug 2024 16:31:17 -0700
From: Nicolin Chen <nicolinc@...dia.com>
To: Jason Gunthorpe <jgg@...dia.com>
CC: "Tian, Kevin" <kevin.tian@...el.com>, "Liu, Yi L" <yi.l.liu@...el.com>,
	"iommu@...ts.linux.dev" <iommu@...ts.linux.dev>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] iommufd/device: Enforce reserved IOVA also when
 attached to hwpt_nested

On Wed, Aug 14, 2024 at 07:38:43PM -0300, Jason Gunthorpe wrote:
> 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@...dia.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@...el.com>
> > > > Signed-off-by: Nicolin Chen <nicolinc@...dia.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@...el.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.

I am hoping for that, yet can't guarantee of accomplishing it at
the end of the day. The SW-defined window can still be a default
region, stored somewhere in VFIO/IOMMUFD instead of the RESV_SW.
That being said, the alternative solution is gonna take a while
to finalize. So, probably doesn't hurt for us to have this patch.

> 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?

The "enforce" in the subject is a quote from this function:
iopt_table_enforce_dev_resv_regions().

So it might be more accurate with:
iommufd: Enforce device reserved regions as well when attached to hwpt_nested

Thanks
Nicolin

Powered by blists - more mailing lists