On Wed, Nov 13, 2019 at 07:43:43AM +0000, Liu, Yi L wrote:
> > From: Alex Williamson <alex.williamson@xxxxxxxxxx>
> > Sent: Wednesday, November 13, 2019 1:26 AM
> > To: Liu, Yi L <yi.l.liu@xxxxxxxxx>
> > Subject: Re: [RFC v2 3/3] vfio/type1: bind guest pasid (guest page tables) to host
> >
> > On Tue, 12 Nov 2019 11:21:40 +0000
> > "Liu, Yi L" <yi.l.liu@xxxxxxxxx> wrote:
> >
> > > > From: Alex Williamson < alex.williamson@xxxxxxxxxx >
> > > > Sent: Friday, November 8, 2019 7:21 AM
> > > > To: Liu, Yi L <yi.l.liu@xxxxxxxxx>
> > > > Subject: Re: [RFC v2 3/3] vfio/type1: bind guest pasid (guest page tables) to host
> > > >
> > > > On Thu, 24 Oct 2019 08:26:23 -0400
> > > > Liu Yi L <yi.l.liu@xxxxxxxxx> wrote:
> > > >
> > > > > This patch adds vfio support to bind guest translation structure
> > > > > to host iommu. VFIO exposes iommu programming capability to user-
> > > > > space. Guest is a user-space application in host under KVM solution.
> > > > > For SVA usage in Virtual Machine, guest owns GVA->GPA translation
> > > > > structure. And this part should be passdown to host to enable nested
> > > > > translation (or say two stage translation). This patch reuses the
> > > > > VFIO_IOMMU_BIND proposal from Jean-Philippe Brucker, and adds new
> > > > > bind type for binding guest owned translation structure to host.
> > > > >
> > > > > *) Add two new ioctls for VFIO containers.
> > > > >
> > > > > - VFIO_IOMMU_BIND: for bind request from userspace, it could be
> > > > > bind a process to a pasid or bind a guest pasid
> > > > > to a device, this is indicated by type
> > > > > - VFIO_IOMMU_UNBIND: for unbind request from userspace, it could be
> > > > > unbind a process to a pasid or unbind a guest pasid
> > > > > to a device, also indicated by type
> > > > > - Bind type:
> > > > > VFIO_IOMMU_BIND_PROCESS: user-space request to bind a process
> > > > > to a device
> > > > > VFIO_IOMMU_BIND_GUEST_PASID: bind guest owned translation
> > > > > structure to host iommu. e.g. guest page table
> > > > >
> > > > > *) Code logic in vfio_iommu_type1_ioctl() to handle
> > VFIO_IOMMU_BIND/UNBIND
> > > > >
> [...]
> > > > > +static long vfio_iommu_type1_unbind_gpasid(struct vfio_iommu *iommu,
> > > > > + void __user *arg,
> > > > > + struct vfio_iommu_type1_bind *bind)
> > > > > +{
> > > > > + struct iommu_gpasid_bind_data gbind_data;
> > > > > + unsigned long minsz;
> > > > > + int ret = 0;
> > > > > +
> > > > > + minsz = sizeof(*bind) + sizeof(gbind_data);
> > > > > + if (bind->argsz < minsz)
> > > > > + return -EINVAL;
> > > >
> > > > But gbind_data can change size if new vendor specific data is added to
> > > > the union, so kernel updates break existing userspace. Fail.
I guess we could take minsz up to the vendor-specific data, copy @format,
and then check the size of vendor-specific data?
> > >
> > > yes, we have a version field in struct iommu_gpasid_bind_data. How
> > > about doing sanity check per versions? kernel knows the gbind_data
> > > size of specific versions. Does it make sense? If yes, I'll also apply it
> > > to the other sanity check in this series to avoid userspace fail after
> > > kernel update.
> >
> > Has it already been decided that the version field will be updated for
> > every addition to the union?
>
> No, just my proposal. Jacob may help to explain the purpose of version
> field. But if we may be too "frequent" for an uapi version number updating
> if we inc version for each change in the union part. I may vote for the
> second option from you below.
>
> > It seems there are two options, either
> > the version definition includes the possible contents of the union,
> > which means we need to support multiple versions concurrently in the
> > kernel to maintain compatibility with userspace and follow deprecation
> > protocols for removing that support, or we need to consider version to
> > be the general form of the structure and interpret the format field to
> > determine necessary length to copy from the user.
>
> As I mentioned above, may be better to let @version field only over the
> general fields and let format to cover the possible changes in union. e.g.
> IOMMU_PASID_FORMAT_INTEL_VTD2 may means version 2 of Intel
> VT-d bind. But either way, I think we need to let kernel maintain multiple
> versions to support compatible userspace. e.g. may have multiple versions
> iommu_gpasid_bind_data_vtd struct in the union part.
I couldn't find where the @version field originated in our old
discussions, but I believe our plan for allowing future extensions was:
* Add new vendor-specific data by introducing a new format
(IOMMU_PASID_FORMAT_INTEL_VTD2, IOMMU_PASID_FORMAT_ARM_SMMUV2...), and
extend the union.
* Add a new common field, if it fits in the existing padding bytes, by
adding a flag (IOMMU_SVA_GPASID_*).
* Add a new common field, if it doesn't fit in the current padding bytes,
or completely change the structure layout, by introducing a new version
(IOMMU_GPASID_BIND_VERSION_2). In that case the kernel has to handle
both new and old structure versions. It would have both
iommu_gpasid_bind_data and iommu_gpasid_bind_data_v2 structs.
I think iommu_cache_invalidate_info and iommu_page_response use the same
scheme. iommu_fault is a bit more complicated because it's
kernel->userspace and requires some negotiation:
https://lore.kernel.org/linux-iommu/77405d39-81a4-d9a8-5d35-27602199867a@xxxxxxx/
[...]
> > If the ioctls have similar purpose and form, then re-using a single
> > ioctl might make sense, but BIND_PROCESS is only a place-holder in this
> > series, which is not acceptable. A dual purpose ioctl does not
> > preclude that we could also use a union for the data field to make the
> > structure well specified.
>
> yes, BIND_PROCESS is only a place-holder here. From kernel p.o.v., both
> BIND_GUEST_PASID and BIND_PROCESS are bind requests from userspace.
> So the purposes are aligned. Below is the content the @data[] field
> supposed to convey for BIND_PROCESS. If we use union, it would leave
> space for extending it to support BIND_PROCESS. If only data[], it is a little
> bit confusing why we define it in such manner if BIND_PROCESS is included
> in this series. Please feel free let me know which one suits better.
>
> +struct vfio_iommu_type1_bind_process {
> + __u32 flags;
> +#define VFIO_IOMMU_BIND_PID (1 << 0)
> + __u32 pasid;
> + __s32 pid;
> +};
> https://patchwork.kernel.org/patch/10394927/
Note that I don't plan to upstream BIND_PROCESS at the moment. It was
useful for testing but I don't know of anyone actually needing it.
> > > > That bind data
> > > > structure expects a format (ex. IOMMU_PASID_FORMAT_INTEL_VTD). How
> > does
> > > > a user determine what formats are accepted from within the vfio API (or
> > > > even outside of the vfio API)?
> > >
> > > The info is provided by vIOMMU emulator (e.g. virtual VT-d). The vSVA patch
> > > from Jacob has a sanity check on it.
> > > https://lkml.org/lkml/2019/10/28/873
> >
> > The vIOMMU emulator runs at a layer above vfio. How does the vIOMMU
> > emulator know that the vfio interface supports virtual VT-d? IMO, it's
> > not acceptable that the user simply assume that an Intel host platform
> > supports VT-d. For example, consider what happens when we need to
> > define IOMMU_PASID_FORMAT_INTEL_VTDv2. How would the user learn that
> > VTDv2 is supported and the original VTD format is not supported?
>
> I guess this may be another info VFIO_IOMMU_GET_INFO should provide.
> It makes sense that vfio be aware of what platform it is running on. right?
> After vfio gets the info, may let vfio fill in the format info. Is it the correct
> direction?
I thought you were planning to put that information in sysfs? We last
discussed this over a year ago so I don't remember where we left it. I
know Alex isn't keen on putting in sysfs what can be communicated through
VFIO, but it is a convenient way to describe IOMMU features:
http://www.linux-arm.org/git?p=linux-jpb.git;a=commitdiff;h=665370d5b5e0022c24b2d2b57975ef6fe7b40870;hp=7ce780d838889b53f5e04ba5d444520621261eda
My problem with GET_INFO was that it could be difficult to extend, and
to describe things like variable-size list of supported page table
formats, but I guess the new info capabilities make this easier.
Thanks,
Jean
