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

Liu, Yi L
2019-12-05T12:19:19Z

> 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