Re: [PATCH v6 0/6] iommufd: Add nesting infrastructure (part 2/2)

[Nicolin Chen]

On Tue, Dec 12, 2023 at 03:21:00PM -0400, Jason Gunthorpe wrote:
> On Tue, Dec 12, 2023 at 11:13:37AM -0800, Nicolin Chen wrote:
> > On Tue, Dec 12, 2023 at 10:44:21AM -0400, Jason Gunthorpe wrote:
> > > On Mon, Dec 11, 2023 at 11:30:00PM -0800, Nicolin Chen wrote:
> > > 
> > > > > > Could the structure just look like this?
> > > > > > struct iommu_dev_assign_virtual_id {
> > > > > >        __u32 size;
> > > > > >        __u32 dev_id;
> > > > > >        __u32 id_type;
> > > > > >        __u32 id;
> > > > > > };
> > > > > 
> > > > > It needs to take in the viommu_id also, and I'd make the id 64 bits
> > > > > just for good luck.
> > > > 
> > > > What is viommu_id required for in this context? I thought we
> > > > already know which SMMU instance to issue commands via dev_id?
> > > 
> > > The viommu_id would be the container that holds the xarray that maps
> > > the vRID to pRID
> > > 
> > > Logically we could have multiple mappings per iommufd as we could have
> > > multiple iommu instances working here.
> > 
> > I see. This is the object to hold a shared stage-2 HWPT/domain then.
> 
> It could be done like that, yes. I wasn't thinking about linking the
> stage two so tightly but perhaps? If we can avoid putting the hwpt
> here that might be more general.
> 
> > // iommufd_private.h
> > 
> > enum iommufd_object_type {
> > 	...
> > +	IOMMUFD_OBJ_VIOMMU,
> > 	...
> > };
> > 
> > +struct iommufd_viommu {
> > +	struct iommufd_object obj;
> > +	struct iommufd_hwpt_paging *hwpt;
> > +	struct xarray devices;
> > +};
> > 
> > struct iommufd_hwpt_paging hwpt {
> > 	...
> > +	struct list_head viommu_list;
> > 	...
> > };
> 
> I'd probably first try to go backwards and link the hwpt to the
> viommu.

I think a VM should have only one hwpt_paging object while one
or more viommu objects, so we could do only viommu->hwpt_paging
and hwpt_paging->viommu_list. How to go backwards?

> > struct iommufd_group {
> > 	...
> > +	struct iommufd_viommu *viommu; // should we attach to viommu instead of hwpt?
> > 	...
> > };
> 
> No. Attach is a statement of translation so you still attach to the HWPT.

OK. It's probably not necessary since we know which piommu the
device is behind. And we only need to link viommu and piommu,
right?

> > Question to finalize how we maps vRID-pRID in the xarray:
> > how should IOMMUFD_DEV_INVALIDATE work? The ioctl structure has
> > a dev_id and a list of commands that belongs to the device. So,
> > it forwards the struct device pointer to the driver along with
> > the commands. Then, doesn't the driver already know the pRID 
> > from the dev pointer without looking up a vRID-pRID table?
> 
> The first version of DEV_INVALIDATE should have no xarray. The
> invalidate commands are stripped of the SID and executed on the given
> dev_id period. VMM splits up the invalidate command list.

Yes. This makes sense to me. VMM knows which device to issue
an IOMMUFD_DEV_INVALIDATE from the vSID/vRID in the commands.

> The second version maybe we have the xarray, or maybe we just push the
> xarray to the eventual viommu series.

I think that I still don't get the purpose of the xarray here.
It was needed previously because a cache invalidate per hwpt
doesn't know which device. Now IOMMUFD_DEV_INVALIDATE knows.

Maybe it's related to that narrative "logically we could have
multiple mappings per iommufd" that you mentioned above. Mind
elaborating a bit?

In my mind, viommu is allocated by VMM per piommu, by detecting
the piommu_id via hw_info. In that case, viommu can only have
one unique device list. If IOMMUFD_DEV_INVALIDATE passes in the
dev_id, we don't really need a mapping of vRID-pRID in a multi-
viommu case either? In another word, VMM already has a mapping
from vRID to dev_id, so it could call the DEV_INVALIDATE ioctl
in the first place?

Thanks
Nicolin