RE: [RFC v2 3/3] vfio/type1: bind guest pasid (guest page tables) to host

["Liu, Yi L" <yi.l.liu@intel.com>]

> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Tuesday, December 3, 2019 8:12 AM
> To: Liu, Yi L <yi.l.liu@intel.com>
> Subject: Re: [RFC v2 3/3] vfio/type1: bind guest pasid (guest page tables) to host
> 
> On Mon, 25 Nov 2019 07:45:18 +0000
> "Liu, Yi L" <yi.l.liu@intel.com> wrote:
> 
> > Hi Alex,
> >
> > Thanks for the review. Here I'd like to conclude the major opens in this
> > thread and see if we can get some agreements to prepare a new version.
> >
> > a) IOCTLs for BIND_GPASID and BIND_PROCESS, share a single IOCTL or two
> >    separate IOCTLs?
> >    Yi: It may be helpful to have separate IOCTLs. The bind data conveyed
> >    for BIND_GPASID and BIND_PROCESS are totally different, and the struct
> >    iommu_gpasid_bind_data has vendor specific data and may even have more
> >    versions in future. To better maintain it, I guess separate IOCTLs for
> >    the two bind types would be better. The structure for BIND_GPASID is
> >    as below:
> >
> >         struct vfio_iommu_type1_bind {
> >                 __u32                           argsz;
> >                 struct iommu_gpasid_bind_data   bind_data;
> >         };
> 
> 
> We've been rather successful at extending ioctls in vfio and I'm
> generally opposed to rampant ioctl proliferation.  If we added @flags
> to the above struct (as pretty much the standard for vfio ioctls), then
> we could use it to describe the type of binding to perform and
> therefore the type of data provided.  I think my major complaint here
> was that we were defining PROCESS but not implementing it.  We can
> design the ioctl to enable it, but not define it until it's implemented.

sure, so I'll pull back the @flags field. BTW. Regards to the payloads,
what would be preferred? @data[] or a wrapper structure like below?

	union {
		struct iommu_gpasid_bind_data   bind_gpasid;
	}bind_data;

> > b) how kernel-space learns the number of bytes to be copied (a.k.a. the
> >    usage of @version field and @format field of struct
> >    iommu_gpasid_bind_data)
> >    Yi: Jean has an excellent recap in prior reply on the plan of future
> >    extensions regards to @version field and @format field. Based on the
> >    plan, kernel space needs to parse the @version field and @format field
> >    to get the length of the current BIND_GPASID request. Also kernel needs
> >    to maintain the new and old structure versions. Follow specific
> >    deprecation policy in future.
> 
> Yes, it seems reasonable, so from the struct above (plus @flags) we
> could determine we have struct iommu_gpasid_bind_data as the payload
> and read that using @version and @format as outlined.

sure, thanks.

> > c) how can vIOMMU emulator know that the vfio interface supports to config
> >    dual stage translation for vIOMMU?
> >    Yi: may do it via VFIO_IOMMU_GET_INFO.
> 
> Yes please.

got it.

> > d) how can vIOMMU emulator know what @version and @format should be set
> >    in struct iommu_gpasid_bind_data?
> >    Yi: currently, we have two ways. First one, may do it via
> >    VFIO_IOMMU_GET_INFO. This is a natural idea as here @version and @format
> >    are used in vfio apis. It makes sense to let vfio to provide related info
> >    to vIOMMU emulator after checking with vendor specific iommu driver. Also,
> >    there is idea to do it via sysfs (/sys/class/iommu/dmar#) as we have plan
> >    to do IOMMU capability sync between vIOMMU and pIOMMU via sysfs. I have
> >    two concern on this option. Current iommu sysfs only provides vendor
> >    specific hardware infos. I'm not sure if it is good to expose infos
> >    defined in IOMMU generic layer via iommu sysfs. If this concern is not
> >    a big thing, I'm fine with both options.
> 
> This seems like the same issue we had with IOMMU reserved regions, I'd
> prefer that a user can figure out how to interact with the vfio
> interface through the vfio interface.  Forcing the user to poke around
> in sysfs requires the user to have read permissions to sysfs in places
> they otherwise wouldn't need.  Thanks,

thanks, let me prepare a new version.

Regards,
Yi Liu

> Alex